miner: timelock the coinbase to the mined block's height

darosior commented at 7:30 pm on March 27, 2025: member

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 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 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.

DrahtBot commented at 7:30 pm on March 27, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32155.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors
Approach ACK	ajtowns
Stale ACK	TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

Bad snapshot format or truncated snapshot after deserializing 1 coins. → …deserializing 1 coin.

DrahtBot added the label Mining on Mar 27, 2025

darosior force-pushed on Mar 27, 2025

in src/node/miner.cpp:166 in 2f55cec200 outdated

157@@ -158,10 +158,13 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
158     CMutableTransaction coinbaseTx;
159     coinbaseTx.vin.resize(1);
160     coinbaseTx.vin[0].prevout.SetNull();
161+    coinbaseTx.vin[0].nSequence = CTxIn::MAX_SEQUENCE_NONFINAL;
162     coinbaseTx.vout.resize(1);
163     coinbaseTx.vout[0].scriptPubKey = m_options.coinbase_output_script;
164     coinbaseTx.vout[0].nValue = nFees + GetBlockSubsidy(nHeight, chainparams.GetConsensus());
165     coinbaseTx.vin[0].scriptSig = CScript() << nHeight << OP_0;
166+    Assert(nHeight > 0);

polespinasa commented at 8:53 am on March 28, 2025:

Is this Assert necessary? Is it even possible to reach this code with nHeight<=0? Genesis block is hardcoded, so this value at list will be 1, right?

darosior commented at 6:01 pm on April 8, 2025:

This is documenting that it isn’t possible.

Sjors commented at 9:01 am on March 28, 2025: member

Tested with Stratum v2 (https://github.com/Sjors/bitcoin/pull/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 locktime values of N-1 and sequence of 4294967294 (0xFFFFFFFE). I explained below why this just works(tm), unlike with Stratum v1.

There’s a question about sequencing here, since it’s a preparatory step towards the Great Consensus Cleanup proposal https://github.com/bitcoin/bips/pull/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 getblocktemplate does not echo coinbase nLockTime. It will require expanding BIP22 / BIP23 and further changes downstream. But the Stratum v2 Template Provider (see #31098) does pass on this information, so any Stratum v2 pool out there will start doing this once it’s in a release.

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 getblocktemplate. And some people may prefer to test on mainnet. So yet another approach could be to put it behind a flag (default true for test networks).

polespinasa commented at 11:29 am on March 28, 2025: contributor

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?

Sjors commented at 11:58 am on March 28, 2025: member

Did a simple test with regtest

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.

polespinasa commented at 12:14 pm on March 28, 2025: contributor

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.

Sjors commented at 1:00 pm on March 28, 2025: member

Wouldn’t help more implement the code that will check if a block is following it?

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.

So how does this change “help” or “put pressure” to add this new rule to miners software?

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.

in src/node/miner.cpp:161 in 2f55cec200 outdated

157@@ -158,10 +158,13 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
158     CMutableTransaction coinbaseTx;
159     coinbaseTx.vin.resize(1);
160     coinbaseTx.vin[0].prevout.SetNull();
161+    coinbaseTx.vin[0].nSequence = CTxIn::MAX_SEQUENCE_NONFINAL;

Sjors commented at 4:16 pm on March 28, 2025:

Note to reviewers: IsFinal() has been present since the first commit as a way to ignore the sequence number, hence the need to avoid SEQUENCE_FINAL.

The proposed consensus rule doesn’t require MAX_SEQUENCE_NONFINAL, anything other than SEQUENCE_FINAL is allowed. Maybe that’s good to say in a comment.

Sjors commented at 8:19 pm on April 9, 2025:

2c10f93d0bd920f20e91f1be7c82e66af2a52919 nit: can you move this down next to nLockTime? This line is what makes the nLockTime bit work, so makes sense to have them together. Also, in https://github.com/Sjors/bitcoin/pull/49 I put this behavior behind a startup flag.

darosior commented at 6:06 pm on April 21, 2025:

I added the comment as you requested above (https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2018964740), hopefully this makes it clearer without having to move it.

I don’t think there should be a startup flag since i don’t see a reason for disabling it: this should not affect miners in any way.

darosior commented at 6:08 pm on April 21, 2025:

Added a comment.

Sjors commented at 8:11 am on April 22, 2025:

It affects any miner that uses the Mining interface, because createNewBlock returns a BlockTemplate which includes a getBlock() method which returns a CBlock that contains the coinbase transaction.

darosior commented at 12:49 pm on April 22, 2025:

Of course. My point is that it does not change anything for a miner if its coinbase transaction has nLockTime 0 or height-1.

in src/node/miner.cpp:167 in 2f55cec200 outdated

162     coinbaseTx.vout.resize(1);
163     coinbaseTx.vout[0].scriptPubKey = m_options.coinbase_output_script;
164     coinbaseTx.vout[0].nValue = nFees + GetBlockSubsidy(nHeight, chainparams.GetConsensus());
165     coinbaseTx.vin[0].scriptSig = CScript() << nHeight << OP_0;
166+    Assert(nHeight > 0);
167+    coinbaseTx.nLockTime = static_cast<uint32_t>(nHeight - 1);

Sjors commented at 4:16 pm on March 28, 2025:

As a sanity check, I changed this to just nHeight, which immediately triggers:

0TestBlockValidity: Consensus::ContextualCheckBlock: bad-txns-nonfinal, non-final transaction

(either when calling getblocktemplate or in sv2 as soon as the Template Provider tries to build a template)

Dropping the nSequence line makes that error go away.

In other words, this guarantees that the coinbase transaction can’t have been present in an older block. But as the BIP points out, nLockTime wasn’t enforced until block 36,000 or so. https://bitcoin.stackexchange.com/questions/90229/nlocktime-in-bitcoin-core

We can then manually verify that all these old blocks have nLockTime set to 0:

0bitcoin % for i in {1..36001}
1do
2bitcoin-cli getblock `bitcoin-cli getblockhash $i` 2 | jq '.tx[0].locktime'
3done | grep -v 0

Note that any alien reorg shenanigans (now that checkpoints are gone) can’t cheat with nLockTime because the rule is now enforced from genesis.

arejula27 commented at 9:50 pm on March 28, 2025:

I was discussing with @hodlinator and @Prabhat1308 that a Coinbase UTXO cannot be spent until 100 blocks have been added after it. Wouldn’t it be better to set this to static_cast<uint32_t>(nHeight +99 );? This could simplify the logic for the Coinbase spend condition and potentially make mempool and transaction validation more efficient

polespinasa commented at 4:26 pm on March 29, 2025:

Wouldn’t it be better to set this to static_cast<uint32_t>(nHeight +99 );?

That would make the transaction invalid and unable to be included in a block until the nHeight + 99 block. It isn’t a condition on when it can be spent, but when it is valid (therefore it can be mined).

arejula27 commented at 4:39 pm on March 29, 2025:

That’s true, I confused it with CLTV.

polespinasa commented at 5:56 pm on March 28, 2025: contributor

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

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:

updating the Bitcoin Core mining code is a first step toward convincing pools to update their (often closed source) code

So I think that this is a more “aggressive” (or convincing 😉) way to convince them.

DrahtBot added the label Needs rebase on Apr 1, 2025

darosior force-pushed on Apr 8, 2025

DrahtBot removed the label Needs rebase on Apr 8, 2025

darosior commented at 8:54 pm on April 8, 2025: member

Rebased.

in src/test/blockfilter_index_tests.cpp:86 in 2c10f93d0b outdated

80@@ -81,7 +81,9 @@ CBlock BuildChainTestingSetup::CreateBlock(const CBlockIndex* prev,
81     }
82     {
83         CMutableTransaction tx_coinbase{*block.vtx.at(0)};
84+        tx_coinbase.nLockTime = static_cast<uint32_t>(prev->nHeight);
85         tx_coinbase.vin.at(0).scriptSig = CScript{} << prev->nHeight + 1;
86+        tx_coinbase.vin.at(0).nSequence = CTxIn::MAX_SEQUENCE_NONFINAL;

TheCharlatan commented at 9:34 am on April 9, 2025:

What do you think about adding a dedicated MakeCoinbase function to encapsulate all of this a bit better and share its behaviour between the tests and production code? There is already a similar function in the functional test framework after all.

Sjors commented at 8:26 pm on April 9, 2025:

Maybe MakeCoinbase(bool with_lock_time = true).

darosior commented at 5:25 pm on April 21, 2025:

So i looked into this and i don’t think there is a clean way of doing it that would help here. What this function is doing is really a hack, creating a block assembler for a specific tip and mining a block with prev that does not correspond to it. However one thing is the nSequence doesn’t change from one block to another. I added here for clarity but i now think it’s unnecessarily confusing, so i removed it.

TheCharlatan commented at 9:08 pm on April 21, 2025:

Ah, I see. I also tried to come up with something, but did not land on anything satisfying either. Was briefly thinking of adding an optional field in the BlockCreateOptions, but that would defeat the point.

in test/functional/mining_mainnet.py:68 in 2c10f93d0b outdated

63@@ -63,6 +64,9 @@ def mine(self, height, prev_hash, blocks, node, fees=0):
64         block.nBits = DIFF_1_N_BITS
65         block.nNonce = blocks['nonces'][height - 1]
66         block.vtx = [create_coinbase(height=height, script_pubkey=bytes.fromhex(COINBASE_SCRIPT_PUBKEY), retarget_period=2016)]
67+        block.vtx[0].nLockTime = 0
68+        block.vtx[0].vin[0].nSequence = SEQUENCE_FINAL
69+        block.vtx[0].rehash()

TheCharlatan commented at 10:06 am on April 9, 2025:

Just a comment, but I also don’t think it would be worth it to re-generate this alternative mainnet chain.

Sjors commented at 8:24 pm on April 9, 2025:

Would be good to add a comment here to explain.

We can always regenerate it later.

darosior commented at 6:08 pm on April 21, 2025:

Added a comment.

in test/functional/mempool_package_rbf.py:387 in 2c10f93d0b outdated

383@@ -384,7 +384,7 @@ def test_wrong_conflict_cluster_size_parents_child(self):
384         # Now make conflicting packages for each coin
385         package_hex1, _package_txns1 = self.create_simple_package(coin1, DEFAULT_FEE, DEFAULT_CHILD_FEE)
386         package_result = node.submitpackage(package_hex1)
387-        assert_equal(f"package RBF failed: {child_result['tx'].rehash()} has 2 ancestors, max 1 allowed", package_result["package_msg"])
388+        assert_equal(f"package RBF failed: {parent1_result['tx'].rehash()} is not the only parent of child {child_result['tx'].rehash()}", package_result["package_msg"])

TheCharlatan commented at 10:23 am on April 9, 2025:

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?

darosior commented at 5:59 pm on April 21, 2025:

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: https://github.com/bitcoin/bitcoin/blob/d91a746815e4428c738f1a096a950292cbdb5469/src/txmempool.cpp#L1277

And returns the first error encountered: https://github.com/bitcoin/bitcoin/blob/d91a746815e4428c738f1a096a950292cbdb5469/src/txmempool.cpp#L1288-L1309

The (ordered) set of conflicts is lexicographically sorted by txid: https://github.com/bitcoin/bitcoin/blob/d91a746815e4428c738f1a096a950292cbdb5469/src/txmempool.h#L396

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. :man_shrugging:

darosior commented at 6:08 pm on April 21, 2025:

Leaving unresolved as it may be useful to other reviewers.

in test/functional/mining_basic.py:366 in c872728e52 outdated

362@@ -362,6 +363,12 @@ def test_block_max_weight(self):
363             expected_msg=f"Error: Specified -blockmaxweight ({MAX_BLOCK_WEIGHT + 1}) exceeds consensus maximum block weight ({MAX_BLOCK_WEIGHT})",
364         )
365 
366+    def test_height_in_locktime(self):

Sjors commented at 8:03 pm on April 9, 2025:

c872728e5260890584f4db6742aefcd98be92643: nothing is calling this function

darosior commented at 9:21 pm on April 9, 2025:

Looks like this slipped through at some point. Fixed now, thanks.

Sjors commented at 8:34 pm on April 9, 2025: member

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.

darosior force-pushed on Apr 9, 2025

TheCharlatan commented at 10:03 am on April 19, 2025: contributor

Concept ACK

darosior force-pushed on Apr 21, 2025

darosior commented at 6:23 pm on April 21, 2025: member

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.

darosior force-pushed on Apr 21, 2025

darosior commented at 7:58 pm on April 21, 2025: member

I have added a commit to also set nLocktime / nSequence for coinbase transactions created in fuzz targets.

The churn in the first commit is pretty annoying. @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.

TheCharlatan approved

TheCharlatan commented at 9:16 pm on April 21, 2025: contributor

ACK 895beb30b375c04804e9596ed798ab56730dcae1

in contrib/signet/miner:105 in 895beb30b3 outdated

101@@ -102,7 +102,8 @@ def generate_psbt(tmpl, reward_spk, *, blocktime=None, poolid=None):
102         scriptSig = CScript(b"" + scriptSig + CScriptOp.encode_op_pushdata(poolid))
103 
104     cbtx = CTransaction()
105-    cbtx.vin = [CTxIn(COutPoint(0, 0xffffffff), scriptSig, 0xffffffff)]
106+    cbtx.nLockTime = tmpl["height"] - 1

ajtowns commented at 2:18 am on April 22, 2025:

Should these be (height - 1) % LOCKTIME_THRESHOLD ? Or are we just assuming a hard fork to a new transaction format in the next 9500 years?

darosior commented at 1:00 pm on April 22, 2025:

These are equivalent. In both cases the coinbase transaction will still be valid, just not timelocked to the block. I think it’s marginally better to not make it modulo LOCKTIME_THRESHOLD for simplicity and to avoid rewinding back to values already used. But really, i think anything past year 2106 shouldn’t matter. :)

in src/test/util/mining.cpp:47 in 895beb30b3 outdated

43 
44         CMutableTransaction coinbase_tx;
45+        coinbase_tx.nLockTime = prev_height;
46         coinbase_tx.vin.resize(1);
47         coinbase_tx.vin[0].prevout.SetNull();
48+        coinbase_tx.vin[0].nSequence = CTxIn::MAX_SEQUENCE_NONFINAL; // Make sure timelock is enforced.

ajtowns commented at 2:29 am on April 22, 2025:

Would it be more elegant to leave the genesis block’s nSequence as MAX_SEQUENCE to avoid the implication that the genesis block might be timelocked? ie,

0coinbase_tx.vin.resize(1);
1coinbase_tx.vin[0].prevout.SetNull();
2if (height > 0) {
3    coinbase_tx.nLockTime = static_cast<uint32_t>(height - 1);
4    coinbase_tx.vin[0].nSeqeunece = CTxIn::MAX_SEQUENCE_NONFINAL;
5}

darosior commented at 1:31 pm on April 22, 2025:

Sure, done.

in test/functional/mining_basic.py:34 in 895beb30b3 outdated

29@@ -30,7 +30,8 @@
30     MAX_BLOCK_WEIGHT,
31     MINIMUM_BLOCK_RESERVED_WEIGHT,
32     ser_uint256,
33-    WITNESS_SCALE_FACTOR
34+    WITNESS_SCALE_FACTOR,
35+    MAX_SEQUENCE_NONFINAL,

ajtowns commented at 4:12 am on April 22, 2025:

Should be sorted

darosior commented at 1:31 pm on April 22, 2025:

Done.

in test/functional/mining_basic.py:371 in 895beb30b3 outdated

362@@ -362,6 +363,12 @@ def test_block_max_weight(self):
363             expected_msg=f"Error: Specified -blockmaxweight ({MAX_BLOCK_WEIGHT + 1}) exceeds consensus maximum block weight ({MAX_BLOCK_WEIGHT})",
364         )
365 
366+    def test_height_in_locktime(self):
367+        self.log.info("Sanity check generated blocks have their coinbase timelocked to their height.")
368+        self.generate(self.nodes[0], 1, sync_fun=self.no_op)
369+        block = self.nodes[0].getblock(self.nodes[0].getbestblockhash(), 2)
370+        assert_equal(block["tx"][0]["locktime"], block["height"] - 1)
371+        assert_equal(block["tx"][0]["vin"][0]["sequence"], MAX_SEQUENCE_NONFINAL)

ajtowns commented at 4:13 am on April 22, 2025:

Maybe add something like

0        genblock = create_block(hashprev=int(block["hash"], 16), tmpl=dict(height=block["height"]+1), ntime=block["time"]+1)
1        genblock.vtx[0].vin[0].nSequence = SEQUENCE_FINAL
2        genblock.vtx[0].rehash()
3        genblock.hashMerkleRoot = genblock.calc_merkle_root()
4        genblock.solve()
5        self.nodes[0].submitblock(genblock.serialize().hex())
6        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).

darosior commented at 1:24 pm on April 22, 2025:

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?

ajtowns commented at 4:32 am on April 22, 2025: contributor

Approach ACK 895beb30b375c04804e9596ed798ab56730dcae1

darosior force-pushed on Apr 22, 2025

in test/functional/test_framework/messages.py:46 in 7f368ea62c outdated

42@@ -43,6 +43,7 @@
43 
44 MAX_BIP125_RBF_SEQUENCE = 0xfffffffd  # Sequence number that is rbf-opt-in (BIP 125) and csv-opt-out (BIP 68)
45 SEQUENCE_FINAL = 0xffffffff  # Sequence number that disables nLockTime if set for every input of a tx
46+MAX_SEQUENCE_NONFINAL = 0xfffffffe  # Highest sequence number that does not disable nLockTime

maflcko commented at 6:46 pm on April 22, 2025:

nit in 7f368ea62cd89e6ff4238ec49efa2eaddd0e5046: Needs rebase, as it is already included in master. I think the name already implies that this is the “highest sequence number with the given property (non-final)”, so the comment seems redundant. The comment seems better to mention how it relates to BIPs. For reference, the existing comment is: Sequence number that is csv-opt-out (BIP 68). See: https://github.com/bitcoin/bitcoin/commit/06439a14c884d7f81f331646ad361e88b3037a51#diff-1698fabd4ddd298eb28e234d97f34e744f568852b77361384c8edb38819de5e3R45

TheCharlatan commented at 7:27 pm on April 22, 2025:

The comment may be confusing in the context of just using nlocktime, so maybe it should be amended to describe its non-BIP68 meaning too?

maflcko commented at 7:41 pm on April 22, 2025:

Yeah, the comments here are one-line summaries of the “real” comments in ./src/primitives/transaction.h only. A third alternative may be to just remove them and refer to the docs in ./src/primitives/transaction.h to avoid having to copy paste the full docs into both places every time they are touched.

darosior commented at 6:03 pm on April 23, 2025:

Rebased on master.

in src/test/util/mining.cpp:50 in e78e92f636 outdated

45@@ -44,6 +46,10 @@ std::vector<std::shared_ptr<CBlock>> CreateBlockChain(size_t total_height, const
46         coinbase_tx.vout[0].scriptPubKey = P2WSH_OP_TRUE;
47         coinbase_tx.vout[0].nValue = GetBlockSubsidy(height + 1, params.GetConsensus());
48         coinbase_tx.vin[0].scriptSig = CScript() << (height + 1) << OP_0;
49+        if (height > 0) {
50+            coinbase_tx.nLockTime = static_cast<uint32_t>(height - 1);

maflcko commented at 7:32 pm on April 22, 2025:

e78e92f6363c4ea1cbedd6631fe56c0cd8e164d5: I don’t think this is right. Why the -1?

For reference, I debugged this via:

 0diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp
 1index 13f6088192..927fcb483c 100644
 2--- a/src/test/fuzz/utxo_snapshot.cpp
 3+++ b/src/test/fuzz/utxo_snapshot.cpp
 4@@ -92,6 +92,7 @@ void initialize_chain()
 5             Assert(processed);
 6             const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))};
 7             Assert(index);
 8+            std::cout << index->nHeight << ": " << block->vtx[0]->nLockTime << std::endl;
 9         }
10     }
11     g_setup = setup.get();

and the output was:

darosior commented at 1:11 pm on April 23, 2025:

Good catch, thanks. I should have re-checked with my follow-up PR which introduces validation before pushing the change for the fuzz target. Will do that this time.

darosior commented at 6:02 pm on April 23, 2025:

Done. Removed the height > 0 condition, corrected height - 1 for height and added a comment that the height variable does not actually correspond to the block height as it might be confusing to readers. Verified correctness by rebasing my Consensus Cleanup PR on top of this one.

darosior force-pushed on Apr 23, 2025

DrahtBot added the label CI failed on Apr 23, 2025

DrahtBot commented at 3:20 pm on April 23, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/41019009412

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

darosior force-pushed on Apr 23, 2025

DrahtBot removed the label CI failed on Apr 23, 2025

Sjors commented at 7:46 am on April 24, 2025: member

Failure in interface_usdt_coinselection seems to be an instance of #32322.

Thanks for splitting out some of the stuff in to new commits.

I think you lost the comment announced here: #32155 (review)

Other than it looks good.

TheCharlatan approved

TheCharlatan commented at 7:46 am on April 24, 2025: contributor

Re-ACK 3904fcd6680dce7a9a0f5f91c5a3069b9fc81782

DrahtBot requested review from ajtowns on Apr 24, 2025

qa: timelock coinbase transactions created in functional tests a5f52cfcc4

contrib: timelock coinbase transactions in signet miner 9c94069d8b

qa: timelock coinbase transactions created in fuzz targets c76dbe9b8b

qa: use prev height as nLockTime for coinbase txs created in unit tests

We don't set the nSequence as it will be set directly in the block template generator in a following
commit.

788aeebf34

miner: timelock coinbase transactions

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.

8f2078af6a

qa: sanity check mined block have their coinbase timelocked to height a58cb3b1c1

darosior force-pushed on Apr 25, 2025

darosior commented at 4:47 pm on April 25, 2025: member

I think you lost the comment announced here: #32155 (comment)

Other than it looks good.

I did not: https://github.com/bitcoin/bitcoin/pull/32155/commits/67267b147f8c48e0775602395fba9db0105dccf4#diff-6b810fb346def9cc80a9d9ab75184dd9f41c4ac63493135a7bdfcb98f75b1ce8R39

Failure in interface_usdt_coinselection seems to be an instance of #32322.

Yes. Rebased on master to avoid the unrelated CI failure after it was fixed in #32336.

Sjors commented at 6:12 pm on April 25, 2025: member

Code review ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d

I’m a bit reluctant to have this merged while BIP54 is in limbo (https://github.com/bitcoin/bips/pull/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 nLockTime as an extranonce.

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.

DrahtBot requested review from TheCharlatan on Apr 25, 2025

DrahtBot added the label CI failed on Apr 29, 2025

DrahtBot removed the label CI failed on Apr 29, 2025

miner: timelock the coinbase to the mined block’s height #32155

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)