From 4b3e2cc8c93885d2df071d2e6524e8225daba581 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 25 Feb 2019 17:30:55 -0500 Subject: [PATCH 1/3] Avoid creating <= 64-byte transactions in most functional tests. --- test/functional/p2p_ibd_stalling.py | 2 +- test/functional/test_framework/blocktools.py | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/test/functional/p2p_ibd_stalling.py b/test/functional/p2p_ibd_stalling.py index 4348bf7ee992..802a06eb851b 100755 --- a/test/functional/p2p_ibd_stalling.py +++ b/test/functional/p2p_ibd_stalling.py @@ -82,7 +82,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) diff --git a/test/functional/test_framework/blocktools.py b/test/functional/test_framework/blocktools.py index 38600bc005a1..c479e465c577 100644 --- a/test/functional/test_framework/blocktools.py +++ b/test/functional/test_framework/blocktools.py @@ -38,6 +38,7 @@ OP_0, OP_RETURN, OP_TRUE, + OP_DROP, ) from .script_util import ( key_to_p2pk_script, @@ -164,7 +165,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() @@ -181,7 +183,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)) From 33e3b23fa18cf3aa4969e5067010448e01c2a456 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Tue, 28 Mar 2023 15:06:26 +1000 Subject: [PATCH 2/3] [test] Separate 64B and 63B tx size tests --- test/functional/data/invalid_txs.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py index d2d7202d8601..ac2f4d286292 100644 --- a/test/functional/data/invalid_txs.py +++ b/test/functional/data/invalid_txs.py @@ -115,7 +115,7 @@ 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 @@ -129,6 +129,20 @@ def get_tx(self): tx.calc_sha256() return tx +class SizeSub64(BadTxTemplate): + reject_reason = "tx-size-small" + expect_disconnect = False + valid_in_block = True + + 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 From 76c9f08dfde8c42e9ebd0055be5a7a2a0e9f41ba Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Tue, 9 Apr 2024 20:18:53 +1000 Subject: [PATCH 3/3] Make 64 byte txs consensus invalid The 64-byte transaction check is executed in a new ContextualBlockPreCheck which must be run before CheckBlock (at least in the final checking before writing the block to disk). This function is a bit awkward but is seemingly the simplest way to implement the new check, with the caveat that, because the new function is called before CheckBlock, it can never return a non-CorruptionPossible error state. Co-Authored-By: Matt Corallo --- src/binana/64btx.json | 6 +++++ src/validation.cpp | 41 ++++++++++++++++++++++++++++- test/functional/data/invalid_txs.py | 9 ++++++- test/functional/feature_block.py | 2 +- 4 files changed, 55 insertions(+), 3 deletions(-) create mode 100644 src/binana/64btx.json diff --git a/src/binana/64btx.json b/src/binana/64btx.json new file mode 100644 index 000000000000..ed771a419304 --- /dev/null +++ b/src/binana/64btx.json @@ -0,0 +1,6 @@ +{ + "binana": [2025, 1, 1], + "deployment": "64BYTETX", + "scriptverify": true, + "scriptverify_discourage": false +} diff --git a/src/validation.cpp b/src/validation.cpp index 8d970432a93c..b894780c6bb7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2421,6 +2421,8 @@ 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() * can fail if those validity checks fail (among other reasons). */ @@ -2437,6 +2439,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 @@ -4241,6 +4248,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 @@ -4523,7 +4552,8 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr& 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; @@ -4658,6 +4688,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; @@ -4785,6 +4819,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()); diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py index ac2f4d286292..e4d85d065e37 100644 --- a/test/functional/data/invalid_txs.py +++ b/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() diff --git a/test/functional/feature_block.py b/test/functional/feature_block.py index 2dfa568c5b6c..2b56da6a9b62 100755 --- a/test/functional/feature_block.py +++ b/test/functional/feature_block.py @@ -160,7 +160,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])