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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32117 (qa: make feature_assumeutxo.py test more robust by darosior)
#29500 (test: create assert_not_equal util by kevkevinpal)

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.

DrahtBot added the label Mining on Mar 27, 2025

miner: set the coinbase nLockTime to block height-1

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.

2f55cec200

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

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?

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.

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.

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

Code Coverage & Benchmarks

Reviews

Conflicts