Consensus Cleanup preparation: backport "miner: timelock the coinbase to the mined block's height"#90
Conversation
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
|
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. |
darosior
force-pushed
the
202509_inquisition_32155
branch
from
1d5208c to
a26b657
Compare
|
Rebased on |
|
Can i get CI to run here please? |
Which offending line? I started with the (inquisition) 29.x branch, cherry-picked the three commits from bitcoin#31907. At that point |
|
|
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.
darosior
force-pushed
the
202509_inquisition_32155
branch
from
a26b657 to
9ac878a
Compare
|
@Sjors here are instructions for the rebase:
|
|
Updated the PR to also backport bitcoin#31910, and add another 1-line preparation commit. Going through the process of documenting the backport surfaced that i took some shortcuts in trying to limit the amount of backported changes. I think this unnecessarily made review harder, so i updated the approach. Hopefully the new version is clearer. See #90 (comment) for details. |
|
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. |
|
Seems like a straightforward backport now, which is nice. Compared to cherry picking by hand, I see some hash differences in the functional tests, an ACK 9ac878a |
|
Look tedious, but the LLM was able to follow your instructions and get a matching end result :-) Post-merge ACK |
5 participants
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 and bitcoin#31910, which the backported PR depends on.