diff options
| -rw-r--r-- | configure.ac | 2 | ||||
| -rw-r--r-- | doc/man/bitcoin-cli.1 | 6 | ||||
| -rw-r--r-- | doc/man/bitcoin-qt.1 | 6 | ||||
| -rw-r--r-- | doc/man/bitcoin-tx.1 | 6 | ||||
| -rw-r--r-- | doc/man/bitcoin-util.1 | 6 | ||||
| -rw-r--r-- | doc/man/bitcoin-wallet.1 | 6 | ||||
| -rw-r--r-- | doc/man/bitcoind.1 | 6 | ||||
| -rw-r--r-- | doc/release-notes.md | 15 | ||||
| -rw-r--r-- | src/net_processing.cpp | 12 | ||||
| -rw-r--r-- | src/primitives/block.h | 8 | ||||
| -rw-r--r-- | src/test/validation_tests.cpp | 216 | ||||
| -rw-r--r-- | src/validation.cpp | 127 | ||||
| -rw-r--r-- | src/validation.h | 3 | ||||
| -rw-r--r-- | src/wallet/wallet.cpp | 4 | ||||
| -rwxr-xr-x | test/functional/p2p_mutated_blocks.py | 116 | ||||
| -rwxr-xr-x | test/functional/test_runner.py | 1 | ||||
| -rwxr-xr-x | test/functional/wallet_keypool.py | 16 |
17 files changed, 491 insertions, 65 deletions
diff --git a/configure.ac b/configure.ac index 9e2af8d8b5..3d9460a1e4 100644 --- a/configure.ac +++ b/configure.ac @@ -2,7 +2,7 @@ AC_PREREQ([2.69]) define(_CLIENT_VERSION_MAJOR, 25) define(_CLIENT_VERSION_MINOR, 2) define(_CLIENT_VERSION_BUILD, 0) -define(_CLIENT_VERSION_RC, 1) +define(_CLIENT_VERSION_RC, 2) define(_CLIENT_VERSION_IS_RELEASE, true) define(_COPYRIGHT_YEAR, 2023) define(_COPYRIGHT_HOLDERS,[The %s developers]) diff --git a/doc/man/bitcoin-cli.1 b/doc/man/bitcoin-cli.1 index 2dbe83068e..d55dfddde5 100644 --- a/doc/man/bitcoin-cli.1 +++ b/doc/man/bitcoin-cli.1 @@ -1,7 +1,7 @@ .\" DO NOT MODIFY THIS FILE! It was generated by help2man 1.49.3. -.TH BITCOIN-CLI "1" "February 2024" "bitcoin-cli v25.2.0rc1" "User Commands" +.TH BITCOIN-CLI "1" "March 2024" "bitcoin-cli v25.2.0rc2" "User Commands" .SH NAME -bitcoin-cli \- manual page for bitcoin-cli v25.2.0rc1 +bitcoin-cli \- manual page for bitcoin-cli v25.2.0rc2 .SH SYNOPSIS .B bitcoin-cli [\fI\,options\/\fR] \fI\,<command> \/\fR[\fI\,params\/\fR] \fI\,Send command to Bitcoin Core\/\fR @@ -15,7 +15,7 @@ bitcoin-cli \- manual page for bitcoin-cli v25.2.0rc1 .B bitcoin-cli [\fI\,options\/\fR] \fI\,help <command> Get help for a command\/\fR .SH DESCRIPTION -Bitcoin Core RPC client version v25.2.0rc1 +Bitcoin Core RPC client version v25.2.0rc2 .SH OPTIONS .HP \-? diff --git a/doc/man/bitcoin-qt.1 b/doc/man/bitcoin-qt.1 index 4204d5ca73..219fdea2ae 100644 --- a/doc/man/bitcoin-qt.1 +++ b/doc/man/bitcoin-qt.1 @@ -1,12 +1,12 @@ .\" DO NOT MODIFY THIS FILE! It was generated by help2man 1.49.3. -.TH BITCOIN-QT "1" "February 2024" "bitcoin-qt v25.2.0rc1" "User Commands" +.TH BITCOIN-QT "1" "March 2024" "bitcoin-qt v25.2.0rc2" "User Commands" .SH NAME -bitcoin-qt \- manual page for bitcoin-qt v25.2.0rc1 +bitcoin-qt \- manual page for bitcoin-qt v25.2.0rc2 .SH SYNOPSIS .B bitcoin-qt [\fI\,command-line options\/\fR] .SH DESCRIPTION -Bitcoin Core version v25.2.0rc1 +Bitcoin Core version v25.2.0rc2 .SH OPTIONS .HP \-? diff --git a/doc/man/bitcoin-tx.1 b/doc/man/bitcoin-tx.1 index b97f1bd9aa..a30db47356 100644 --- a/doc/man/bitcoin-tx.1 +++ b/doc/man/bitcoin-tx.1 @@ -1,7 +1,7 @@ .\" DO NOT MODIFY THIS FILE! It was generated by help2man 1.49.3. -.TH BITCOIN-TX "1" "February 2024" "bitcoin-tx v25.2.0rc1" "User Commands" +.TH BITCOIN-TX "1" "March 2024" "bitcoin-tx v25.2.0rc2" "User Commands" .SH NAME -bitcoin-tx \- manual page for bitcoin-tx v25.2.0rc1 +bitcoin-tx \- manual page for bitcoin-tx v25.2.0rc2 .SH SYNOPSIS .B bitcoin-tx [\fI\,options\/\fR] \fI\,<hex-tx> \/\fR[\fI\,commands\/\fR] \fI\,Update hex-encoded bitcoin transaction\/\fR @@ -9,7 +9,7 @@ bitcoin-tx \- manual page for bitcoin-tx v25.2.0rc1 .B bitcoin-tx [\fI\,options\/\fR] \fI\,-create \/\fR[\fI\,commands\/\fR] \fI\,Create hex-encoded bitcoin transaction\/\fR .SH DESCRIPTION -Bitcoin Core bitcoin\-tx utility version v25.2.0rc1 +Bitcoin Core bitcoin\-tx utility version v25.2.0rc2 .SH OPTIONS .HP \-? diff --git a/doc/man/bitcoin-util.1 b/doc/man/bitcoin-util.1 index 02593581c1..92631434c5 100644 --- a/doc/man/bitcoin-util.1 +++ b/doc/man/bitcoin-util.1 @@ -1,12 +1,12 @@ .\" DO NOT MODIFY THIS FILE! It was generated by help2man 1.49.3. -.TH BITCOIN-UTIL "1" "February 2024" "bitcoin-util v25.2.0rc1" "User Commands" +.TH BITCOIN-UTIL "1" "March 2024" "bitcoin-util v25.2.0rc2" "User Commands" .SH NAME -bitcoin-util \- manual page for bitcoin-util v25.2.0rc1 +bitcoin-util \- manual page for bitcoin-util v25.2.0rc2 .SH SYNOPSIS .B bitcoin-util [\fI\,options\/\fR] [\fI\,commands\/\fR] \fI\,Do stuff\/\fR .SH DESCRIPTION -Bitcoin Core bitcoin\-util utility version v25.2.0rc1 +Bitcoin Core bitcoin\-util utility version v25.2.0rc2 .SH OPTIONS .HP \-? diff --git a/doc/man/bitcoin-wallet.1 b/doc/man/bitcoin-wallet.1 index dda630ff6a..9d21044f84 100644 --- a/doc/man/bitcoin-wallet.1 +++ b/doc/man/bitcoin-wallet.1 @@ -1,9 +1,9 @@ .\" DO NOT MODIFY THIS FILE! It was generated by help2man 1.49.3. -.TH BITCOIN-WALLET "1" "February 2024" "bitcoin-wallet v25.2.0rc1" "User Commands" +.TH BITCOIN-WALLET "1" "March 2024" "bitcoin-wallet v25.2.0rc2" "User Commands" .SH NAME -bitcoin-wallet \- manual page for bitcoin-wallet v25.2.0rc1 +bitcoin-wallet \- manual page for bitcoin-wallet v25.2.0rc2 .SH DESCRIPTION -Bitcoin Core bitcoin\-wallet version v25.2.0rc1 +Bitcoin Core bitcoin\-wallet version v25.2.0rc2 .PP bitcoin\-wallet is an offline tool for creating and interacting with Bitcoin Core wallet files. By default bitcoin\-wallet will act on wallets in the default mainnet wallet directory in the datadir. diff --git a/doc/man/bitcoind.1 b/doc/man/bitcoind.1 index 30fe309972..3e3325e8e5 100644 --- a/doc/man/bitcoind.1 +++ b/doc/man/bitcoind.1 @@ -1,12 +1,12 @@ .\" DO NOT MODIFY THIS FILE! It was generated by help2man 1.49.3. -.TH BITCOIND "1" "February 2024" "bitcoind v25.2.0rc1" "User Commands" +.TH BITCOIND "1" "March 2024" "bitcoind v25.2.0rc2" "User Commands" .SH NAME -bitcoind \- manual page for bitcoind v25.2.0rc1 +bitcoind \- manual page for bitcoind v25.2.0rc2 .SH SYNOPSIS .B bitcoind [\fI\,options\/\fR] \fI\,Start Bitcoin Core\/\fR .SH DESCRIPTION -Bitcoin Core version v25.2.0rc1 +Bitcoin Core version v25.2.0rc2 .SH OPTIONS .HP \-? diff --git a/doc/release-notes.md b/doc/release-notes.md index eb85c31a6b..32072de6a9 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -1,9 +1,9 @@ -25.2rc1 Release Notes +25.2rc2 Release Notes ================== -Bitcoin Core version 25.2rc1 is now available from: +Bitcoin Core version 25.2rc2 is now available from: - <https://bitcoincore.org/bin/bitcoin-core-25.2/test.rc1> + <https://bitcoincore.org/bin/bitcoin-core-25.2/test.rc2> This release includes various bug fixes and performance improvements, as well as updated translations. @@ -51,6 +51,12 @@ Notable changes ### Wallet - #29176 wallet: Fix use-after-free in WalletBatch::EraseRecords +- #29510 wallet: `getrawchangeaddress` and `getnewaddress` failures should not affect keypools for descriptor wallets + +### P2P and network changes + +- #29412 p2p: Don't process mutated blocks +- #29524 p2p: Don't consider blocks mutated if they don't connect to known prev block Credits ======= @@ -60,6 +66,9 @@ Thanks to everyone who directly contributed to this release: - Martin Zumsande - Sebastian Falbesoner - MarcoFalke +- UdjinM6 +- dergoegge +- Greg Sanders As well as to everyone that helped with translations on [Transifex](https://www.transifex.com/bitcoin/bitcoin/). diff --git a/src/net_processing.cpp b/src/net_processing.cpp index d48a118dd6..43e15487f3 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4629,6 +4629,17 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, LogPrint(BCLog::NET, "received block %s peer=%d\n", pblock->GetHash().ToString(), pfrom.GetId()); + const CBlockIndex* prev_block{WITH_LOCK(m_chainman.GetMutex(), return m_chainman.m_blockman.LookupBlockIndex(pblock->hashPrevBlock))}; + + // Check for possible mutation if it connects to something we know so we can check for DEPLOYMENT_SEGWIT being active + if (prev_block && IsBlockMutated(/*block=*/*pblock, + /*check_witness_root=*/DeploymentActiveAfter(prev_block, m_chainman, Consensus::DEPLOYMENT_SEGWIT))) { + LogPrint(BCLog::NET, "Received mutated block from peer=%d\n", peer->m_id); + Misbehaving(*peer, 100, "mutated block"); + WITH_LOCK(cs_main, RemoveBlockRequest(pblock->GetHash(), peer->m_id)); + return; + } + bool forceProcessing = false; const uint256 hash(pblock->GetHash()); bool min_pow_checked = false; @@ -4644,7 +4655,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, mapBlockSource.emplace(hash, std::make_pair(pfrom.GetId(), true)); // Check work on this block against our anti-dos thresholds. - const CBlockIndex* prev_block = m_chainman.m_blockman.LookupBlockIndex(pblock->hashPrevBlock); if (prev_block && prev_block->nChainWork + CalculateHeadersWork({pblock->GetBlockHeader()}) >= GetAntiDoSWorkThreshold()) { min_pow_checked = true; } diff --git a/src/primitives/block.h b/src/primitives/block.h index bd11279a6e..6117c477f8 100644 --- a/src/primitives/block.h +++ b/src/primitives/block.h @@ -71,8 +71,10 @@ public: // network and disk std::vector<CTransactionRef> vtx; - // memory only - mutable bool fChecked; + // Memory-only flags for caching expensive checks + mutable bool fChecked; // CheckBlock() + mutable bool m_checked_witness_commitment{false}; // CheckWitnessCommitment() + mutable bool m_checked_merkle_root{false}; // CheckMerkleRoot() CBlock() { @@ -96,6 +98,8 @@ public: CBlockHeader::SetNull(); vtx.clear(); fChecked = false; + m_checked_witness_commitment = false; + m_checked_merkle_root = false; } CBlockHeader GetBlockHeader() const diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index 0b4c491615..c9b9671671 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -4,11 +4,16 @@ #include <chainparams.h> #include <consensus/amount.h> +#include <consensus/merkle.h> +#include <core_io.h> +#include <hash.h> #include <net.h> #include <signet.h> #include <uint256.h> #include <validation.h> +#include <string> + #include <test/util/setup_common.h> #include <boost/test/unit_test.hpp> @@ -144,4 +149,215 @@ BOOST_AUTO_TEST_CASE(test_assumeutxo) BOOST_CHECK_EQUAL(out210.nChainTx, 200U); } +BOOST_AUTO_TEST_CASE(block_malleation) +{ + // Test utilities that calls `IsBlockMutated` and then clears the validity + // cache flags on `CBlock`. + auto is_mutated = [](CBlock& block, bool check_witness_root) { + bool mutated{IsBlockMutated(block, check_witness_root)}; + block.fChecked = false; + block.m_checked_witness_commitment = false; + block.m_checked_merkle_root = false; + return mutated; + }; + auto is_not_mutated = [&is_mutated](CBlock& block, bool check_witness_root) { + return !is_mutated(block, check_witness_root); + }; + + // Test utilities to create coinbase transactions and insert witness + // commitments. + // + // Note: this will not include the witness stack by default to avoid + // triggering the "no witnesses allowed for blocks that don't commit to + // witnesses" rule when testing other malleation vectors. + auto create_coinbase_tx = [](bool include_witness = false) { + CMutableTransaction coinbase; + coinbase.vin.resize(1); + if (include_witness) { + coinbase.vin[0].scriptWitness.stack.resize(1); + coinbase.vin[0].scriptWitness.stack[0] = std::vector<unsigned char>(32, 0x00); + } + + coinbase.vout.resize(1); + coinbase.vout[0].scriptPubKey.resize(MINIMUM_WITNESS_COMMITMENT); + coinbase.vout[0].scriptPubKey[0] = OP_RETURN; + coinbase.vout[0].scriptPubKey[1] = 0x24; + coinbase.vout[0].scriptPubKey[2] = 0xaa; + coinbase.vout[0].scriptPubKey[3] = 0x21; + coinbase.vout[0].scriptPubKey[4] = 0xa9; + coinbase.vout[0].scriptPubKey[5] = 0xed; + + auto tx = MakeTransactionRef(coinbase); + assert(tx->IsCoinBase()); + return tx; + }; + auto insert_witness_commitment = [](CBlock& block, uint256 commitment) { + assert(!block.vtx.empty() && block.vtx[0]->IsCoinBase() && !block.vtx[0]->vout.empty()); + + CMutableTransaction mtx{*block.vtx[0]}; + CHash256().Write(commitment).Write(std::vector<unsigned char>(32, 0x00)).Finalize(commitment); + memcpy(&mtx.vout[0].scriptPubKey[6], commitment.begin(), 32); + block.vtx[0] = MakeTransactionRef(mtx); + }; + + { + CBlock block; + + // Empty block is expected to have merkle root of 0x0. + BOOST_CHECK(block.vtx.empty()); + block.hashMerkleRoot = uint256{1}; + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + block.hashMerkleRoot = uint256{}; + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false)); + + // Block with a single coinbase tx is mutated if the merkle root is not + // equal to the coinbase tx's hash. + block.vtx.push_back(create_coinbase_tx()); + BOOST_CHECK(block.vtx[0]->GetHash() != block.hashMerkleRoot); + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + block.hashMerkleRoot = block.vtx[0]->GetHash(); + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false)); + + // Block with two transactions is mutated if the merkle root does not + // match the double sha256 of the concatenation of the two transaction + // hashes. + block.vtx.push_back(MakeTransactionRef(CMutableTransaction{})); + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + HashWriter hasher; + hasher.write(Span(reinterpret_cast<const std::byte*>(block.vtx[0]->GetHash().data()), 32)); + hasher.write(Span(reinterpret_cast<const std::byte*>(block.vtx[1]->GetHash().data()), 32)); + block.hashMerkleRoot = hasher.GetHash(); + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false)); + + // Block with two transactions is mutated if any node is duplicate. + { + block.vtx[1] = block.vtx[0]; + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + HashWriter hasher; + hasher.write(Span(reinterpret_cast<const std::byte*>(block.vtx[0]->GetHash().data()), 32)); + hasher.write(Span(reinterpret_cast<const std::byte*>(block.vtx[1]->GetHash().data()), 32)); + block.hashMerkleRoot = hasher.GetHash(); + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + } + + // Blocks with 64-byte coinbase transactions are not considered mutated + block.vtx.clear(); + { + CMutableTransaction mtx; + mtx.vin.resize(1); + mtx.vout.resize(1); + mtx.vout[0].scriptPubKey.resize(4); + block.vtx.push_back(MakeTransactionRef(mtx)); + block.hashMerkleRoot = block.vtx.back()->GetHash(); + assert(block.vtx.back()->IsCoinBase()); + assert(GetSerializeSize(block.vtx.back(), PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) == 64); + } + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false)); + } + + { + // Test merkle root malleation + + // Pseudo code to mine transactions tx{1,2,3}: + // + // ``` + // loop { + // tx1 = random_tx() + // tx2 = random_tx() + // tx3 = deserialize_tx(txid(tx1) || txid(tx2)); + // if serialized_size_without_witness(tx3) == 64 { + // print(hex(tx3)) + // break + // } + // } + // ``` + // + // The `random_tx` function used to mine the txs below simply created + // empty transactions with a random version field. + CMutableTransaction tx1; + BOOST_CHECK(DecodeHexTx(tx1, "ff204bd0000000000000", /*try_no_witness=*/true, /*try_witness=*/false)); + CMutableTransaction tx2; + BOOST_CHECK(DecodeHexTx(tx2, "8ae53c92000000000000", /*try_no_witness=*/true, /*try_witness=*/false)); + CMutableTransaction tx3; + BOOST_CHECK(DecodeHexTx(tx3, "cdaf22d00002c6a7f848f8ae4d30054e61dcf3303d6fe01d282163341f06feecc10032b3160fcab87bdfe3ecfb769206ef2d991b92f8a268e423a6ef4d485f06", /*try_no_witness=*/true, /*try_witness=*/false)); + { + // Verify that double_sha256(txid1||txid2) == txid3 + HashWriter hasher; + hasher.write(Span(reinterpret_cast<const std::byte*>(tx1.GetHash().data()), 32)); + hasher.write(Span(reinterpret_cast<const std::byte*>(tx2.GetHash().data()), 32)); + assert(hasher.GetHash() == tx3.GetHash()); + // Verify that tx3 is 64 bytes in size (without witness). + assert(GetSerializeSize(tx3, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) == 64); + } + + CBlock block; + block.vtx.push_back(MakeTransactionRef(tx1)); + block.vtx.push_back(MakeTransactionRef(tx2)); + uint256 merkle_root = block.hashMerkleRoot = BlockMerkleRoot(block); + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false)); + + // Mutate the block by replacing the two transactions with one 64-byte + // transaction that serializes into the concatenation of the txids of + // the transactions in the unmutated block. + block.vtx.clear(); + block.vtx.push_back(MakeTransactionRef(tx3)); + BOOST_CHECK(!block.vtx.back()->IsCoinBase()); + BOOST_CHECK(BlockMerkleRoot(block) == merkle_root); + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + } + + { + CBlock block; + block.vtx.push_back(create_coinbase_tx(/*include_witness=*/true)); + { + CMutableTransaction mtx; + mtx.vin.resize(1); + mtx.vin[0].scriptWitness.stack.resize(1); + mtx.vin[0].scriptWitness.stack[0] = {0}; + block.vtx.push_back(MakeTransactionRef(mtx)); + } + block.hashMerkleRoot = BlockMerkleRoot(block); + // Block with witnesses is considered mutated if the witness commitment + // is not validated. + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false)); + // Block with invalid witness commitment is considered mutated. + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/true)); + + // Block with valid commitment is not mutated + { + auto commitment{BlockWitnessMerkleRoot(block)}; + insert_witness_commitment(block, commitment); + block.hashMerkleRoot = BlockMerkleRoot(block); + } + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/true)); + + // Malleating witnesses should be caught by `IsBlockMutated`. + { + CMutableTransaction mtx{*block.vtx[1]}; + assert(!mtx.vin[0].scriptWitness.stack[0].empty()); + ++mtx.vin[0].scriptWitness.stack[0][0]; + block.vtx[1] = MakeTransactionRef(mtx); + } + // Without also updating the witness commitment, the merkle root should + // not change when changing one of the witnesses. + BOOST_CHECK(block.hashMerkleRoot == BlockMerkleRoot(block)); + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/true)); + { + auto commitment{BlockWitnessMerkleRoot(block)}; + insert_witness_commitment(block, commitment); + block.hashMerkleRoot = BlockMerkleRoot(block); + } + BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/true)); + + // Test malleating the coinbase witness reserved value + { + CMutableTransaction mtx{*block.vtx[0]}; + mtx.vin[0].scriptWitness.stack.resize(0); + block.vtx[0] = MakeTransactionRef(mtx); + block.hashMerkleRoot = BlockMerkleRoot(block); + } + BOOST_CHECK(is_mutated(block, /*check_witness_root=*/true)); + } +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validation.cpp b/src/validation.cpp index 1fd8f0e326..5ef371b97b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3486,6 +3486,60 @@ static bool CheckBlockHeader(const CBlockHeader& block, BlockValidationState& st return true; } +static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state) +{ + if (block.m_checked_merkle_root) return true; + + bool mutated; + uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated); + if (block.hashMerkleRoot != hashMerkleRoot2) + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txnmrklroot", "hashMerkleRoot mismatch"); + + // Check for merkle tree malleability (CVE-2012-2459): repeating sequences + // of transactions in a block without affecting the merkle root of a block, + // while still invalidating it. + if (mutated) + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txns-duplicate", "duplicate transaction"); + + block.m_checked_merkle_root = true; + return true; +} + +static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_commitment, BlockValidationState& state) +{ + if (expect_witness_commitment) { + if (block.m_checked_witness_commitment) return true; + + int commitpos = GetWitnessCommitmentIndex(block); + if (commitpos != NO_WITNESS_COMMITMENT) { + bool malleated = false; + uint256 hashWitness = BlockWitnessMerkleRoot(block, &malleated); + // The malleation check is ignored; as the transaction tree itself + // already does not permit it, it is impossible to trigger in the + // witness tree. + if (block.vtx[0]->vin[0].scriptWitness.stack.size() != 1 || block.vtx[0]->vin[0].scriptWitness.stack[0].size() != 32) { + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-nonce-size", strprintf("%s : invalid witness reserved value size", __func__)); + } + CHash256().Write(hashWitness).Write(block.vtx[0]->vin[0].scriptWitness.stack[0]).Finalize(hashWitness); + if (memcmp(hashWitness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) { + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-merkle-match", strprintf("%s : witness merkle commitment mismatch", __func__)); + } + + block.m_checked_witness_commitment = true; + return true; + } + } + + // No witness data is allowed in blocks that don't commit to witness data, as this would otherwise leave room for spam + for (const auto& tx : block.vtx) { + if (tx->HasWitness()) { + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "unexpected-witness", strprintf("%s : unexpected witness data found", __func__)); + } + } + + return true; +} + bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot) { // These are checks that are independent of context. @@ -3504,17 +3558,8 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu } // Check the merkle root. - if (fCheckMerkleRoot) { - bool mutated; - uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated); - if (block.hashMerkleRoot != hashMerkleRoot2) - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txnmrklroot", "hashMerkleRoot mismatch"); - - // Check for merkle tree malleability (CVE-2012-2459): repeating sequences - // of transactions in a block without affecting the merkle root of a block, - // while still invalidating it. - if (mutated) - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txns-duplicate", "duplicate transaction"); + if (fCheckMerkleRoot && !CheckMerkleRoot(block, state)) { + return false; } // All potential-corruption validation must be done before we do any @@ -3605,6 +3650,37 @@ bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consens [&](const auto& header) { return CheckProofOfWork(header.GetHash(), header.nBits, consensusParams);}); } +bool IsBlockMutated(const CBlock& block, bool check_witness_root) +{ + BlockValidationState state; + if (!CheckMerkleRoot(block, state)) { + LogPrint(BCLog::VALIDATION, "Block mutated: %s\n", state.ToString()); + return true; + } + + if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { + // Consider the block mutated if any transaction is 64 bytes in size (see 3.1 + // in "Weaknesses in Bitcoin’s Merkle Root Construction": + // https://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20190225/a27d8837/attachment-0001.pdf). + // + // Note: This is not a consensus change as this only applies to blocks that + // don't have a coinbase transaction and would therefore already be invalid. + return std::any_of(block.vtx.begin(), block.vtx.end(), + [](auto& tx) { return ::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) == 64; }); + } else { + // Theoretically it is still possible for a block with a 64 byte + // coinbase transaction to be mutated but we neglect that possibility + // here as it requires at least 224 bits of work. + } + + if (!CheckWitnessMalleation(block, check_witness_root, state)) { + LogPrint(BCLog::VALIDATION, "Block mutated: %s\n", state.ToString()); + return true; + } + + return false; +} + arith_uint256 CalculateHeadersWork(const std::vector<CBlockHeader>& headers) { arith_uint256 total_work{0}; @@ -3713,33 +3789,8 @@ static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& stat // * There must be at least one output whose scriptPubKey is a single 36-byte push, the first 4 bytes of which are // {0xaa, 0x21, 0xa9, 0xed}, and the following 32 bytes are SHA256^2(witness root, witness reserved value). In case there are // multiple, the last one is used. - bool fHaveWitness = false; - if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_SEGWIT)) { - int commitpos = GetWitnessCommitmentIndex(block); - if (commitpos != NO_WITNESS_COMMITMENT) { - bool malleated = false; - uint256 hashWitness = BlockWitnessMerkleRoot(block, &malleated); - // The malleation check is ignored; as the transaction tree itself - // already does not permit it, it is impossible to trigger in the - // witness tree. - if (block.vtx[0]->vin[0].scriptWitness.stack.size() != 1 || block.vtx[0]->vin[0].scriptWitness.stack[0].size() != 32) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-nonce-size", strprintf("%s : invalid witness reserved value size", __func__)); - } - CHash256().Write(hashWitness).Write(block.vtx[0]->vin[0].scriptWitness.stack[0]).Finalize(hashWitness); - if (memcmp(hashWitness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-merkle-match", strprintf("%s : witness merkle commitment mismatch", __func__)); - } - fHaveWitness = true; - } - } - - // No witness data is allowed in blocks that don't commit to witness data, as this would otherwise leave room for spam - if (!fHaveWitness) { - for (const auto& tx : block.vtx) { - if (tx->HasWitness()) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "unexpected-witness", strprintf("%s : unexpected witness data found", __func__)); - } - } + if (!CheckWitnessMalleation(block, DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_SEGWIT), state)) { + return false; } // After the coinbase witness reserved value and commitment are verified, diff --git a/src/validation.h b/src/validation.h index fd0e2115f7..b6e2e66a44 100644 --- a/src/validation.h +++ b/src/validation.h @@ -352,6 +352,9 @@ bool TestBlockValidity(BlockValidationState& state, /** Check with the proof of work on each blockheader matches the value in nBits */ bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams); +/** Check if a block has been mutated (with respect to its merkle root and witness commitments). */ +bool IsBlockMutated(const CBlock& block, bool check_witness_root); + /** Return the sum of the work on a given set of headers */ arith_uint256 CalculateHeadersWork(const std::vector<CBlockHeader>& headers); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0d664a4539..e6276d7f78 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2596,8 +2596,10 @@ util::Result<CTxDestination> ReserveDestination::GetReservedDestination(bool int if (nIndex == -1) { CKeyPool keypool; - auto op_address = m_spk_man->GetReservedDestination(type, internal, nIndex, keypool); + int64_t index; + auto op_address = m_spk_man->GetReservedDestination(type, internal, index, keypool); if (!op_address) return op_address; + nIndex = index; address = *op_address; fInternal = keypool.fInternal; } diff --git a/test/functional/p2p_mutated_blocks.py b/test/functional/p2p_mutated_blocks.py new file mode 100755 index 0000000000..737edaf5bf --- /dev/null +++ b/test/functional/p2p_mutated_blocks.py @@ -0,0 +1,116 @@ +#!/usr/bin/env python3 +# Copyright (c) The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +""" +Test that an attacker can't degrade compact block relay by sending unsolicited +mutated blocks to clear in-flight blocktxn requests from other honest peers. +""" + +from test_framework.p2p import P2PInterface +from test_framework.messages import ( + BlockTransactions, + msg_cmpctblock, + msg_block, + msg_blocktxn, + HeaderAndShortIDs, +) +from test_framework.test_framework import BitcoinTestFramework +from test_framework.blocktools import ( + COINBASE_MATURITY, + create_block, + add_witness_commitment, + NORMAL_GBT_REQUEST_PARAMS, +) +from test_framework.util import assert_equal +from test_framework.wallet import MiniWallet +import copy + +class MutatedBlocksTest(BitcoinTestFramework): + def set_test_params(self): + self.setup_clean_chain = True + self.num_nodes = 1 + self.extra_args = [ + [ + "-testactivationheight=segwit@1", # causes unconnected headers/blocks to not have segwit considered deployed + ], + ] + + def run_test(self): + self.wallet = MiniWallet(self.nodes[0]) + self.generate(self.wallet, COINBASE_MATURITY) + + honest_relayer = self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=0, connection_type="outbound-full-relay") + attacker = self.nodes[0].add_p2p_connection(P2PInterface()) + + # Create new block with two transactions (coinbase + 1 self-transfer). + # The self-transfer transaction is needed to trigger a compact block + # `getblocktxn` roundtrip. + tx = self.wallet.create_self_transfer()["tx"] + block = create_block(tmpl=self.nodes[0].getblocktemplate(NORMAL_GBT_REQUEST_PARAMS), txlist=[tx]) + add_witness_commitment(block) + block.solve() + + # Create mutated version of the block by changing the transaction + # version on the self-transfer. + mutated_block = copy.deepcopy(block) + mutated_block.vtx[1].nVersion = 4 + + # Announce the new block via a compact block through the honest relayer + cmpctblock = HeaderAndShortIDs() + cmpctblock.initialize_from_block(block, use_witness=True) + honest_relayer.send_message(msg_cmpctblock(cmpctblock.to_p2p())) + + # Wait for a `getblocktxn` that attempts to fetch the self-transfer + def self_transfer_requested(): + if not honest_relayer.last_message.get('getblocktxn'): + return False + + get_block_txn = honest_relayer.last_message['getblocktxn'] + return get_block_txn.block_txn_request.blockhash == block.sha256 and \ + get_block_txn.block_txn_request.indexes == [1] + honest_relayer.wait_until(self_transfer_requested, timeout=5) + + # Block at height 101 should be the only one in flight from peer 0 + peer_info_prior_to_attack = self.nodes[0].getpeerinfo() + assert_equal(peer_info_prior_to_attack[0]['id'], 0) + assert_equal([101], peer_info_prior_to_attack[0]["inflight"]) + + # Attempt to clear the honest relayer's download request by sending the + # mutated block (as the attacker). + with self.nodes[0].assert_debug_log(expected_msgs=["Block mutated: bad-txnmrklroot, hashMerkleRoot mismatch"]): + attacker.send_message(msg_block(mutated_block)) + # Attacker should get disconnected for sending a mutated block + attacker.wait_for_disconnect(timeout=5) + + # Block at height 101 should *still* be the only block in-flight from + # peer 0 + peer_info_after_attack = self.nodes[0].getpeerinfo() + assert_equal(peer_info_after_attack[0]['id'], 0) + assert_equal([101], peer_info_after_attack[0]["inflight"]) + + # The honest relayer should be able to complete relaying the block by + # sending the blocktxn that was requested. + block_txn = msg_blocktxn() + block_txn.block_transactions = BlockTransactions(blockhash=block.sha256, transactions=[tx]) + honest_relayer.send_and_ping(block_txn) + assert_equal(self.nodes[0].getbestblockhash(), block.hash) + + # Check that unexpected-witness mutation check doesn't trigger on a header that doesn't connect to anything + assert_equal(len(self.nodes[0].getpeerinfo()), 1) + attacker = self.nodes[0].add_p2p_connection(P2PInterface()) + block_missing_prev = copy.deepcopy(block) + block_missing_prev.hashPrevBlock = 123 + block_missing_prev.solve() + + # Attacker gets a DoS score of 10, not immediately disconnected, so we do it 10 times to get to 100 + for _ in range(10): + assert_equal(len(self.nodes[0].getpeerinfo()), 2) + with self.nodes[0].assert_debug_log(expected_msgs=["AcceptBlock FAILED (prev-blk-not-found)"]): + attacker.send_message(msg_block(block_missing_prev)) + attacker.wait_for_disconnect(timeout=5) + + +if __name__ == '__main__': + MutatedBlocksTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 2ff877e820..2b04a5a7d2 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -275,6 +275,7 @@ BASE_SCRIPTS = [ 'wallet_crosschain.py', 'mining_basic.py', 'feature_signet.py', + 'p2p_mutated_blocks.py', 'wallet_implicitsegwit.py --legacy-wallet', 'rpc_named_arguments.py', 'feature_startupnotify.py', diff --git a/test/functional/wallet_keypool.py b/test/functional/wallet_keypool.py index bd97851153..6331a5f456 100755 --- a/test/functional/wallet_keypool.py +++ b/test/functional/wallet_keypool.py @@ -103,11 +103,18 @@ class KeyPoolTest(BitcoinTestFramework): nodes[0].getrawchangeaddress() nodes[0].getrawchangeaddress() nodes[0].getrawchangeaddress() - addr = set() + # remember keypool sizes + wi = nodes[0].getwalletinfo() + kp_size_before = [wi['keypoolsize_hd_internal'], wi['keypoolsize']] # the next one should fail assert_raises_rpc_error(-12, "Keypool ran out", nodes[0].getrawchangeaddress) + # check that keypool sizes did not change + wi = nodes[0].getwalletinfo() + kp_size_after = [wi['keypoolsize_hd_internal'], wi['keypoolsize']] + assert_equal(kp_size_before, kp_size_after) # drain the external keys + addr = set() addr.add(nodes[0].getnewaddress(address_type="bech32")) addr.add(nodes[0].getnewaddress(address_type="bech32")) addr.add(nodes[0].getnewaddress(address_type="bech32")) @@ -115,8 +122,15 @@ class KeyPoolTest(BitcoinTestFramework): addr.add(nodes[0].getnewaddress(address_type="bech32")) addr.add(nodes[0].getnewaddress(address_type="bech32")) assert len(addr) == 6 + # remember keypool sizes + wi = nodes[0].getwalletinfo() + kp_size_before = [wi['keypoolsize_hd_internal'], wi['keypoolsize']] # the next one should fail assert_raises_rpc_error(-12, "Error: Keypool ran out, please call keypoolrefill first", nodes[0].getnewaddress) + # check that keypool sizes did not change + wi = nodes[0].getwalletinfo() + kp_size_after = [wi['keypoolsize_hd_internal'], wi['keypoolsize']] + assert_equal(kp_size_before, kp_size_after) # refill keypool with three new addresses nodes[0].walletpassphrase('test', 1) |