consensus: skip future duplicate disconnected txids on reorg#167
Draft
l0rinc wants to merge 2 commits into
Draft
consensus: skip future duplicate disconnected txids on reorg#167l0rinc wants to merge 2 commits into
l0rinc wants to merge 2 commits into
Conversation
feature_block.py already builds a valid duplicate coinbase at height 120 after spending the earlier output at height 119. Extend that path with an equal-work fork from genesis so the test documents that the duplicate coinbase branch remains active until a stronger branch forces a disconnect. This prepares the reorg reproducer without changing behavior yet.
DisconnectedBlockTransactions stores transactions in a list and indexes the same entries by txid. A duplicate txid encountered while disconnecting a stronger genesis fork after a spent BIP30 duplicate coinbase is inserted into the list before the map detects the duplicate, so debug builds abort at the inserted assertion and non-assert builds can leave the list and map out of sync. Use the txid index insertion result to detect duplicates before appending to the list, so the second copy is ignored and the list and map stay in sync. Add direct unit coverage for the list/map invariant, then update feature_block.py to turn the existing BIP30 duplicate coinbase setup into a stronger genesis-fork reorg. The functional test invalidates that fork afterward so the rest of the test continues from the duplicate branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
DisconnectedBlockTransactionskeeps disconnected transactions in two separate structures,queuedTxanditers_by_txid, which must describe the same txid set (header). Before this change,AddTransactionsFromBlock()inserted intoqueuedTxbefore inserting intoiters_by_txid, then asserted that the map insertion succeeded (implementation).That assumption is too strong for historical duplicate coinbase txids. Core already documents the BIP30/BIP34 duplicate-coinbase edge case and the future-consensus-cleanup need around it (validation comments).
DisconnectTip()passes the full block transaction vector into the disconnect pool (call site); coinbases are skipped only later whenMaybeUpdateMempoolForReorg()drains the pool (mempool resurrection loop). During a chain switch, one disconnect pool is reused while disconnecting all blocks back to the fork point before mempool resurrection (chain switch loop).Fix
Make duplicate txids no-ops for the disconnect pool by inserting into
iters_by_txidfirst withtry_emplace()and appending toqueuedTxonly for newly queued txids. This keeps the list, map, and cached memory usage synchronized.The duplicate branch remains a runtime check rather than an assert because historical duplicate coinbase txids are still reachable. BIP54 proposes coinbase
nLockTime/nSequencecommitments that make future duplicate coinbases impossible after activation (BIP54 specification, rationale), at which point this no-op can be reconsidered as an assert.Related Implementations
Knots currently has the same list-plus-map shape and insertion-before-map assertion as Core (Knots implementation, storage members/comment).
Bitcoin Cash Node and Bitcoin ABC use a different disconnect-pool container with a
boost::multi_index::hashed_uniquetxid index, so they do not have Core’s list/map divergence failure mode (BCHN current container/accounting, ABC current container). That unique container shape was introduced byf18009a4d63e(BCHN/ABC commit); the explicit duplicate skip inaddForBlock()was introduced by904931e89007(commit, ABC current skip). BCHN later made memory accounting defensive on successful insert infa0d374801ad(commit).It's not obvious what libbitcoin does, but the reorg code I found is chaser/archive based (confirm reorg, set reorganized, organize path), and I did not find a Core-style
DisconnectedBlockTransactionsanalogue to cite as prior art.