Make 64 byte txs consensus invalid

The 64-byte transaction check is executed in a CheckBlock, which now
optionally receives a chainman and pindexPrev context (to allow checking
soft-fork activation status). Also ensures that CheckBlock will always
return BLOCK_MUTATED errors when mutation is possible, but not at other
times, which allows removing redundant calls to CheckBlock in a future
patch.

Co-Authored-By: Matt Corallo <git@bluematt.me>

src/blockencodings.cpp

@@ -207,7 +207,7 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
 
     BlockValidationState state;
     CheckBlockFn check_block = m_check_block_mock ? m_check_block_mock : CheckBlock;
-    if (!check_block(block, state, Params().GetConsensus(), /*fCheckPoW=*/true, /*fCheckMerkleRoot=*/true)) {
+    if (!check_block(block, state, Params().GetConsensus(), /*fCheckPoW=*/true, /*fCheckMerkleRoot=*/true, /*chainman=*/nullptr, /*pprevIndex=*/nullptr)) {
         // TODO: We really want to just check merkle tree manually here,
         // but that is expensive, and CheckBlock caches a block's
         // "checked-status" (in the CBlock?). CBlock should be able to

src/blockencodings.h

@@ -14,6 +14,8 @@ class BlockValidationState;
 namespace Consensus {
 struct Params;
 };
+class ChainstateManager;
+class CBlockIndex;
 
 // Transaction compression schemes for compact block relay can be introduced by writing
 // an actual formatter here.
@@ -141,7 +143,7 @@ class PartiallyDownloadedBlock {
     CBlockHeader header;
 
     // Can be overridden for testing
-    using CheckBlockFn = std::function<bool(const CBlock&, BlockValidationState&, const Consensus::Params&, bool, bool)>;
+    using CheckBlockFn = std::function<bool(const CBlock&, BlockValidationState&, const Consensus::Params&, bool, bool, const ChainstateManager*, const CBlockIndex*)>;
     CheckBlockFn m_check_block_mock{nullptr};
 
     explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {}

src/consensus/params.h

@@ -35,6 +35,7 @@ enum DeploymentPos : uint16_t {
     DEPLOYMENT_CHECKTEMPLATEVERIFY, // Deployment of CTV (BIP 119)
     DEPLOYMENT_ANYPREVOUT,
     DEPLOYMENT_OP_CAT,
+    DEPLOYMENT_64BYTETX,
     // NOTE: Also add new deployments to VersionBitsDeploymentInfo in deploymentinfo.cpp
     MAX_VERSION_BITS_DEPLOYMENTS
 };

src/deploymentinfo.cpp

@@ -27,6 +27,10 @@ const struct VBDeploymentInfo VersionBitsDeploymentInfo[Consensus::MAX_VERSION_B
         /*.name =*/ "opcat",
         /*.gbt_force =*/ true,
     },
+    {
+        /*.name =*/ "64bytetx",
+        /*.gbt_force =*/ true,
+    },
 };
 
 std::string DeploymentName(Consensus::BuriedDeployment dep)

src/kernel/chainparams.cpp

@@ -584,6 +584,8 @@ class CRegTestParams : public CChainParams
         consensus.vDeployments[Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY] = SetupDeployment{.activate = 0x60007700, .abandon = 0x40007700, .always = true};
         consensus.vDeployments[Consensus::DEPLOYMENT_ANYPREVOUT] = SetupDeployment{.activate = 0x60007600, .abandon = 0x40007600, .always = true};
         consensus.vDeployments[Consensus::DEPLOYMENT_OP_CAT] = SetupDeployment{.activate = 0x62000100, .abandon = 0x42000100, .always = true};
+        consensus.vDeployments[Consensus::DEPLOYMENT_64BYTETX] = SetupDeployment{.activate = 0x60000001, .abandon = 0x40000001, .always = true}; // needs BIN assigned
+
         consensus.nMinimumChainWork = uint256{};
         consensus.defaultAssumeValid = uint256{};
 

src/rpc/blockchain.cpp

@@ -1396,6 +1396,7 @@ UniValue DeploymentInfo(const CBlockIndex* blockindex, const ChainstateManager&
     SoftForkDescPushBack(blockindex, softforks, chainman, Consensus::DEPLOYMENT_CHECKTEMPLATEVERIFY);
     SoftForkDescPushBack(blockindex, softforks, chainman, Consensus::DEPLOYMENT_ANYPREVOUT);
     SoftForkDescPushBack(blockindex, softforks, chainman, Consensus::DEPLOYMENT_OP_CAT);
+    SoftForkDescPushBack(blockindex, softforks, chainman, Consensus::DEPLOYMENT_64BYTETX);
     return softforks;
 }
 } // anon namespace

src/test/fuzz/partially_downloaded_block.cpp

@@ -33,7 +33,7 @@ void initialize_pdb()
 
 PartiallyDownloadedBlock::CheckBlockFn FuzzedCheckBlock(std::optional<BlockValidationResult> result)
 {
-    return [result](const CBlock&, BlockValidationState& state, const Consensus::Params&, bool, bool) {
+    return [result](const CBlock&, BlockValidationState& state, const Consensus::Params&, bool, bool, const ChainstateManager*, const CBlockIndex*) {
         if (result) {
             return state.Invalid(*result);
         }

src/validation.cpp

@@ -2401,7 +2401,6 @@ unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Chainstat
     return flags;
 }
 
-
 /** Apply the effects of this block (with given index) on the UTXO set represented by coins.
  *  Validity checks that depend on the UTXO set are also done; ConnectBlock()
  *  can fail if those validity checks fail (among other reasons). */
@@ -2431,7 +2430,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
     // is enforced in ContextualCheckBlockHeader(); we wouldn't want to
     // re-enforce that rule here (at least until we make it impossible for
     // the clock to go backward).
-    if (!CheckBlock(block, state, params.GetConsensus(), !fJustCheck, !fJustCheck)) {
+    if (!CheckBlock(block, state, params.GetConsensus(), !fJustCheck, !fJustCheck, &m_chainman, pindex->pprev)) {
         if (state.GetResult() == BlockValidationResult::BLOCK_MUTATED) {
             // We don't write down blocks to disk if they may have been
             // corrupted, so this should be impossible unless we're having hardware
@@ -3981,7 +3980,7 @@ static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_comm
     return true;
 }
 
-bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot)
+bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot, const ChainstateManager* chainman, const CBlockIndex* pindexPrev)
 {
     // These are checks that are independent of context.
 
@@ -3993,14 +3992,42 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
     if (!CheckBlockHeader(block, state, consensusParams, fCheckPOW))
         return false;
 
+    /**
+     * We want to enforce certain rules (specifically the 64-byte transaction check)
+     * before we check the merkle root. This allows us to enforce
+     * malleability checks which may interact with other CheckBlock checks.
+     */
+    bool all_64b = true;
+    bool any_64b = false;
+    for (const auto& tx : block.vtx) {
+        if (::GetSerializeSize(TX_NO_WITNESS(tx)) == 64) {
+            any_64b = true;
+        } else {
+            all_64b = false;
+        }
+        if (any_64b && !all_64b) break;
+    }
+
+    // if there are txs, and they are all 64 bytes, then we may have been given a mutated block
+    auto MaybeMutated = [&](bool r) -> bool {
+        if (any_64b && all_64b && !r && state.IsInvalid()) {
+            state.Invalid(BlockValidationResult::BLOCK_MUTATED, state.GetRejectReason(), state.GetDebugMessage());
+        }
+        return r;
+    };
+
+    if (chainman != nullptr && DeploymentActiveAfter(pindexPrev, *chainman, Consensus::DEPLOYMENT_64BYTETX) && any_64b) {
+        return MaybeMutated(state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "64-byte-transaction", strprintf("64-byte tx (exclusing witness) present in block")));
+    }
+
     // Signet only: check block solution
     if (consensusParams.signet_blocks && fCheckPOW && !CheckSignetBlockSolution(block, consensusParams)) {
-        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-signet-blksig", "signet block signature validation failure");
+        return MaybeMutated(state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-signet-blksig", "signet block signature validation failure"));
     }
 
     // Check the merkle root.
     if (fCheckMerkleRoot && !CheckMerkleRoot(block, state)) {
-        return false;
+        return MaybeMutated(false);
     }
 
     // All potential-corruption validation must be done before we do any
@@ -4011,14 +4038,14 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
 
     // Size limits
     if (block.vtx.empty() || block.vtx.size() * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT || ::GetSerializeSize(TX_NO_WITNESS(block)) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT)
-        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-length", "size limits failed");
+        return MaybeMutated(state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-length", "size limits failed"));
 
     // First transaction must be coinbase, the rest must not be
     if (block.vtx.empty() || !block.vtx[0]->IsCoinBase())
-        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cb-missing", "first tx is not coinbase");
+        return MaybeMutated(state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cb-missing", "first tx is not coinbase"));
     for (unsigned int i = 1; i < block.vtx.size(); i++)
         if (block.vtx[i]->IsCoinBase())
-            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cb-multiple", "more than one coinbase");
+            return MaybeMutated(state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cb-multiple", "more than one coinbase"));
 
     // Check transactions
     // Must check for duplicate inputs (see CVE-2018-17144)
@@ -4028,8 +4055,8 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
             // CheckBlock() does context-free validation checks. The only
             // possible failures are consensus failures.
             assert(tx_state.GetResult() == TxValidationResult::TX_CONSENSUS);
-            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(),
-                                 strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), tx_state.GetDebugMessage()));
+            return MaybeMutated(state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(),
+                                 strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), tx_state.GetDebugMessage())));
         }
     }
     unsigned int nSigOps = 0;
@@ -4038,9 +4065,9 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
         nSigOps += GetLegacySigOpCount(*tx);
     }
     if (nSigOps * WITNESS_SCALE_FACTOR > MAX_BLOCK_SIGOPS_COST)
-        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops", "out-of-bounds SigOpCount");
+        return MaybeMutated(state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops", "out-of-bounds SigOpCount"));
 
-    if (fCheckPOW && fCheckMerkleRoot)
+    if (fCheckPOW && fCheckMerkleRoot && (!any_64b || chainman != nullptr))
         block.fChecked = true;
 
     return true;
@@ -4478,7 +4505,7 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr<const CBlock>& pblock,
 
     const CChainParams& params{GetParams()};
 
-    if (!CheckBlock(block, state, params.GetConsensus()) ||
+    if (!CheckBlock(block, state, params.GetConsensus(), true, true, this, pindex->pprev) ||
         !ContextualCheckBlock(block, state, *this, pindex->pprev)) {
         if (state.IsInvalid() && state.GetResult() != BlockValidationResult::BLOCK_MUTATED) {
             pindex->nStatus |= BLOCK_FAILED_VALID;
@@ -4613,7 +4640,7 @@ bool TestBlockValidity(BlockValidationState& state,
         LogError("%s: Consensus::ContextualCheckBlockHeader: %s\n", __func__, state.ToString());
         return false;
     }
-    if (!CheckBlock(block, state, chainparams.GetConsensus(), fCheckPOW, fCheckMerkleRoot)) {
+    if (!CheckBlock(block, state, chainparams.GetConsensus(), fCheckPOW, fCheckMerkleRoot, &chainstate.m_chainman, pindexPrev)) {
         LogError("%s: Consensus::CheckBlock: %s\n", __func__, state.ToString());
         return false;
     }
@@ -4733,7 +4760,7 @@ VerifyDBResult CVerifyDB::VerifyDB(
             return VerifyDBResult::CORRUPTED_BLOCK_DB;
         }
         // check level 1: verify block validity
-        if (nCheckLevel >= 1 && !CheckBlock(block, state, consensus_params)) {
+        if (nCheckLevel >= 1 && !CheckBlock(block, state, consensus_params, true, true, &chainstate.m_chainman, pindex->pprev)) {
             LogPrintf("Verification error: found bad block at %d, hash=%s (%s)\n",
                       pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString());
             return VerifyDBResult::CORRUPTED_BLOCK_DB;

src/validation.h

@@ -389,7 +389,7 @@ class ValidationCache
 /** Functions for validating blocks and updating the block tree */
 
 /** Context-independent validity checks */
-bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true);
+bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true, const ChainstateManager* chainman = nullptr, const CBlockIndex* pindexPrev = nullptr);
 
 /** Check a block is completely valid from start to finish (only works on top of our current best block) */
 bool TestBlockValidity(BlockValidationState& state,

test/functional/data/invalid_txs.py

@@ -76,6 +76,9 @@ class BadTxTemplate:
     # the mempool (i.e. does it violate policy but not consensus)?
     valid_in_block = False
 
+    # Do we need a signature for this tx
+    wants_signature = True
+
     def __init__(self, *, spend_tx=None, spend_block=None):
         self.spend_tx = spend_block.vtx[0] if spend_block else spend_tx
         self.spend_avail = sum(o.nValue for o in self.spend_tx.vout)
@@ -101,6 +104,7 @@ def get_tx(self):
 class InputMissing(BadTxTemplate):
     reject_reason = "bad-txns-vin-empty"
     expect_disconnect = True
+    wants_signature = False
 
     # We use a blank transaction here to make sure
     # it is interpreted as a non-witness transaction.
@@ -118,7 +122,9 @@ def get_tx(self):
 class SizeExactly64(BadTxTemplate):
     reject_reason = "tx-size-small"
     expect_disconnect = False
-    valid_in_block = True
+    valid_in_block = False
+    wants_signature = False
+    block_reject_reason = "64-byte-transaction"
 
     def get_tx(self):
         tx = CTransaction()
@@ -133,6 +139,7 @@ class SizeSub64(BadTxTemplate):
     reject_reason = "tx-size-small"
     expect_disconnect = False
     valid_in_block = True
+    wants_signature = False
 
     def get_tx(self):
         tx = CTransaction()

test/functional/feature_block.py

@@ -159,7 +159,7 @@ def run_test(self):
             blockname = f"for_invalid.{TxTemplate.__name__}"
             self.next_block(blockname)
             badtx = template.get_tx()
-            if TxTemplate != invalid_txs.InputMissing:
+            if template.wants_signature:
                 self.sign_tx(badtx, attempt_spend_tx)
             badtx.rehash()
             badblock = self.update_block(blockname, [badtx])

test/functional/rpc_blockchain.py

@@ -261,6 +261,20 @@ def check_signalling_deploymentinfo_result(self, gdi_result, height, blockhash):
                 'height': 0,
                 'active': True,
             },
+            '64bytetx': {
+                'type': 'heretical',
+                'heretical': {
+                    'binana-id': 'BIN-2016-0000-001',
+                    'start_time': -1,
+                    'timeout': 0x7fffffffffffffff,  # testdummy does not have a timeout so is set to the max int64 value
+                    'period': 144,
+                    'status': 'active',
+                    'status_next': 'active',
+                    'since': 0,
+                },
+                'active': True,
+                'height': 0,
+            },
           }
         })