Repository navigation
net: keep wallet transactions in the mempool after a multi-block reorg - #1243
niteshbalusu11 wants to merge 8 commits into
Conversation
A branch reorg re-added the disconnected transactions on the fork point, before the new branch connected. A Core wallet stamps nLockTime with the height it last saw, so a child confirmed in the second disconnected block was not final there. It failed NotFinal and was lost from the mempool and the orphanage (reardencode#1236). As Core does in ActivateBestChainStep, the branch reorg now holds the template mark across the disconnect and the connect, and re-adds the transactions once on the tip that results, oldest block first. A restore after a failed connect re-adds the dropped prefix the same way. With parents first, a missing input is a parent that did not come back, so the re-add drops the tx instead of parking it in the orphanage. invalidateblock and the IBD rewind still re-add on the fork point, before the RAM scripthash head drops. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Electrum restatuses every subscription on a reorg tip event. connect_at sent those events while the disconnected transactions were still out of the pool, and their re-add sends no announcement, so a watched script could keep a status without them. The branch reorg now carries its template hold, the disconnected transactions, and the tip events in one BranchReorg. connect_at queues its event there, and the reorg sends them once the transactions are back, as Core signals BlockConnected after MaybeUpdateMempoolForReorg. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![rearden-grok[bot]](https://avatars.githubusercontent.com/u/730881?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES. Two bugs.
The success path pushes a scripthash status with the disconnected transaction missing. See the note on drop_above in accept_branch_disconnect.
After a failed connect, tip events for the rolled-back prefix are still delivered. See the note on finish_branch_reorg.
Nit: disconnect_harvest_in_commit treats a block_at_height error as an empty block and still returns Ok, so that block's transactions never re-enter.
A failed reorg connect restores the old branch, but the tip events of the prefix it had connected stayed queued. They went out after the restore with reorg_branch_len set, so Electrum pushed the abandoned header and peers were told about it while the old tip was back. The restore now drops those events once the prefix is disconnected. The old branch's reconnect events still go out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A branch reorg walks the scripthash watermark back at its disconnect and re-adds the disconnected txs only after the new branch connects. The 50 ms tick saw the walk-back, rescanned, and pushed a status with those txs in neither the index nor the pool. TemplateHold now bumps a settle counter when it is taken and again when it drops, so the count is odd while a connect, disconnect, or reorg is out. The tick reads statuses only while the count is even, and drops a read during which the count changed. It reads again on the next tick, and sh_seen does not move until a read lands. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tick was held back while a reorg held the pool, but a tip event or a pool-admit announcement read in that window still pushed a status with the disconnected txs in neither the index nor the pool. That happens when an earlier reorg's events are read after a later reorg has started. emit_sh_notes and the announcement arm now read through settled_restatus_notes. A dropped read sets sh_rescan, and the next tick reads every sub again even if the watermark is back where it was, which a same-height reorg leaves it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Note 🤖 Claude Opus 5.5 responding on behalf of niteshbalusu11 @reardencode this is ready for another look. It is rebased onto master 5cc40ff, and all required checks are green on b7a7a31. Both of rearden-grok's findings are fixed, with a reply on each thread. f3612f4 stops announcing the prefix that a restore dropped. 2189520 and b7a7a31 hold every Electrum scripthash status read until the pool settles. The |
There was a problem hiding this comment.
COMMENT. The two earlier bugs are fixed. The harvest nit is still open, and the new electrum tests each wait on the wall clock.
The scripthash push hole is closed. TemplateHold bumps pool_settle on take and on every drop, including a hold that never commits a hash. Tip, tick, and admit reads go through settled_restatus_notes. A dropped read sets sh_rescan, so the next tick restatuses every subscription even when the watermark is back where it started. Holds take template_prev, so they cannot overlap and the counter cannot read even while one is out. Drop runs on unwind, so an early return does not leave the count odd.
The abandoned header is closed. accept_branch_restore clears reorg.tips after the prefix disconnect and before the old branch is connected. The restore test rejects that hash.
The block_at_height nit is still open. See disconnect_harvest_in_commit.
expect_push_after waits 400ms of wall time, and the three new electrum tests each call it. See that line. The block/bip34 job moving from 2.06s to 3.44s is the same 298 tests, and this diff does not touch that crate.
disconnect_harvest_in_commit took a block_at_height error as an empty block, disconnected it, and returned Ok, so that block's transactions never went back in the pool. The error now returns before the block leaves the chain. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The three held-pool tests waited 400 ms of wall time for a push that should not come, which also passes when the runtime stalls before a tick. They now serve one connection over a duplex pipe on a paused clock. They assert pool_settle_gen is odd while held, and use a ping as the barrier: the server polls ticks and events before the next request, so a ping answered first means those reads pushed nothing. The tick test advances the clock so a tick is due before the ping. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The previous commit returned on a block read fault from inside the disconnect loop. A rewind past the 16 cached bodies disconnected those blocks and then stopped on the first unreadable one, skipping the cache truncation, the pool re-add, the scripthash and chain-work cleanup, and the template mark. disconnect_harvest_in_commit now reads every body above keep_height first, so a read fault leaves the chain where it was. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Note 🤖 Claude Opus 5.5 responding on behalf of niteshbalusu11 @reardencode this is ready for another look. It is rebased onto master 5cc40ff, and all required checks are green on 042ca84. rearden-grok's second review left two items, and both are fixed, with a reply on each thread. 25163be and 042ca84 make a disconnect read every block it drops before the first disconnect, so a body that cannot be read fails the disconnect with the chain untouched. dd57671 moves the three held-pool electrum tests to a paused clock with a ping barrier, so they no longer wait on wall time. |
After a reorg that replaces two or more blocks, rbitcoin lost transactions that Bitcoin Core puts back (#1236). We re-added the disconnected transactions on the fork point, before the new branch connected. A Core wallet sets
nLockTimeto the height it last saw, so a transaction confirmed in the second disconnected block is not final on the fork point. It failedNotFinaland was dropped from both the mempool and the orphanage. The issue's script sometimes passed only because Core re-relayed the child within its 5 s wait.A branch reorg now follows Core's
ActivateBestChainStep. It disconnects, connects the new branch, then re-adds the transactions once on the resulting tip, oldest block first. Its tip events (Electrum restatus, peer announcements) go out after that re-add. A restore after a failed connect does not announce the prefix it dropped. The template hold covers the whole reorg, sogetblocktemplatenever sees the pool in between. As in Core, a transaction whose parent does not come back is dropped and is not parked in the orphanage.invalidateblockand the IBD rewind still re-add on the fork point. A disconnect reads every block it drops before it starts, so a block that cannot be read back fails the disconnect instead of losing its transactions.While the new branch connects, a disconnected transaction is in neither the mempool overlay nor the scripthash index. An Electrum query in that window does not see it. Subscription pushes wait: a status read that overlaps a held pool is dropped, and the next tick reads every subscription again.
Proved with regtest tests that fail on master or before each follow-up, and with the issue's script against Core 31.1. Master logs
re-accepted 1/2and loses the child. This branch logsre-accepted 2/2and matched Core's mempool in 14/14 runs.Reviewed with sol-loop: 6 rounds with gpt-6.1-sol on high. Sol verdict: approved.
Fixes #1236.
Created with Claude Opus 5.5 in Claude Code.