fix: don't panic the net task when an M3 lands late (#146)

[rsantacroce]

Fixes #146.

The reported bug

connect_withdrawal_bundle_submitted asserted that a bundle's WithdrawalBundleSubmitted event arrives exactly one sidechain block after the block that created it:

assert_eq!(bundle_block_height, block_height - 1);

@Coinelius's diagnosis in #146 is correct and the reasoning holds up: an M3 is a coinbase message, written by the block producer from its own enforcer's database, so it reaches the mainchain only when the sidechain operator themself mines a mainchain block. For an operator who is not the dominant miner that is an arbitrary number of blocks later, and the assert fires — killing the net task while the process stays up and the unit still reports active.

bundle_block_height is read nowhere else in the function, so the mismatch is now a warning and the submission is applied normally. saturating_sub also removes the height-0 underflow.

What the fix uncovered

Removing the assert exposed the rest of the same assumption, on both sides of the connect/disconnect pair. Neither was reachable before, because the assert killed the node first.

1. The inverse path guessed the creation height. disconnect_withdrawal_bundle_submitted restored the pending bundle with bundle_status.height - 1 — the same "M3 was prompt" assumption. The height had nowhere else to live: pending_withdrawal_bundle carries it, and that record is deleted when the submission is connected.

It is now recorded in a new withdrawal_bundle_creation_heights database keyed by m6id. A separate database rather than a field on the withdrawal_bundles record, so databases written by earlier versions keep working — a missing entry falls back to the old guess with a warning. NUM_DBS goes 17 → 18.

2. The consumer of that height was wrong regardless of M3 timing. connect stores the collected bundle keyed by the height it ran at; disconnect reads the same height for the same block (connect runs after the block is connected, disconnect before the tip is rolled back — the expired_swaps round trip relies on exactly this and its test passes). The cleanup compared against block_height - 1, which no block ever matched, so a bundle collected by a disconnected block stayed pending on the chain it had been reorged off. Its failure-gap comparison was also > where collection in connect uses >=, so the two disagreed at exactly WITHDRAWAL_BUNDLE_FAILURE_GAP.

Tests

Five new tests in lib/state/two_way_peg_data.rs, using the heights from the report (bundle at 11, M3 at 25). Four fail against the previous code:

Test	Against previous code
delayed_bundle_submission_applies_instead_of_panicking	assertion left == right failed, left: 11, right: 24
bundle_submission_at_height_zero_does_not_underflow	attempt to subtract with overflow
disconnecting_a_late_submission_restores_the_true_creation_height	restores 24 instead of 11
disconnect_drops_the_bundle_collected_by_that_block	cleanup never fires
disconnect_keeps_a_bundle_collected_by_an_earlier_block	passes either way — guards against over-deletion

cargo test --workspace 59 passed / 0 failed (was 54). cargo fmt --check, cargo clippy --all-targets --all-features, and cargo check --workspace --all-targets are all clean — the remaining clippy warnings are the pre-existing heed::EnvFlags::NO_TLS deprecations in lib/node/mod.rs and lib/wallet.rs.

Deliberately not in this PR

disconnect has two more assert_eq!(block_height - 1, ...) guards, on the deposit and withdrawal-bundle-event block records, with the same off-by-one and the same panic-on-a-worker-thread hazard. By the reasoning above they look wrong too, but confirming that needs a reorg fixture with real deposit events rather than a comparison read, and I would rather not bundle an unvalidated consensus-path change into a panic fix. Happy to take it on as a follow-up.

🤖 Generated with Claude Code

https://claude.ai/code/session_011UwnSESebfdHQkLs2arAEt

[Coinelius]

Thanks for turning this around so fast — and for chasing the assumption past the
line I reported.

Up front, so you can weigh it accordingly: I'm not an experienced developer —
infrastructure is my trade. Sections 1-4 below are a code review by Claude, which
has access to my node and its logs; I'm passing it on rather than vouching for it
myself. Section 5 is data from my own node, which I can stand behind. The
Coinshift node is stopped at the moment, but I'm happy to start it back up and run
whatever tests would be useful against the chain it holds.

---

Claude's review

The fix is correct, including the part flagged as the least certain inference.
Three things to add — one confirmation, one upgrade to the "deliberately not in
this PR" section, and one suggestion about the migration fallback — plus a minor
note.

1. The connect/disconnect height symmetry is confirmed in the caller

The PR justifies bundle_height == block_height from the expired_swaps round
trip. That inference is right, and it is load-bearing enough to be worth pinning
to the code that actually sequences it:

• block::connect_prevalidated_block writes the new height —
  state.height.put(rwtxn, &(), &pre.next_height) (lib/state/block.rs:455) —
  and net_task.rs calls it at line 104-118, before
  state.connect_two_way_peg_data(...) at lib/node/net_task.rs:119.
  So 2WPD connect for block N sees try_get_height() == N.
• state.disconnect_two_way_peg_data(...) (lib/node/net_task.rs:242) runs
  before state.disconnect_tip(...) (line 243), and disconnect_tip is what
  rolls the height back: state.height.put(rwtxn, &(), &(height - 1))
  (lib/state/block.rs:887).
  So 2WPD disconnect for block N also sees try_get_height() == N.

Both sides see N for the same block. bundle_height == block_height - 1 could
therefore never match, exactly as the PR concludes.

2. The two remaining asserts don't need a reorg fixture

The PR leaves these for their own change, because validating them looked like it
needed a reorg fixture with real deposit events. Given the sequencing above, it
doesn't — they fall out of the write/read pair by inspection:

connect writes	disconnect asserts
deposit_blocks ← (hash, block_height) — two_way_peg_data.rs:962	assert_eq!(block_height - 1, last_deposit_block_height) — :1437
withdrawal_bundle_event_blocks ← (hash, block_height) — :980	assert_eq!(block_height - 1, last_withdrawal_bundle_event_block_height) — :1399

connect stores N. disconnect compares N-1. The assert_eq! on the block hash
immediately above each one guarantees the record being compared is the one this
block wrote, so there is no case where the last record belongs to an earlier
block and the comparison accidentally holds. Both asserts fail unconditionally
whenever a disconnected block carried a deposit or a withdrawal-bundle event.

Same failure mode as #146: assert_eq! on a tokio worker, process survives,
systemd still green, net task gone.

These arguably belong in this PR rather than a follow-up, because this PR is what
makes the disconnect path reachable at all. Before it, the connect-side assert
killed the node first. After it, the first reorg that touches a deposit lands on
the next panic — a fix that trades a known panic for an unknown one is a worse
place to stop than either end.

3. The legacy fallback can be exact rather than a guess

The new withdrawal_bundle_creation_heights database is the right call — the
height genuinely isn't recoverable in the clear. But the fallback for databases
written by earlier versions:

.unwrap_or_else(|| bundle_status.height.saturating_sub(1))

reproduces the #146 bug for precisely the databases that hit #146. The alphanet
node's database is one of them: m6id
859739a4ad7f9d3830d7f53d11a0982de81ab5b456afcc686df69b81d7a7c3f3, true creation
height 11, submitted at 25. After upgrading, a disconnect of that block
restores the pending bundle at 24 — silently, with only a warning.

That's recoverable, because the bundle commits to its own creation height.
WithdrawalBundle::new (lib/types/mod.rs:198-215) builds

hash([spend_utxos.keys()..., OutPoint::Regular { txid: [0; 32], vout: block_height }])

and stores it as an OP_RETURN push in tx.output[1]. At the point the fallback
runs the bundle is already in hand (WithdrawalBundleInfo::Known), and
spend_utxos() / tx() are both public. So instead of guessing, the fallback can
test candidate heights against that commitment and take the one that matches —
bounded by the submission height, run once, only on a legacy disconnect. That
turns the one lossy path in the migration into an exact one.

If you'd rather keep the PR tight, a debug_assert! or a louder warning naming
the affected m6id would at least make the imprecision visible.

4. Minor, same family

lib/node/net_task.rs:153 and :165 both do applied_height < height - 1, which
underflows at height 0 (wraps in release rather than panicking, so it's low
severity). Worth a saturating_sub while the -1 convention is being swept.

---

5. From my node

The patched connect path has been running on slot 255 since 2026-09-01 — live
confirmation on real data rather than a fixture, at the same heights as your test:

2026-09-01T01:33:52.335119Z  WARN coinshift::state::two_way_peg_data:
  lib/state/two_way_peg_data.rs:130: Withdrawal bundle submitted later than the
  block after the one that created it
  bundle_block_height=11 block_height=25
  m6id=859739a4ad7f9d3830d7f53d11a0982de81ab5b456afcc686df69b81d7a7c3f3

2026-09-01T01:33:52.335143Z  INFO coinshift::state::two_way_peg_data:
  lib/state/two_way_peg_data.rs:156: Withdrawal bundle submitted to parent chain
  block_height=25 total_withdrawal_sats=1000000 total_spent_sats=1000000
  output_count=1

The bundle applied cleanly and the chain advanced 25 → 26, its first new block
since the panic.

Three corrections and notes to the record while I'm here:

• The m6id in assert_eq!(bundle_block_height, block_height - 1) panics the net task when an M3 lands more than one block after the bundle #146 and the one in these logs are the same value, byte-order
  reversed: f3c3a7d7…a4399785 (enforcer/L1 display) is the exact reverse of
  859739a4…d7a7c3f3 (node logs). Flagging it so nobody chases a phantom
  mismatch.
• The panic fired inside a connect of a candidate tip, not a reorg. I was
  imprecise about this in assert_eq!(bundle_block_height, block_height - 1) panics the net task when an M3 lands more than one block after the bundle #146. Full sequence: the assert panics at 05:22:04, then
  Not ready to reorg and mine: BMM not accepted as new tip follow ~0.8 s later
  as downstream effects, and the net task's death only surfaces at 05:38:21 on the
  next tip attempt. That 16-minute gap between the panic and any visible error is
  a good part of why this was hard to spot.
• The sidechain disconnect path has never run on this node — zero occurrences
  across the entire journal. That is why the two asserts in §2 have never fired
  here, and it is not evidence that they are safe.

---

As above, I can bring the node up and run any patch against it — it's the only
chain with a real M3 in its history, and I can reproduce a late submission on
demand.