miner: timelock the coinbase to the mined block's height#32155
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32155. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. LLM Linter (✨ experimental)Possible typos and grammar issues: Bad snapshot format or truncated snapshot after deserializing 1 coins. → …deserializing 1 coin. |
5e7a418 to
a122b4a
Compare
|
Tested with Stratum v2 (Sjors#68 + first commit here, SRI pool role connected to their custom signet, sv2 -> sv1 translator and a BitAxe). Indeed I'm seeing blocks at height N with There's a question about sequencing here, since it's a preparatory step towards the Great Consensus Cleanup proposal bitcoin/bips#1800. In a way it's similiar to #30681 and other PRs that prepared our mining code for the timewarp fix from that same BIP. But it's also different. Taking account of the timewarp fix only potentially impacts the timestamp of the first block in a retarget period, i.e. push it a bit in the future. This only happens under circumstances that are common on testnet4 but rare on mainnet. Whereas this PR impacts every mined block. More specifically, for Stratum v1 this PR changes nothing, because The fact that Stratum v1 support requires downstream changes is a good reason to get started early. Moreover, because deploying this soft fork is more involved than simply updating Bitcoin Core, it's potentially less safe. By changing the default mining behavior long before the fork activates (or even signals) the actual deployment would be a lot safer. However the BIP is brand new, so we shouldn't make these (specific) mining changes a fait accompli. Although nothing would break if we later abandon this approach in favor of another BIP30 fix, it still seems better to wait before doing this on mainnet until the BIP has been out there for a while. Arguably, waiting for the v30 release is long enough and we can simply revert this PR if needed. Another approach could be to make this testnet only, and then decide if we want to apply it to mainnet a bit closer to the release. I do think we should have this in v30 to at least enable downstream testing, along with any changes to |
polespinasa
left a comment
There was a problem hiding this comment.
Concept ack
Did a simple test with regtest and the blocks are correctly mined with a coinbase tx setting nLockTime=heigh-1 and nSequence=4294967294.
I agree adding this before the soft-fork is good so we give time to miners.
I have one question although, if I'm not wrong the node miner code is no longer used in mainnet so this PR only affects to test networks?
Unfortunately regtest is not representative here. It's the only network for which Bitcoin Core will mine by itself. For all other networks, including testnet (3 and 4), you need to use external software. |
This is what I though. So how does this change "help" or "put pressure" to add this new rule to miners software? Wouldn't help more implement the code that will check if a block is following it? Obviously code that would never be ran without another change to add an activation date. |
That's what activating the soft-fork does. But there's a chicken-egg problem, because one reason to delay activating a soft fork is because part of the ecosystem isn't ready.
As I explained in my first comment, for Stratum v2 this just works(tm). For Stratum v1 additional changes are needed in other software. Not sure what the best approach is there. For them the change here is a bit like the proverbial tree in the forest. |
Sorry maybe I didn't explain myself. I meant to introduce the code, but without activating it and without activation date. So if this consensus change doesn't go forward, we make sure that we will not have hard fork because of nodes running a version with the check code implemented. Just put it there to say: "Here it is this new condition, at some point this is going to have an activation date, start implementing it". Maybe that's not the way to do it, just dropping ideas. As @darosior said:
So I think that this is a more "aggressive" (or convincing 😉) way to convince them. |
a122b4a to
c872728
Compare
|
Rebased. |
There was a problem hiding this comment.
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 is still confusing to me. Shouldn't this just continue working with the decrementing counter for the sequence number? Or is that entirely unrelated?
There was a problem hiding this comment.
It's only related to the coinbase nSequence/nLockTime change insofar as those change the txid of the coinbase transactions and therefore of all transactions in the test (and all other tests, but this test is one of those tests that implicitly relied on specific txid values).
This test is exercising the error returned by CheckConflictTopology, which iterates through the set of conflicts:
Line 1277 in d91a746
And returns the first error encountered:
Lines 1288 to 1309 in d91a746
The (ordered) set of conflicts is lexicographically sorted by txid:
Line 396 in d91a746
Therefore, changing nothing but the txid of the transactions involved in the test may cause the order of conflicts checked by CheckConflictTopology to change, and another error to be encountered first and therefore the test to fail since it asserts the specific error message.
For example, the check you are commenting on sets up the following topology: two distinct transaction A and A' are spent by a transaction B. C, which conflicts with A, is broadcast. Here CheckTopology could either bail on C having two parents ("C has 2 ancestors, max 1 allowed") or on A having a descendant which has other parents ("A is not the only parent of child C"). Which error message is returned depends on which is encountered first, which depends on the value of the txid of the transactions involved. 🤷♂️
There was a problem hiding this comment.
Leaving unresolved as it may be useful to other reviewers.
Sjors
left a comment
There was a problem hiding this comment.
The churn in the first commit is pretty annoying.
Maybe with a MakeCoinbase helper the change can split across more than one commit? E.g. the first commit could set with_lock_time to false for ChainType::REGTEST and height < 300 so the assume valid blocks don't change.
c872728 to
21f450a
Compare
21f450a to
ce5f4ea
Compare
|
Do people think i should change the value of the coinbase transaction used in the fuzz test here as well? I previously kept it along with the PR introducing validation for those fields, but since i change this in unit tests already it might make sense to do it here. |
ce5f4ea to
895beb3
Compare
|
I have added a commit to also set nLocktime / nSequence for coinbase transactions created in fuzz targets.
@Sjors (from this and in-person discussions) i have split as much as possible of the changes from this commit into preparatory commits. Now there is in the main commit only the necessary churn to keep commits hygienic. Hopefully it helps. |
sedited
left a comment
There was a problem hiding this comment.
ACK 895beb30b375c04804e9596ed798ab56730dcae1
ajtowns
left a comment
There was a problem hiding this comment.
Approach ACK 895beb30b375c04804e9596ed798ab56730dcae1
There was a problem hiding this comment.
Maybe add something like
genblock = create_block(hashprev=int(block["hash"], 16), tmpl=dict(height=block["height"]+1), ntime=block["time"]+1)
genblock.vtx[0].vin[0].nSequence = SEQUENCE_FINAL
genblock.vtx[0].rehash()
genblock.hashMerkleRoot = genblock.calc_merkle_root()
genblock.solve()
self.nodes[0].submitblock(genblock.serialize().hex())
assert_equal(self.nodes[0].getbestblockhash(), genblock.hash)to ensure there's explicit test coverage for blocks with a coinbase whose nSequence is final?
I believe with this PR there remains implicit test coverage via feature_taproot (gen_test_vectors uses SEQUENCE_FINAL) and via rpc_getblockstats (via the test data in data/rpc_getblockstats.json).
There was a problem hiding this comment.
It feels out of place in mining_basic, and the PR that introduces validation checks for nSequence/nLockTime in coinbase does check various combinations in feature_block, including this one. So i think it's better left there?
|
Code review ACK a58cb3b I'm a bit reluctant to have this merged while BIP54 is in limbo (bitcoin/bips#1800). Additionally I think it's worth waiting a few weeks (?) to see if someone responds to https://delvingbitcoin.org/t/great-consensus-cleanup-revival/710/85 about the use of Let's add this to the v30 milestone though. Unless by that time someone advocates a different approach, it's better to just pick one approach and encourage all miners to do it. This is the only part of the proposal where normal BIP9 style signalling is of no use, because miners will certainly produce an invalid block if they don't modify their pool software. And that might take ages. |
BIP54 draft merged. |
|
In 8f2078a "miner: timelock coinbase transactions" I'm having a hard time understanding why Edit: Oh, is it because of set ordering by txid or something like that, and the error message depends on which tx is checked first? If so, it would be nice to have that in the commit message. |
|
ACK a58cb3b |
Yes, see #32155 (comment) above. |
The Consensus Cleanup soft fork proposal includes enforcing that coinbase transactions set their
nLockTime 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
coinbasetxnfield, and miningpool 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 commit making the change also updates a bunch of seemingly-unrelated tests. This is because those tests were asserting error messages based on the txid of transactions involved, and changing the coinbase transaction structure necessarily changes the txid of all tests' transactions.