aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorglozow <gloriajzhao@gmail.com>2024-03-11 09:46:22 +0000
committerglozow <gloriajzhao@gmail.com>2024-03-11 09:54:18 +0000
commitc33e83a53a863e55413bee2893716b93165bf656 (patch)
treeefacfa32b84fbb4f2ebd0594180cae34094d1d2f
parenta718bfafe7ad38bab8a5782ae0db719480984238 (diff)
parentc68d4d0ac5b8537ba5e1c0c512b807768e1c5c1f (diff)
Merge bitcoin/bitcoin#29509: [26.x] backports and final changes for v26.1rc2v26.1rc2
c68d4d0ac5b8537ba5e1c0c512b807768e1c5c1f [doc] update manual pages for v26.1rc2 (glozow) bd715bfb3030f392b3b19f9a05aada48e385a0d9 [build] bump version to v26.1rc2 (glozow) b6d006d2a2b840e4a5af96c8d838e1cf589d3bce update release notes 26.1 (glozow) fce992b38e59c90babe505eda0d72f05d79eb2f3 fuzz: restrict fopencookie usage to Linux & FreeBSD (fanquake) 40c56a4d1341017b02dcb71882b1b1f03f880b1d test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures (UdjinM6) 7c82b2758c6bcfb8a94d2086f0d40088286815e8 wallet: Avoid updating `ReserveDestination::nIndex` when `GetReservedDestination` fails (UdjinM6) b5419ce6b621121bb1a0ec497968eb16cc012c39 p2p: Don't consider blocks mutated if they don't connect to known prev block (Greg Sanders) 0535c253fe71ae9d875827cafed41a8889f4a702 [test] IsBlockMutated unit tests (dergoegge) 8141498f3ad3ae9c42c32346ee73dc7f29e72cb5 [validation] Cache merkle root and witness commitment checks (dergoegge) 0c5c5962cbfdfd532cebc6706d5b838488b89d53 [test] Add regression test for #27608 (dergoegge) 24736350e932799c66c999470fa3837e25576fc7 [net processing] Don't process mutated blocks (dergoegge) 50c0b61a9d562240d5fe4bd79324b0c0e79caa5c [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge) aff368fa817b065d99729186d304fff02f6e527b [validation] Introduce IsBlockMutated (dergoegge) 076c67c3aae424e58863dde3bc37cedecc496935 [refactor] Cleanup merkle root checks (dergoegge) 97a1d0a45959a29464ae73087c7a0adcdebd5a61 [validation] Isolate merkle root checks (dergoegge) 4ac0eb543d028379bb2b86ab08bbbb2f9f48d5b1 test: Drop `x` modifier in `fsbridge::fopen` call for mingw builds (Hennadii Stepanov) Pull request description: Includes: - #29357 - #29412 - #29524 - #29510 - #29529 Also does: - update to release notes - bump to rc2 - manpages - (no changes to bitcoin.conf) ACKs for top commit: achow101: ACK c68d4d0ac5b8537ba5e1c0c512b807768e1c5c1f Tree-SHA512: 2f8c3dd705e3f9f33403b3cc17e8006510ff827d7dbd609b09732a1669964e9b001cfecdc63d8d8daeb8f39c652e1e4ad0aac873d44d259c21803de85688ed2b
-rw-r--r--configure.ac2
-rw-r--r--doc/man/bitcoin-cli.16
-rw-r--r--doc/man/bitcoin-qt.16
-rw-r--r--doc/man/bitcoin-tx.16
-rw-r--r--doc/man/bitcoin-util.16
-rw-r--r--doc/man/bitcoin-wallet.16
-rw-r--r--doc/man/bitcoind.16
-rw-r--r--doc/release-notes.md10
-rw-r--r--src/net_processing.cpp12
-rw-r--r--src/primitives/block.h8
-rw-r--r--src/test/fuzz/util.cpp2
-rw-r--r--src/test/streams_tests.cpp9
-rw-r--r--src/test/validation_tests.cpp216
-rw-r--r--src/validation.cpp154
-rw-r--r--src/validation.h3
-rw-r--r--src/wallet/wallet.cpp4
-rwxr-xr-xtest/functional/p2p_mutated_blocks.py116
-rwxr-xr-xtest/functional/test_runner.py1
-rwxr-xr-xtest/functional/wallet_keypool.py16
19 files changed, 525 insertions, 64 deletions
diff --git a/configure.ac b/configure.ac
index 472372a6ed..56ea874b6b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2,7 +2,7 @@ AC_PREREQ([2.69])
define(_CLIENT_VERSION_MAJOR, 26)
define(_CLIENT_VERSION_MINOR, 1)
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 a84e8405d9..4f6a81b50e 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.1.
-.TH BITCOIN-CLI "1" "February 2024" "bitcoin-cli v26.1.0rc1" "User Commands"
+.TH BITCOIN-CLI "1" "March 2024" "bitcoin-cli v26.1.0rc2" "User Commands"
.SH NAME
-bitcoin-cli \- manual page for bitcoin-cli v26.1.0rc1
+bitcoin-cli \- manual page for bitcoin-cli v26.1.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 v26.1.0rc1
.B bitcoin-cli
[\fI\,options\/\fR] \fI\,help <command> Get help for a command\/\fR
.SH DESCRIPTION
-Bitcoin Core RPC client version v26.1.0rc1
+Bitcoin Core RPC client version v26.1.0rc2
.SH OPTIONS
.HP
\-?
diff --git a/doc/man/bitcoin-qt.1 b/doc/man/bitcoin-qt.1
index 4e048c0f59..179588f4ce 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.1.
-.TH BITCOIN-QT "1" "February 2024" "bitcoin-qt v26.1.0rc1" "User Commands"
+.TH BITCOIN-QT "1" "March 2024" "bitcoin-qt v26.1.0rc2" "User Commands"
.SH NAME
-bitcoin-qt \- manual page for bitcoin-qt v26.1.0rc1
+bitcoin-qt \- manual page for bitcoin-qt v26.1.0rc2
.SH SYNOPSIS
.B bitcoin-qt
[\fI\,command-line options\/\fR]
.SH DESCRIPTION
-Bitcoin Core version v26.1.0rc1
+Bitcoin Core version v26.1.0rc2
.SH OPTIONS
.HP
\-?
diff --git a/doc/man/bitcoin-tx.1 b/doc/man/bitcoin-tx.1
index 9b27b88a8e..8b06d4b587 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.1.
-.TH BITCOIN-TX "1" "February 2024" "bitcoin-tx v26.1.0rc1" "User Commands"
+.TH BITCOIN-TX "1" "March 2024" "bitcoin-tx v26.1.0rc2" "User Commands"
.SH NAME
-bitcoin-tx \- manual page for bitcoin-tx v26.1.0rc1
+bitcoin-tx \- manual page for bitcoin-tx v26.1.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 v26.1.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 v26.1.0rc1
+Bitcoin Core bitcoin\-tx utility version v26.1.0rc2
.SH OPTIONS
.HP
\-?
diff --git a/doc/man/bitcoin-util.1 b/doc/man/bitcoin-util.1
index 0e74204930..260cb592e8 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.1.
-.TH BITCOIN-UTIL "1" "February 2024" "bitcoin-util v26.1.0rc1" "User Commands"
+.TH BITCOIN-UTIL "1" "March 2024" "bitcoin-util v26.1.0rc2" "User Commands"
.SH NAME
-bitcoin-util \- manual page for bitcoin-util v26.1.0rc1
+bitcoin-util \- manual page for bitcoin-util v26.1.0rc2
.SH SYNOPSIS
.B bitcoin-util
[\fI\,options\/\fR] [\fI\,commands\/\fR] \fI\,Do stuff\/\fR
.SH DESCRIPTION
-Bitcoin Core bitcoin\-util utility version v26.1.0rc1
+Bitcoin Core bitcoin\-util utility version v26.1.0rc2
.SH OPTIONS
.HP
\-?
diff --git a/doc/man/bitcoin-wallet.1 b/doc/man/bitcoin-wallet.1
index f09d68b2c6..266a6305c8 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.1.
-.TH BITCOIN-WALLET "1" "February 2024" "bitcoin-wallet v26.1.0rc1" "User Commands"
+.TH BITCOIN-WALLET "1" "March 2024" "bitcoin-wallet v26.1.0rc2" "User Commands"
.SH NAME
-bitcoin-wallet \- manual page for bitcoin-wallet v26.1.0rc1
+bitcoin-wallet \- manual page for bitcoin-wallet v26.1.0rc2
.SH DESCRIPTION
-Bitcoin Core bitcoin\-wallet version v26.1.0rc1
+Bitcoin Core bitcoin\-wallet version v26.1.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 717d6503d7..d22dde22bf 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.1.
-.TH BITCOIND "1" "February 2024" "bitcoind v26.1.0rc1" "User Commands"
+.TH BITCOIND "1" "March 2024" "bitcoind v26.1.0rc2" "User Commands"
.SH NAME
-bitcoind \- manual page for bitcoind v26.1.0rc1
+bitcoind \- manual page for bitcoind v26.1.0rc2
.SH SYNOPSIS
.B bitcoind
[\fI\,options\/\fR] \fI\,Start Bitcoin Core\/\fR
.SH DESCRIPTION
-Bitcoin Core version v26.1.0rc1
+Bitcoin Core version v26.1.0rc2
.SH OPTIONS
.HP
\-?
diff --git a/doc/release-notes.md b/doc/release-notes.md
index db0d3ddd3f..bd4d3d548f 100644
--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@@ -45,10 +45,12 @@ Notable changes
- #28994 wallet: skip BnB when SFFO is enabled
- #28920 wallet: birth time update during tx scanning
- #29176 wallet: Fix use-after-free in WalletBatch::EraseRecords
+- #29510 wallet: getrawchangeaddress and getnewaddress failures should not affect keypools for descriptor wallets
### RPC
- #29003 rpc: fix getrawtransaction segfault
+- #28784 rpc: keep .cookie file if it was not generated
### Logs
@@ -57,6 +59,8 @@ Notable changes
### P2P and network changes
- #29200 net: create I2P sessions using both ECIES-X25519 and ElGamal encryption
+- #29412 p2p: Don't process mutated blocks
+- #29524 p2p: Don't consider blocks mutated if they don't connect to known prev block
### Build
@@ -73,22 +77,28 @@ Notable changes
- #28391 refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access
- #29179 test: wallet rescan with reorged parent + IsFromMe child in mempool
- #28791 snapshots: don't core dump when running -checkblockindex after loadtxoutset
+- #29357 test: Drop x modifier in fsbridge::fopen call for MinGW builds
+- #29529 fuzz: restrict fopencookie usage to Linux & FreeBSD
Credits
=======
Thanks to everyone who directly contributed to this release:
+- dergoegge
- fanquake
- furszy
- glozow
+- Greg Sanders
- Hennadii Stepanov
- Jon Atack
- MarcoFalke
- Mark Friedenbach
- Martin Zumsande
- Murch
+- Roman Zeyde
- stickies-v
+- UdjinM6
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 3bfb606037..7570c11d3a 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -4689,6 +4689,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;
@@ -4704,7 +4715,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 99accfc7dd..832f8a03f7 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()
{
@@ -95,6 +97,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/fuzz/util.cpp b/src/test/fuzz/util.cpp
index 87ca2f6aed..db73c19735 100644
--- a/src/test/fuzz/util.cpp
+++ b/src/test/fuzz/util.cpp
@@ -255,7 +255,7 @@ FILE* FuzzedFileProvider::open()
[&] {
mode = "a+";
});
-#if defined _GNU_SOURCE && !defined __ANDROID__
+#if defined _GNU_SOURCE && (defined(__linux__) || defined(__FreeBSD__))
const cookie_io_functions_t io_hooks = {
FuzzedFileProvider::read,
FuzzedFileProvider::write,
diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp
index f03f7c1da2..9cca92501b 100644
--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -29,7 +29,14 @@ BOOST_AUTO_TEST_CASE(xor_file)
BOOST_CHECK_EXCEPTION(xor_file.ignore(1), std::ios_base::failure, HasReason{"AutoFile::ignore: file handle is nullpt"});
}
{
- AutoFile xor_file{raw_file("wbx"), xor_pat};
+#ifdef __MINGW64__
+ // Our usage of mingw-w64 and the msvcrt runtime does not support
+ // the x modifier for the _wfopen().
+ const char* mode = "wb";
+#else
+ const char* mode = "wbx";
+#endif
+ AutoFile xor_file{raw_file(mode), xor_pat};
xor_file << test1 << test2;
}
{
diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp
index 14440571eb..fd5ea49ec1 100644
--- a/src/test/validation_tests.cpp
+++ b/src/test/validation_tests.cpp
@@ -4,12 +4,17 @@
#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 <util/chaintype.h>
#include <validation.h>
+#include <string>
+
#include <test/util/setup_common.h>
#include <boost/test/unit_test.hpp>
@@ -145,4 +150,215 @@ BOOST_AUTO_TEST_CASE(test_assumeutxo)
BOOST_CHECK_EQUAL(out110_2.nChainTx, 111U);
}
+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 5d435fef8b..f02d1e32a4 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3621,6 +3621,87 @@ 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 merkle_root = BlockMerkleRoot(block, &mutated);
+ if (block.hashMerkleRoot != merkle_root) {
+ return state.Invalid(
+ /*result=*/BlockValidationResult::BLOCK_MUTATED,
+ /*reject_reason=*/"bad-txnmrklroot",
+ /*debug_message=*/"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(
+ /*result=*/BlockValidationResult::BLOCK_MUTATED,
+ /*reject_reason=*/"bad-txns-duplicate",
+ /*debug_message=*/"duplicate transaction");
+ }
+
+ block.m_checked_merkle_root = true;
+ return true;
+}
+
+/** CheckWitnessMalleation performs checks for block malleation with regard to
+ * its witnesses.
+ *
+ * Note: If the witness commitment is expected (i.e. `expect_witness_commitment
+ * = true`), then the block is required to have at least one transaction and the
+ * first transaction needs to have at least one input. */
+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) {
+ assert(!block.vtx.empty() && !block.vtx[0]->vin.empty());
+ const auto& witness_stack{block.vtx[0]->vin[0].scriptWitness.stack};
+
+ if (witness_stack.size() != 1 || witness_stack[0].size() != 32) {
+ return state.Invalid(
+ /*result=*/BlockValidationResult::BLOCK_MUTATED,
+ /*reject_reason=*/"bad-witness-nonce-size",
+ /*debug_message=*/strprintf("%s : invalid witness reserved value size", __func__));
+ }
+
+ // The malleation check is ignored; as the transaction tree itself
+ // already does not permit it, it is impossible to trigger in the
+ // witness tree.
+ uint256 hash_witness = BlockWitnessMerkleRoot(block, /*mutated=*/nullptr);
+
+ CHash256().Write(hash_witness).Write(witness_stack[0]).Finalize(hash_witness);
+ if (memcmp(hash_witness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) {
+ return state.Invalid(
+ /*result=*/BlockValidationResult::BLOCK_MUTATED,
+ /*reject_reason=*/"bad-witness-merkle-match",
+ /*debug_message=*/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(
+ /*result=*/BlockValidationResult::BLOCK_MUTATED,
+ /*reject_reason=*/"unexpected-witness",
+ /*debug_message=*/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.
@@ -3639,17 +3720,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
@@ -3740,6 +3812,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};
@@ -3848,33 +3951,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 7ce60da634..e9f21528c6 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -360,6 +360,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 78febb8195..31372cb3da 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2561,8 +2561,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 a642a1ee7d..0ddde6c554 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -298,6 +298,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 d2341fb12e..6ed8572347 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)