Consensus Cleanup preparation: backport "miner: timelock the coinbase to the mined block's height" #90
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
This PR/commit seems to have been soft-rejected upstream, so doesn't seem very appealing to maintain here. Rather than do that or change magic values, perhaps it would be simpler to just remove the "Bad snapshot data after deserializing 1 coins" case, without replacing it? |
darosior
force-pushed
the
202509_inquisition_32155
branch
from
6c4bfeb to
1d5208c
Compare
Author
|
Sure, done. Also gave a bit of context in the commit message with reference to both my Core PR which contains an extensive description of the issue and to this PR for why it's being simply deleted in inquisition. |
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.
darosior
force-pushed
the
202509_inquisition_32155
branch
from
1d5208c to
a26b657
Compare
Author
|
Rebased on |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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 backports bitcoin#32155, merged and about to be released in Bitcoin Core 30, to Bitcoin Inquisition 29. This is a preparatory change for a Consensus Cleanup PR to inquisition.
This also backports bitcoin#31907, which the backported PR depends on.
It also cherry-picks the commit from bitcoin#32117, because the hashes being different here triggered the brittleness described there. Instead of changing magic values until the test pass, make the test more robust beforehand.(EDIT: instead of cherry-picking the offending line was simply removed for now.)