Prevent 64 byte txs at consensus level

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/validation.cpp

@@ -2401,6 +2401,7 @@ unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Chainstat
     return flags;
 }
 
+static bool ContextualBlockPreCheck(const CBlock& block, BlockValidationState& state, const ChainstateManager& chainman, const CBlockIndex* pindexPrev);
 
 /** 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()
@@ -2418,6 +2419,11 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
     const auto time_start{SteadyClock::now()};
     const CChainParams& params{m_chainman.GetParams()};
 
+    if (!ContextualBlockPreCheck(block, state, m_chainman, pindex->pprev)) {
+        LogError("%s: Consensus::ContextualBlockPreCheck: %s", __func__, state.ToString());
+        return false;
+    }
+
     // Check it again in case a previous version let a bad block in
     // NOTE: We don't currently (re-)invoke ContextualCheckBlock() or
     // ContextualCheckBlockHeader() here. This means that if we add a new
@@ -4196,6 +4202,28 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio
     return true;
 }
 
+/**
+ * We want to enforce certain rules (specifically the 64-byte transaction check)
+ * before we call CheckBlock to check the merkle root. This allows us to enforce
+ * malleability checks which may interact with other CheckBlock checks.
+ * This is currently called both in AcceptBlock prior to writing the block to
+ * disk and in ConnectBlock.
+ * Note that as this is called before merkle-tree checks so must never return a
+ * non-malleable error condition.
+ */
+static bool ContextualBlockPreCheck(const CBlock& block, BlockValidationState& state, const ChainstateManager& chainman, const CBlockIndex* pindexPrev)
+{
+    if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_64BYTETX)) {
+      for (const auto& tx : block.vtx) {
+            if (::GetSerializeSize(TX_NO_WITNESS(tx)) == 64) {
+                return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "64-byte-transaction", strprintf("size of tx %s without witness is 64 bytes", tx->GetHash().ToString()));
+            }
+        }
+    }
+
+    return true;
+}
+
 /** NOTE: This function is not currently invoked by ConnectBlock(), so we
  *  should consider upgrade issues if we change which consensus rules are
  *  enforced in this function (eg by adding a new consensus rule). See comment
@@ -4478,7 +4506,8 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr<const CBlock>& pblock,
 
     const CChainParams& params{GetParams()};
 
-    if (!CheckBlock(block, state, params.GetConsensus()) ||
+    if (!ContextualBlockPreCheck(block, state, *this, pindex->pprev) ||
+        !CheckBlock(block, state, params.GetConsensus()) ||
         !ContextualCheckBlock(block, state, *this, pindex->pprev)) {
         if (state.IsInvalid() && state.GetResult() != BlockValidationResult::BLOCK_MUTATED) {
             pindex->nStatus |= BLOCK_FAILED_VALID;
@@ -4613,6 +4642,10 @@ bool TestBlockValidity(BlockValidationState& state,
         LogError("%s: Consensus::ContextualCheckBlockHeader: %s\n", __func__, state.ToString());
         return false;
     }
+    if (!ContextualBlockPreCheck(block, state, chainstate.m_chainman, pindexPrev)) {
+        LogError("%s: Consensus::ContextualBlockPreCheck: %s", __func__, state.ToString());
+        return false;
+    }
     if (!CheckBlock(block, state, chainparams.GetConsensus(), fCheckPOW, fCheckMerkleRoot)) {
         LogError("%s: Consensus::CheckBlock: %s\n", __func__, state.ToString());
         return false;
@@ -4733,6 +4766,11 @@ VerifyDBResult CVerifyDB::VerifyDB(
             return VerifyDBResult::CORRUPTED_BLOCK_DB;
         }
         // check level 1: verify block validity
+        if (nCheckLevel >= 1 && !ContextualBlockPreCheck(block, state, chainstate.m_chainman, pindex->pprev)) {
+            LogPrintf("Verification error: found bad block at %d due to soft-fork, hash=%s (%s)\n",
+                         pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString());
+            return VerifyDBResult::CORRUPTED_BLOCK_DB;
+        }
         if (nCheckLevel >= 1 && !CheckBlock(block, state, consensus_params)) {
             LogPrintf("Verification error: found bad block at %d, hash=%s (%s)\n",
                       pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString());

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.
@@ -115,10 +119,12 @@ def get_tx(self):
 
 # The following check prevents exploit of lack of merkle
 # tree depth commitment (CVE-2017-12842)
-class SizeTooSmall(BadTxTemplate):
+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()
@@ -129,6 +135,21 @@ def get_tx(self):
         tx.calc_sha256()
         return tx
 
+class SizeSub64(BadTxTemplate):
+    reject_reason = "tx-size-small"
+    expect_disconnect = False
+    valid_in_block = True
+    wants_signature = False
+
+    def get_tx(self):
+        tx = CTransaction()
+        tx.vin.append(self.valid_txin)
+        tx.vout.append(CTxOut(0, CScript([OP_RETURN] + ([OP_0] * (MIN_PADDING - 3)))))
+        assert len(tx.serialize_without_witness()) == 63
+        assert MIN_STANDARD_TX_NONWITNESS_SIZE - 1 == 64
+        tx.calc_sha256()
+        return tx
+
 
 class BadInputOutpointIndex(BadTxTemplate):
     # Won't be rejected - nonexistent outpoint index is treated as an orphan since the coins

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/p2p_ibd_stalling.py

@@ -81,7 +81,7 @@ def run_test(self):
 
         # Need to wait until 1023 blocks are received - the magic total bytes number is a workaround in lack of an rpc
         # returning the number of downloaded (but not connected) blocks.
-        bytes_recv = 172761 if not self.options.v2transport else 169692
+        bytes_recv = 172761+2*1023 if not self.options.v2transport else 169692+2*1023
         self.wait_until(lambda: self.total_bytes_recv_for_blocks() == bytes_recv)
 
         self.all_sync_send_with_ping(peers)

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,
+            },
           }
         })
 

test/functional/test_framework/blocktools.py

@@ -37,6 +37,7 @@
     OP_1,
     OP_RETURN,
     OP_TRUE,
+    OP_DROP,
 )
 from .script_util import (
     key_to_p2pk_script,
@@ -144,7 +145,8 @@ def create_coinbase(height, pubkey=None, *, script_pubkey=None, extra_output_scr
     elif script_pubkey is not None:
         coinbaseoutput.scriptPubKey = script_pubkey
     else:
-        coinbaseoutput.scriptPubKey = CScript([OP_TRUE])
+        # Use two OP_TRUEs and an OP_DROP to ensure we're always > 64 bytes in non-witness size
+        coinbaseoutput.scriptPubKey = CScript([OP_TRUE, OP_TRUE, OP_DROP])
     coinbase.vout = [coinbaseoutput]
     if extra_output_script is not None:
         coinbaseoutput2 = CTxOut()
@@ -161,7 +163,7 @@ def create_tx_with_script(prevtx, n, script_sig=b"", *, amount, output_script=No
        Can optionally pass scriptPubKey and scriptSig, default is anyone-can-spend output.
     """
     if output_script is None:
-        output_script = CScript()
+        output_script = CScript([OP_TRUE, OP_DROP, OP_TRUE, OP_DROP])
     tx = CTransaction()
     assert n < len(prevtx.vout)
     tx.vin.append(CTxIn(COutPoint(prevtx.sha256, n), script_sig, SEQUENCE_FINAL))