BIP54 "Consensus Cleanup" implementation #99
Conversation
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
In the assumeutxo functional tests, the final test case with alternated UTxO data tests the error raised when deserializing a snapshot that contains a coin with an amount not in range (<0 or >MAX_MONEY). The current malleation uses an undocumented byte string and offset which makes it hard to maintain. In addition, the undocumented offset is set surprisingly high (39 bytes is well into the serialization of the amount which starts at offset 36). Similarly the value is surprisingly small, presumably one was adjusted for the other. But there is no comment explaining how they were chosen, why not in a clearer manner and what they are supposed to represent. Instead replace this seemingly magic value with a clear one, MAX_MONEY + 1, serialize the whole value for the amount field at the correct offset, and document the whole thing for the next person around.
This test case is brittle as it asserts a specific error string, when the error string depends on data in the snapshot not controlled by the test (i.e. not injected by the test before asserting the error string). This can be fixed in a more involved way as per Bitcoin Core PR 32117, but since this PR is now closed in Core, in the meantime just disable the brittle test in inquisition (see discussion in Bitcoin Inquisition PR 90).
We don't set the nSequence as it will be set directly in the block template generator in a following commit.
The Consensus Cleanup soft fork proposal includes enforcing that coinbase transactions set their locktime field to the block height, minus 1 (as well as their nSequence such as to not disable the timelock). If such a fork were to be activated by Bitcoin users, miners need to be ready to produce compliant blocks at the risk of losing substantial amounts mining would-be invalid blocks. As miners are unfamously slow to upgrade, it's good to make this change as early as possible. Although Bitcoin Core's GBT implementation does not provide the "coinbasetxn" field, and mining pool software crafts the coinbase on its own, updating the Bitcoin Core mining code is a first step toward convincing pools to update their (often closed source) code. A possible followup is also to introduce new fields to GBT. In addition, this first step also makes it possible to test future Consensus Cleanup changes. The changes to the seemingly-unrelated RBF tests is because these tests assert an error message which may vary depending on the txid of the transactions used in the test. This commit changes the coinbase transaction structure and therefore impact the txid of transactions in all tests. The change to the "Bad snapshot" error message in the assumeutxo functional test is because this specific test case reads into the txid of the next transaction in the snapshot and asserts the error message based it gets on deserializing this txid as a coin for the previous transaction. As this commit changes this txid it impacts the deserialization error raised.
This encapsulates the soft fork configuration logic as set by the `-testactivationheight` (for buried deployments) and `-vbparams` (for version bits deployments) options which for the moment are regtest-only, in order to make them available on other networks as well in the next commit. Can be reviewed using git's --color-moved option.
This allows to set `-testactivationheight` and `-vbparams` on all networks instead of exclusively on regtest.
Prior commits are preparatory work. Following commits is the implementation of BIP54.
Move the function that checks whether a transaction respects the BIP54 sigops rule to the consensus folder (along with the accompanying constant), as it will be made consensus-critical in the next commit. Can be reviewed with git's --color-moved option.
When BIP54 is active, make sure transaction in blocks do not violate the BIP54 limit on the number of potentially-executed legacy sigops.
In Taproot the signature commits to the list of spent outputs.
Test the newly introduced limit with various combinations of inputs and outputs types, historical transactions, and exercise some implementation-specific edge cases. Record each test case and optionally write them to disk as JSON to generate the BIP test vectors.
The fuzz target was specifically crafted to support seeding it with the BIP54 test vectors generated by the unit test in the previous commit.
We are going to introduce the timewarp fix for mainnet with a greater grace period. Rename the MAX_TIMEWARP value for testnet to differentiate them.
The test vectors were generated using https://github.com/darosior/bitcoin/tree/bip54_miner
That is, enforce nLockTime be set to height-1 and nSequence not be set to final.
The test vectors were generated using https://github.com/darosior/bitcoin/tree/bip54_miner
darosior
force-pushed
the
2509_inquisition_consensus_cleanup
branch
from
ee557fc to
bc95a67
Compare
… vectors) This adds tests exercising the bounds of the checks on the invalid transaction size, for various types of transactions (legacy, Segwit, bytes in input/output to get to 64 bytes) as well as sanity checking against some known historical violations. Thanks to Chris Stewart for digging up the historical violations to this rule.
It's not a standardness limit anymore, it was made consensus.
The previously introduced unit tests extensively test the specific implementation of each mitigation. This functional test complements them by sanity checking all mitigations in a "black box" manner. For the added timestamp constraints, it mimicks how they would get exploited (by implementing pseudo timewarp and Murch-Zawy attacks) and demonstrates those exploits are not possible anymore after BIP54 activates.
darosior
force-pushed
the
2509_inquisition_consensus_cleanup
branch
from
bc95a67 to
8d513a0
Compare
Collaborator
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
This was referenced Oct 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Based on top of #98, this implements the BIP54 consensus rules.
Each of the 4 mitigations comes with its own unit test in a new
src/test/bip54_tests.cppmodule. The unit tests can be used to generate JSON test vectors for the BIP (opt-in). For those unit tests that require mainnet blocks, the JSON test vectors were generated separately and included in this PR as data files.Besides the unit test for each mitigation, this contains a fuzz harness for the sigop accounting logic as well as a functional test which goes over all mitigations in pseudo realistic situations (for instance, mimic a Timewarp attack and a Murch-Zawy attack to demonstrate the new timestamp restrictions would prevent that). The fuzz target for the sigop accounting logic was designed to make it possible to seed it from the BIP test vectors. This branch implements that.
The chains of headers for the timestamp restrictions test vectors were generated using this unit test (note the premined headers to allow modifications without re-generating all headers from scratch). The small chains of blocks for the coinbase restrictions test vectors were generated using this unit test.