BIP54 "Consensus Cleanup" implementation

[darosior]

This implements the BIP54 consensus rules.

Each of the 4 mitigations comes with its own unit test in a new src/test/bip54_tests.cpp module. The unit tests can be used to generate JSON test vectors for the BIP (opt-in). For those unit tests that require mainnet blocks, the JSON test vectors were generated separately and included in this PR as data files.

Besides the unit test for each mitigation, this contains a fuzz harness for the sigop accounting logic as well as a functional test which goes over all mitigations in pseudo realistic situations (for instance, mimic a Timewarp attack and a Murch-Zawy attack to demonstrate the new timestamp restrictions would prevent that). The fuzz target for the sigop accounting logic was designed to make it possible to seed it from the BIP test vectors. This branch implements that.

The chains of headers for the timestamp restrictions test vectors were generated using this unit test (note the premined headers to allow modifications without re-generating all headers from scratch). The small chains of blocks for the coinbase restrictions test vectors were generated using this unit test.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #94 (Allow configuring target block time for a signet by benthecarman)

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.

[ariard]

Just going over the introduced consensus code fix by fix for now.

[darosior]

Thanks for kicking CI @ajtowns. I fixed the missing annotations that DEBUG_LOCKORDER complained about.

[instagibbs]

Focused primarily on BIP compliance, everything looks good to me

[ariard]

Did a first review on the 64-byte fix, answering opened comments.

[Sjors]

The consensus changes, with respect to their specification in BIP54, look good to me:

• 84e1043 validation: make BIP54 sigops check consensus-critical
• 87e9e65 consensus: prevent timewarp attacks with a 2h grace period
• c6c1b18 consensus: prevent negative difficulty adjustment intervals
• 00745a2 consensus: mandate coinbase transactions be timelocked to block height
• 8ee48e4 consensus: make 64 bytes transactions invalid

I think this PR is well organised (starting at ---). Each consensus change is first prepared, done in a minimal commit, and then followed by new tests.

I'll study the test changes in more detail later.

[ariard]

See second comment on nSequence == SEQUENCE_FINAL`.

[ariard]

Finished a first round of review of the code for the 4 consensus fixes:

• “difficulty adjustment exploits”
• “long block validation time”
• “merkle tree malleability with 64-byte transaction”
• “possibility of duplicate coinbase transactions”

Will review more the fixes at the conceptual level and the breadth / depth of their respective test coverage.

[Sjors]

See inline suggestion for renaming the bip_active argument to check_bip54 and making it a bit more clear in the tests if they're checking standardness (pre activation) or consensus (post activation).

The other consensus commit changes for the 19d159e push (compared to my previous review) look fine.

[Sjors]

In d1d435f validation: make BIP54 sigops check consensus-critical: note to other reviewers: this doesn't prematurely drop the standardness check before activation.

[ajtowns]

This does, however, make the BIP54 checks mandatory for the mempool, such that they cannot be disabled by acceptnonstdtxn, aiui. (Which is good: you don't want consensus-invalid txs in your mempool at the point the consensus change activates)

[darosior]

That said, the cache doesn't seem necessary, just avoiding serializing the entire block as json seems to improve things ~60x for me

Would simply adding the block's coinbase's locktime to the output of verbosity-1 getblock be sufficient then?

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index bbe401fcc6c..78062c74e6f 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -183,6 +183,7 @@ UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIn
     result.pushKV("strippedsize", (int)::GetSerializeSize(TX_NO_WITNESS(block)));
     result.pushKV("size", (int)::GetSerializeSize(TX_WITH_WITNESS(block)));
     result.pushKV("weight", (int)::GetBlockWeight(block));
+    result.pushKV("cblocktime", block.vtx.at(0)->nLockTime);
     UniValue txs(UniValue::VARR);
 
     switch (verbosity) {
@@ -724,6 +725,7 @@ static RPCHelpMan getblock()
                     {RPCResult::Type::NUM, "version", "The block version"},
                     {RPCResult::Type::STR_HEX, "versionHex", "The block version formatted in hexadecimal"},
                     {RPCResult::Type::STR_HEX, "merkleroot", "The merkle root"},
+                    {RPCResult::Type::NUM, "locktime", "The coinbase transaction's locktime value"},
                     {RPCResult::Type::ARR, "tx", "The transaction ids",
                         {{RPCResult::Type::STR_HEX, "", "The transaction id"}}},
                     {RPCResult::Type::NUM_TIME, "time",       "The block time expressed in " + UNIX_EPOCH_TIME},

I've tested this on mainnet with your snippet from earlier and it's reasonably performant.

[ariard]

On the coinbase transaction and requesting the nLocktime to be set to the current block height minus one and the nSequence field must not be equal to 0xff_ff_ff_ff, I'm still thinking the second check is unecessary. The first check is ensuring that the transaction is always final (2nd check of IsFinal()). I’ve yet to verify with a test, but post-BIP54 the 3rd check in IsFinal() should never be reach by validating a coinbase transaction on its own.

#99 (comment)

https://groups.google.com/g/bitcoindev/c/6TTlDwP2OQg

Per se, unless I'm missing something, I don't think the value of the nSequence field needs to be set to a precise value and the whole field could be even “free" up field either for (a) for an extra nonce or (b) as a space where to fit a new commitment structure in the future (and that's particularly interesting as it would work even if there are no segwit txn in the block cf. BIP141). cc @darosior

[ariard]

This is not consensus, but more relay. At CMPCTBLOCK reception, there must be at least one transaction.

    if (cmpctblock.header.IsNull() || (cmpctblock.shorttxids.empty() && cmpctblock.prefilledtxn.empty()))
        return READ_STATUS_INVALID;

Sanitizing that the received coinbase is respecting the new rules of consensus avoids to store cmpct block state for a block that will be overall consenus invalid (note the validation of the coinbase is already lax afaict).

[darosior]

That would be a tiny optimisation in the event that someone decided to waste an enormous amount of energy creating an invalid block. I don't think it's worth it.

[ariard]

The number of compact block relay peers != block relay peers iirc, so I think there still some margin of shitting the coinbase tx and playing with it, but effectively I don’t think it’s something per se new coming with BIP54, and it’s rather ante date it.

[ajtowns]

Would simply adding the block's coinbase's locktime to the output of verbosity-1 getblock be sufficient then?

Yeah, seems like -- it takes about 3 times as long as the dedicated RPC for me in a quick test, but that's good enough for how much simpler it is. I found "cblocktime" confusing (CBlock time?), maybe tx0_locktime? Also should report whether nseq is final or not. Could also just report "bip54_coinbase_readiness": true/false.

I messed around with the dedicated approach some more; it looks like if you add a dedicated ReadBlockCoinbaseOnly method to block storage, which just reads the header and coinbase tx, but not the rest of the block, then the dedicated rpc gets a massive speedup. My figures are:

• just using the RPCs we have now: 2 retarget periods takes about 2 hours to scan
• a couple of lines of code to add reporting it to getblock 1: 2 retarget periods takes about 10 minutes to scan
• dedicated rpc: 2 retarget periods takes about 4 minutes to scan
• dedicated rpc loading block headers and coinbase tx only: 2 retarget periods takes under 5 seconds to scan

I'll update I've updated my branch, but the getblock 1 approach seems like the sweet spot. (It's probably also faster if your node doesn't have quite as convoluted a setup as mine)

[Sjors]

I found "cblocktime" confusing (CBlock time?), maybe tx0_locktime?

Indeed, it reads like "c block time", cb_locktime should work.

Also should report whether nseq is final or not. Could also just report "bip54_coinbase_readiness": true/false.

Ideally we keep it generic enough so it can be PR'd to Bitcoin Core indepedent of BIP54.

Maybe add a coinbase field to getblock with (a guaranteed to be small subset of) fields similar to the CoinbaseTx struct from bitcoin#33819 : version, sequence and lock_time. Could add an is_final bool for convenience, given its relevance.

---

Update: bitcoin#34512

[ajtowns]

ACK 55cd9a5 -- this looks good to me i think, and should be ready to merge

[darosior]

#99 (comment)

https://groups.google.com/g/bitcoindev/c/6TTlDwP2OQg

Per se, unless I'm missing something, I don't think the value of the nSequence field needs to be set

@ariard i have replied to you on the mailing list. TL;DR: it seems my explanation was lacking and you missed the purpose for making sure nSequence is never final.

to a precise value

Note also we don't constrain it to be a specific value. On the contrary it can be anything as long it's not the specific final value.

[ariard]

@ariard i have replied to you on the mailing list. TL;DR: it seems my explanation was lacking and you missed the purpose for making sure nSequence is never final.

@darosior I think I got your idea with the assumption that BIP54 can be buried in the future, and the check to be applied on all historical coinbase transactions, therefore ensuring any old coinbase cannot be used to generate a post-BIP54 action height identifier collision.

Replied, I’m still thinking you can make a simpler assumption by just relying on the PoW ordering mechanism itself. There can be only and only one block valid at a given chain height.

[ariard]

Note also we don't constrain it to be a specific value. On the contrary it can be anything as long it's not the specific final value.

Yes, I see it doesn’t have to be set to a specific value. See the point on protecting the coinbase nSequence field. It’s of independent interest, that the check is kept or not.

[ajtowns]

@darosior I think I got your idea with the assumption that BIP54 can be buried in the future, and the check to be applied on all historical coinbase transactions,

Historical coinbase transactions on mainnet, testnet3, testnet4 and signet all fail to comply with bip54 rules, so that approach isn't possible.

[ariard]

Historical coinbase transactions on mainnet, testnet3, testnet4 and signet all fail to comply with bip54 rules, so that approach isn't possible.

Okay so up to Darosior what he meant here on the ml:

"For instance, if BIP 54 activation height ever gets buried in Bitcoin Core, the BIP 30 check could just be disabled past
this height instead of having to figure out if we are on a chain that contains the historical BIP 34 activation block hash [3].

So strictly a buried activation height, nothing like a retroactive validation from genesis like it was done for BIP30.

Point is the additional check in nsequence field is superflous imho.

[ajtowns]

The issue is what happens if there is a reorg back to a point where neither BIP 34 or BIP 54 are active. At that point, BIP 30 checks must be enabled. With the nsequence check, those checks no longer need to be performed after BIP 54 is enabled because no coinbase that complies with its rules can have ever previously appeared in the blockchain. Also, this discussion should be on the mailing list, not here.