29.x: Implement BIP 54 (Consensus Cleanup) without mainnet activation - #11
Open
darosior wants to merge 38 commits into
Open
29.x: Implement BIP 54 (Consensus Cleanup) without mainnet activation#11darosior wants to merge 38 commits into
darosior wants to merge 38 commits into
Conversation
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.
The chain starts at block 1, not genesis.
The assumeutxo data for the fuzz target could change and invalidate the hash silently, preventing the fuzz target from reaching some code paths. Fix this by sanity checking the snapshot values during initialization.
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).
… framework This constant was introduced in Bitcoin Core PR 31953 (commit fa86190), and PR 32155, which we are about to backport, depends on it. Instead of backporting unrelated PR 31953, just introduce the constant here.
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 with `--color-moved-ws=allow-indentation-change`.
…orks This allows unit tests to set `-testactivationheight` and `-vbparams` on all networks instead of exclusively on regtest. Those are kept test-network-only when used as startup parameters.
Some functional tests were still hardcoding parameters. Using the constant allows to change the rules in a single place if necessary.
Prior commits are preparatory work. Following commits is the implementation of BIP54.
BIP54 counts sigops differently from existing sigops-based checks. Since we are overloading the sigops term, make clear the constant refers to BIP54-sigops, not other kinds of pre-existing sigops. -BEGIN VERIFY SCRIPT- sed -i 's/MAX_TX_LEGACY_SIGOPS/MAX_TX_BIP54_SIGOPS/g' $(git grep -l MAX_TX_LEGACY_SIGOPS src/) -END VERIFY SCRIPT-
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, enforce that block transactions do not violate the BIP54 limit on the number of legacy sigops present in Scripts that get executed during block validation.
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. -BEGIN VERIFY SCRIPT- for f in $(git grep -l MAX_TIMEWARP); do sed -i "s/MAX_TIMEWARP/MAX_TIMEWARP_TESTNET4/g" "$f"; done -END VERIFY SCRIPT-
Documentation about the test vectors, including about their structure and content, as well as reproduction instructions, is available here: https://github.com/bitcoin/bips/tree/master/bip-0054/test_vectors
…k height When BIP 54 is active, coinbase transactions must have their nLockTime field set to the block height minus 1 (since it encodes the last height at which the transaction is invalid), and their nSequence field may be anything but the maximum value (which indicates "final", bypassing timelock validation).
Documentation about the test vectors' structure and content, as well as instructions for generating them is available at https://github.com/bitcoin/bips/tree/master/bip-0054/test_vectors .
… 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. Thanks to Anthony Towns for the scripted diff script. -BEGIN VERIFY SCRIPT- sed -i 's/MAX_STD_LEGACY_SIGOPS/MAX_TX_BIP54_SIGOPS/g' $(git grep -l MAX_STD_LEGACY_SIGOPS) sed -i 's/signature operations in validating a transaction./signature operations in a single transaction, per BIP54./' test/functional/test_framework/script_util.py -END VERIFY SCRIPT- Co-Authored-by: Anthony Towns <aj@erisian.com.au>
The previously introduced unit tests extensively test the specific implementation of each mitigation. This functional test complements them by end-to-end testing all mitigations. 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.
Owner
Author
|
Note to self: on 29.x, bitcoin#33050 was not backported. Therefore we should be more careful about returning a consensus error for BIP 54 failures during mempool submission prior to activation. |
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
4 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.
This is a backport of #8 for version 29.
It's based on a backport of the preparatory PRs bitcoin#31907, bitcoin#31910, bitcoin#32155, bitcoin#35333 and bitcoin#35335.