From 6421b986f6c043fd1266c399e542279ad373822f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sat, 18 Jan 2025 15:37:08 +0100 Subject: [PATCH 1/7] test: validate duplicate detection in `CheckTransaction` The `CheckTransaction` validation function in https://github.com/bitcoin/bitcoin/blob/master/src/consensus/tx_check.cpp#L41-L45 relies on a correct ordering relation for detecting duplicate transaction inputs. This update to the tests ensures that: * Accurate detection of duplicates: Beyond trivial cases (e.g., two identical inputs), duplicates are detected correctly in more complex scenarios. * Consistency across methods: Both sorted sets and hash-based sets behave identically when detecting duplicates for `COutPoint` and related values. * Robust ordering and equality relations: The function maintains expected behavior for ordering and equality checks. Using randomized testing with shuffled inputs (to avoid any remaining bias introduced), the enhanced test validates that `CheckTransaction` remains robust and reliable across various input configurations. It confirms identical behavior to a hashing-based duplicate detection mechanism, ensuring consistency and correctness. To make sure the new branches in the follow-up commits will be covered, `basic_transaction_tests` was extended a randomized test one comparing against the old implementation (and also an alternative duplicate). The iterations and ranges were chosen such that every new branch is expected to be hit once. --- src/test/transaction_tests.cpp | 220 +++++++++++++++++++++++++++++++-- src/test/util/setup_common.cpp | 5 + src/test/util/setup_common.h | 1 + 3 files changed, 217 insertions(+), 9 deletions(-) diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 8beeec49915..0abd8aa0b46 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -406,20 +406,110 @@ BOOST_AUTO_TEST_CASE(tx_oversized) } } -BOOST_AUTO_TEST_CASE(basic_transaction_tests) +static CMutableTransaction CreateTransaction() { - // Random real transaction (e2769b09e784f32f62ef849763d4f45b98e07ba658647343b915ff832b110436) - unsigned char ch[] = {0x01, 0x00, 0x00, 0x00, 0x01, 0x6b, 0xff, 0x7f, 0xcd, 0x4f, 0x85, 0x65, 0xef, 0x40, 0x6d, 0xd5, 0xd6, 0x3d, 0x4f, 0xf9, 0x4f, 0x31, 0x8f, 0xe8, 0x20, 0x27, 0xfd, 0x4d, 0xc4, 0x51, 0xb0, 0x44, 0x74, 0x01, 0x9f, 0x74, 0xb4, 0x00, 0x00, 0x00, 0x00, 0x8c, 0x49, 0x30, 0x46, 0x02, 0x21, 0x00, 0xda, 0x0d, 0xc6, 0xae, 0xce, 0xfe, 0x1e, 0x06, 0xef, 0xdf, 0x05, 0x77, 0x37, 0x57, 0xde, 0xb1, 0x68, 0x82, 0x09, 0x30, 0xe3, 0xb0, 0xd0, 0x3f, 0x46, 0xf5, 0xfc, 0xf1, 0x50, 0xbf, 0x99, 0x0c, 0x02, 0x21, 0x00, 0xd2, 0x5b, 0x5c, 0x87, 0x04, 0x00, 0x76, 0xe4, 0xf2, 0x53, 0xf8, 0x26, 0x2e, 0x76, 0x3e, 0x2d, 0xd5, 0x1e, 0x7f, 0xf0, 0xbe, 0x15, 0x77, 0x27, 0xc4, 0xbc, 0x42, 0x80, 0x7f, 0x17, 0xbd, 0x39, 0x01, 0x41, 0x04, 0xe6, 0xc2, 0x6e, 0xf6, 0x7d, 0xc6, 0x10, 0xd2, 0xcd, 0x19, 0x24, 0x84, 0x78, 0x9a, 0x6c, 0xf9, 0xae, 0xa9, 0x93, 0x0b, 0x94, 0x4b, 0x7e, 0x2d, 0xb5, 0x34, 0x2b, 0x9d, 0x9e, 0x5b, 0x9f, 0xf7, 0x9a, 0xff, 0x9a, 0x2e, 0xe1, 0x97, 0x8d, 0xd7, 0xfd, 0x01, 0xdf, 0xc5, 0x22, 0xee, 0x02, 0x28, 0x3d, 0x3b, 0x06, 0xa9, 0xd0, 0x3a, 0xcf, 0x80, 0x96, 0x96, 0x8d, 0x7d, 0xbb, 0x0f, 0x91, 0x78, 0xff, 0xff, 0xff, 0xff, 0x02, 0x8b, 0xa7, 0x94, 0x0e, 0x00, 0x00, 0x00, 0x00, 0x19, 0x76, 0xa9, 0x14, 0xba, 0xde, 0xec, 0xfd, 0xef, 0x05, 0x07, 0x24, 0x7f, 0xc8, 0xf7, 0x42, 0x41, 0xd7, 0x3b, 0xc0, 0x39, 0x97, 0x2d, 0x7b, 0x88, 0xac, 0x40, 0x94, 0xa8, 0x02, 0x00, 0x00, 0x00, 0x00, 0x19, 0x76, 0xa9, 0x14, 0xc1, 0x09, 0x32, 0x48, 0x3f, 0xec, 0x93, 0xed, 0x51, 0xf5, 0xfe, 0x95, 0xe7, 0x25, 0x59, 0xf2, 0xcc, 0x70, 0x43, 0xf9, 0x88, 0xac, 0x00, 0x00, 0x00, 0x00, 0x00}; - std::vector vch(ch, ch + sizeof(ch) -1); - DataStream stream(vch); + // Serialized random real transaction (e2769b09e784f32f62ef849763d4f45b98e07ba658647343b915ff832b110436) + static constexpr auto ser_tx{"01000000016bff7fcd4f8565ef406dd5d63d4ff94f318fe82027fd4dc451b04474019f74b4000000008c493046022100da0dc6aecefe1e06efdf05773757deb168820930e3b0d03f46f5fcf150bf990c022100d25b5c87040076e4f253f8262e763e2dd51e7ff0be157727c4bc42807f17bd39014104e6c26ef67dc610d2cd192484789a6cf9aea9930b944b7e2db5342b9d9e5b9ff79aff9a2ee1978dd7fd01dfc522ee02283d3b06a9d03acf8096968d7dbb0f9178ffffffff028ba7940e000000001976a914badeecfdef0507247fc8f74241d73bc039972d7b88ac4094a802000000001976a914c10932483fec93ed51f5fe95e72559f2cc7043f988ac0000000000"_hex}; CMutableTransaction tx; - stream >> TX_WITH_WITNESS(tx); + DataStream(ser_tx) >> TX_WITH_WITNESS(tx); + return tx; +} + +BOOST_AUTO_TEST_CASE(transaction_duplicate_input_test) +{ + auto tx{CreateTransaction()}; + TxValidationState state; BOOST_CHECK_MESSAGE(CheckTransaction(CTransaction(tx), state) && state.IsValid(), "Simple deserialized transaction should be valid."); - // Check that duplicate txins fail - tx.vin.push_back(tx.vin[0]); - BOOST_CHECK_MESSAGE(!CheckTransaction(CTransaction(tx), state) || !state.IsValid(), "Transaction with duplicate txins should be invalid."); + // Add duplicate input + tx.vin.emplace_back(tx.vin[0]); + std::ranges::shuffle(tx.vin, m_rng); + BOOST_CHECK_MESSAGE(!CheckTransaction(CTransaction(tx), state) || !state.IsValid(), "Transaction with 2 duplicate txins should be invalid."); + + // ... add a valid input for more complex check + tx.vin.emplace_back(COutPoint(Txid::FromUint256(uint256{1}), 1)); + std::ranges::shuffle(tx.vin, m_rng); + BOOST_CHECK_MESSAGE(!CheckTransaction(CTransaction(tx), state) || !state.IsValid(), "Transaction with 3 inputs (2 valid, 1 duplicate) should be invalid."); +} + +BOOST_AUTO_TEST_CASE(transaction_duplicate_detection_test) +{ + // Randomized testing against hash- and tree-based duplicate check + auto reference_duplicate_check_hash{[](const std::vector& vin) { + std::unordered_set vInOutPoints; + for (const auto& txin : vin) { + if (!vInOutPoints.insert(txin.prevout).second) { + return false; + } + } + return true; + }}; + auto reference_duplicate_check_tree{[](const std::vector& vin) { + std::set vInOutPoints; + for (const auto& txin : vin) { + if (!vInOutPoints.insert(txin.prevout).second) { + return false; + } + } + return true; + }}; + + std::vector hashes; + std::vector ns; + for (int i = 0; i < 10; ++i) { + hashes.emplace_back(Txid::FromUint256(m_rng.rand256())); + ns.emplace_back(m_rng.rand32()); + } + auto tx{CreateTransaction()}; + TxValidationState state; + for (int i{0}; i < 100; ++i) { + if (m_rng.randbool()) { + tx.vin.clear(); + } + for (int j{0}, num_inputs{1 + m_rng.randrange(5)}; j < num_inputs; ++j) { + if (COutPoint outpoint(hashes[m_rng.randrange(hashes.size())], ns[m_rng.randrange(ns.size())]); !outpoint.IsNull()) { + tx.vin.emplace_back(outpoint); + } + } + std::ranges::shuffle(tx.vin, m_rng); + + bool actual{CheckTransaction(CTransaction(tx), state)}; + BOOST_CHECK_EQUAL(actual, reference_duplicate_check_hash(tx.vin)); + BOOST_CHECK_EQUAL(actual, reference_duplicate_check_tree(tx.vin)); + } +} + +BOOST_AUTO_TEST_CASE(transaction_null_prevout_detection_test) +{ + // Randomized testing against linear null prevout check + auto reference_null_prevout_check_hash{[](const std::vector& vin) { + for (const auto& txin : vin) { + if (txin.prevout.IsNull()) { + return false; + } + } + return true; + }}; + + auto tx{CreateTransaction()}; + TxValidationState state; + for (int i{0}; i < 100; ++i) { + if (m_rng.randbool()) { + tx.vin.clear(); + } + for (int j{0}, num_inputs{1 + m_rng.randrange(5)}; j < num_inputs; ++j) { + switch (m_rng.randrange(5)) { + case 0: tx.vin.emplace_back(COutPoint()); break; // Null prevout + case 1: tx.vin.emplace_back(Txid::FromUint256(uint256::ZERO), m_rng.rand32()); break; // Null hash, random index + case 2: tx.vin.emplace_back(Txid::FromUint256(m_rng.rand256()), COutPoint::NULL_INDEX); break; // Random hash, Null index + default: tx.vin.emplace_back(Txid::FromUint256(m_rng.rand256()), m_rng.rand32()); // Random prevout + } + } + std::ranges::shuffle(tx.vin, m_rng); + + BOOST_CHECK_EQUAL(CheckTransaction(CTransaction(tx), state), reference_null_prevout_check_hash(tx.vin)); + } } BOOST_AUTO_TEST_CASE(test_Get) @@ -1048,4 +1138,116 @@ BOOST_AUTO_TEST_CASE(test_IsStandard) CheckIsNotStandard(t, "dust"); } +BOOST_AUTO_TEST_CASE(test_uint256_sorting) +{ + // Sorting + std::vector original{ + uint256{1}, + uint256{2}, + uint256{3} + }; + + std::vector shuffled{original}; + std::ranges::shuffle(shuffled, m_rng); + std::sort(shuffled.begin(), shuffled.end()); + + BOOST_CHECK_EQUAL_COLLECTIONS(original.begin(), original.end(), shuffled.begin(), shuffled.end()); + + // Operators + constexpr auto a{uint256{1}}, + b{uint256{2}}, + c{uint256{3}}; + + BOOST_CHECK(a == a); + BOOST_CHECK(a == uint256{1}); + BOOST_CHECK(b == b); + BOOST_CHECK(c == c); + BOOST_CHECK(a != b); + BOOST_CHECK(a != uint256{10}); + BOOST_CHECK(a != c); + BOOST_CHECK(b != c); + + BOOST_CHECK(a < b); + BOOST_CHECK(a < uint256{10}); + BOOST_CHECK(b < c); + BOOST_CHECK(a < c); +} + +BOOST_AUTO_TEST_CASE(test_transaction_identifier_sorting) +{ + std::vector original{ + Txid::FromUint256(uint256{1}), + Txid::FromUint256(uint256{2}), + Txid::FromUint256(uint256{3}) + }; + + std::vector shuffled{original}; + std::ranges::shuffle(shuffled, m_rng); + std::sort(shuffled.begin(), shuffled.end()); + + BOOST_CHECK_EQUAL_COLLECTIONS(original.begin(), original.end(), shuffled.begin(), shuffled.end()); + + // Operators + const auto a(Txid::FromUint256(uint256{1})), + b(Txid::FromUint256(uint256{2})), + c(Txid::FromUint256(uint256{3})); + + BOOST_CHECK(a == uint256{1}); + + BOOST_CHECK(a == a); + BOOST_CHECK(a == Txid::FromUint256(uint256{1})); + BOOST_CHECK(b == b); + BOOST_CHECK(c == c); + BOOST_CHECK(a != b); + BOOST_CHECK(a != Txid::FromUint256(uint256{10})); + BOOST_CHECK(a != c); + BOOST_CHECK(b != c); + + BOOST_CHECK(a < b); + BOOST_CHECK(a < Txid::FromUint256(uint256{10})); + BOOST_CHECK(b < c); + BOOST_CHECK(a < c); +} + +BOOST_AUTO_TEST_CASE(test_coutpoint_sorting) +{ + // Sorting + std::vector original{ + COutPoint(Txid::FromUint256(uint256{1}), 1), + COutPoint(Txid::FromUint256(uint256{1}), 2), + COutPoint(Txid::FromUint256(uint256{1}), 3), + COutPoint(Txid::FromUint256(uint256{2}), 1), + COutPoint(Txid::FromUint256(uint256{2}), 2), + COutPoint(Txid::FromUint256(uint256{2}), 3), + COutPoint(Txid::FromUint256(uint256{3}), 1), + COutPoint(Txid::FromUint256(uint256{3}), 2), + COutPoint(Txid::FromUint256(uint256{3}), 3) + }; + + std::vector shuffled{original}; + std::ranges::shuffle(shuffled, m_rng); + std::sort(shuffled.begin(), shuffled.end()); + + BOOST_CHECK_EQUAL_COLLECTIONS(original.begin(), original.end(), shuffled.begin(), shuffled.end()); + + // Operators + const auto a{COutPoint(Txid::FromUint256(uint256{1}), 1)}, + b{COutPoint(Txid::FromUint256(uint256{1}), 2)}, + c{COutPoint(Txid::FromUint256(uint256{2}), 1)}; + + BOOST_CHECK(a == a); + BOOST_CHECK(a == COutPoint(Txid::FromUint256(uint256{1}), 1)); + BOOST_CHECK(b == b); + BOOST_CHECK(c == c); + BOOST_CHECK(a != b); + BOOST_CHECK(a != COutPoint(Txid::FromUint256(uint256{1}), 10)); + BOOST_CHECK(a != c); + BOOST_CHECK(b != c); + + BOOST_CHECK(a < b); + BOOST_CHECK(a < COutPoint(Txid::FromUint256(uint256{1}), 10)); + BOOST_CHECK(b < c); + BOOST_CHECK(a < c); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2cf25c4a2ad..f631b61f814 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -615,3 +615,8 @@ std::ostream& operator<<(std::ostream& os, const uint256& num) { return os << num.ToString(); } + +std::ostream& operator<<(std::ostream& os, const COutPoint& outpoint) +{ + return os << outpoint.hash << ", " << outpoint.n; +} diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index 33ad2584573..a12c985bb26 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -285,6 +285,7 @@ inline std::ostream& operator<<(std::ostream& os, const std::optional& v) std::ostream& operator<<(std::ostream& os, const arith_uint256& num); std::ostream& operator<<(std::ostream& os, const uint160& num); std::ostream& operator<<(std::ostream& os, const uint256& num); +std::ostream& operator<<(std::ostream& os, const COutPoint& outpoint); // @} /** From 8f81c3d994742d7ea5e988a300cd3686976ad2a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sat, 18 Jan 2025 11:43:26 +0100 Subject: [PATCH 2/7] bench: measure `CheckBlock` speed separately from serialization > cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc) && build/src/bench/bench_bitcoin -filter='CheckBlockBench|DuplicateInputs' -min-time=10000 > C++ compiler .......................... AppleClang 16.0.0.16000026 | ns/block | block/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 372,743.63 | 2,682.81 | 1.1% | 10.99 | `CheckBlockBench` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 3,304,694.54 | 302.60 | 0.5% | 11.05 | `DuplicateInputs` > C++ compiler .......................... GNU 13.3.0 | ns/block | block/s | err% | ins/block | cyc/block | IPC | bra/block | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 1,096,261.84 | 912.19 | 0.1% | 7,963,390.88 | 3,487,375.26 | 2.283 | 1,266,941.00 | 1.8% | 11.03 | `CheckBlockBench` | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 8,366,309.48 | 119.53 | 0.0% | 23,865,177.67 | 26,620,160.23 | 0.897 | 5,972,887.41 | 4.0% | 10.78 | `DuplicateInputs` --- src/bench/checkblock.cpp | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/src/bench/checkblock.cpp b/src/bench/checkblock.cpp index 9558d64f199..42654535b10 100644 --- a/src/bench/checkblock.cpp +++ b/src/bench/checkblock.cpp @@ -25,7 +25,7 @@ // a block off the wire, but before we can relay the block on to peers using // compact block relay. -static void DeserializeBlockTest(benchmark::Bench& bench) +static void DeserializeBlockBench(benchmark::Bench& bench) { DataStream stream(benchmark::data::block413567); std::byte a{0}; @@ -39,26 +39,18 @@ static void DeserializeBlockTest(benchmark::Bench& bench) }); } -static void DeserializeAndCheckBlockTest(benchmark::Bench& bench) +static void CheckBlockBench(benchmark::Bench& bench) { - DataStream stream(benchmark::data::block413567); - std::byte a{0}; - stream.write({&a, 1}); // Prevent compaction - - ArgsManager bench_args; - const auto chainParams = CreateChainParams(bench_args, ChainType::MAIN); - + CBlock block; + DataStream(benchmark::data::block413567) >> TX_WITH_WITNESS(block); + const auto chainParams = CreateChainParams(ArgsManager{}, ChainType::MAIN); bench.unit("block").run([&] { - CBlock block; // Note that CBlock caches its checked state, so we need to recreate it here - stream >> TX_WITH_WITNESS(block); - bool rewound = stream.Rewind(benchmark::data::block413567.size()); - assert(rewound); - + block.fChecked = block.m_checked_witness_commitment = block.m_checked_merkle_root = false; // Reset the cached state BlockValidationState validationState; - bool checked = CheckBlock(block, validationState, chainParams->GetConsensus()); - assert(checked); + bool checked = CheckBlock(block, validationState, chainParams->GetConsensus(), /*fCheckPOW=*/true, /*fCheckMerkleRoot=*/true); + assert(checked && validationState.IsValid()); }); } -BENCHMARK(DeserializeBlockTest, benchmark::PriorityLevel::HIGH); -BENCHMARK(DeserializeAndCheckBlockTest, benchmark::PriorityLevel::HIGH); +BENCHMARK(DeserializeBlockBench, benchmark::PriorityLevel::HIGH); +BENCHMARK(CheckBlockBench, benchmark::PriorityLevel::HIGH); From 7c4f0d045c532c309f7aca1af3ba51e0795957a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 21 Jan 2025 14:01:31 +0100 Subject: [PATCH 3/7] bench: add `ProcessTransactionBench` to measure `CheckBlock` in context The newly introduced `ProcessTransactionBench` incorporates multiple steps in the validation pipeline, offering a more comprehensive view of `CheckBlock` performance within a realistic transaction validation context. Previous microbenchmarks, such as DeserializeAndCheckBlockTest and DuplicateInputs, focused on isolated aspects of transaction and block validation. While these tests provided valuable insights for targeted profiling, they lacked context regarding the broader validation process, where interactions between components play a critical role. > cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc) && build/src/bench/bench_bitcoin -filter='ProcessTransactionBench' -min-time=10000 > C++ compiler .......................... AppleClang 16.0.0.16000026 | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 9,585.10 | 104,328.55 | 0.1% | 11.03 | `ProcessTransactionBench` > C++ compiler .......................... GNU 13.3.0 | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 56,199.57 | 17,793.73 | 0.1% | 229,263.01 | 178,766.31 | 1.282 | 15,509.97 | 0.5% | 10.91 | `ProcessTransactionBench` --- src/bench/mempool_stress.cpp | 57 ++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/src/bench/mempool_stress.cpp b/src/bench/mempool_stress.cpp index fbac25db5f5..2fe0622d18b 100644 --- a/src/bench/mempool_stress.cpp +++ b/src/bench/mempool_stress.cpp @@ -13,10 +13,19 @@ #include #include #include +#include +#include +#include +#include +#include +#include +#include +#include #include #include #include +#include #include class CCoinsViewCache; @@ -126,5 +135,53 @@ static void MempoolCheck(benchmark::Bench& bench) }); } +static void ProcessTransactionBench(benchmark::Bench& bench) +{ + const auto testing_setup{MakeNoLogFileContext()}; + CTxMemPool& pool{*Assert(testing_setup->m_node.mempool)}; + ChainstateManager& chainman{*testing_setup->m_node.chainman}; + + CBlock block; + DataStream(benchmark::data::block413567) >> TX_WITH_WITNESS(block); + + std::vector txs(block.vtx.size() - 1); + for (size_t i{1}; i < block.vtx.size(); ++i) { + CMutableTransaction mtx{*block.vtx[i]}; + for (auto& txin : mtx.vin) { + txin.nSequence = CTxIn::SEQUENCE_FINAL; + txin.scriptSig.clear(); + txin.scriptWitness.stack = {WITNESS_STACK_ELEM_OP_TRUE}; + } + txs[i - 1] = MakeTransactionRef(std::move(mtx)); + } + + CCoinsViewCache* coins_tip{nullptr}; + size_t cached_coin_count{0}; + { + LOCK(cs_main); + coins_tip = &chainman.ActiveChainstate().CoinsTip(); + for (const auto& tx : txs) { + const Coin coin(CTxOut(2 * tx->GetValueOut(), P2WSH_OP_TRUE), 1, /*fCoinBaseIn=*/false); + for (const auto& in : tx->vin) { + coins_tip->AddCoin(in.prevout, Coin{coin}, /*possible_overwrite=*/false); + cached_coin_count++; + } + } + } + + bench.batch(txs.size()).run([&] { + LOCK2(cs_main, pool.cs); + assert(coins_tip->GetCacheSize() == cached_coin_count); + for (const auto& tx : txs) pool.removeRecursive(*tx, MemPoolRemovalReason::REPLACED); + assert(pool.size() == 0); + + for (const auto& tx : txs) { + const auto res{chainman.ProcessTransaction(tx, /*test_accept=*/true)}; + assert(res.m_result_type == MempoolAcceptResult::ResultType::VALID); + } + }); +} + BENCHMARK(ComplexMemPool, benchmark::PriorityLevel::HIGH); BENCHMARK(MempoolCheck, benchmark::PriorityLevel::HIGH); +BENCHMARK(ProcessTransactionBench, benchmark::PriorityLevel::HIGH); From c161ef8ac40ba843fe3760d68deb06793991d180 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sat, 25 Jan 2025 14:07:24 +0100 Subject: [PATCH 4/7] optimization: move duplicate checks outside of coinbase branch `IsCoinBase` means single input with NULL prevout, so it makes sense to restrict duplicate check to non-coinbase transactions only. The behavior is the same as before, except that single-input-transactions aren't checked for duplicates anymore (~70-90% of the cases, see https://transactionfee.info/charts/transactions-1in). I've added braces to the conditions and loops to simplify review of followup commits. > cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc) && build/src/bench/bench_bitcoin -filter='CheckBlockBench|DuplicateInputs|ProcessTransactionBench' -min-time=10000 > C++ compiler .......................... AppleClang 16.0.0.16000026 | ns/block | block/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 335,917.12 | 2,976.92 | 1.3% | 11.01 | `CheckBlockBench` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 3,286,337.42 | 304.29 | 1.1% | 10.90 | `DuplicateInputs` | 9,561.02 | 104,591.35 | 0.2% | 11.02 | `ProcessTransactionBench` --- src/consensus/tx_check.cpp | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index b3fee1e8b1a..050bca84d47 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -38,22 +38,25 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state) // of a tx as spent, it does not check if the tx has duplicate inputs. // Failure to run this check will result in either a crash or an inflation bug, depending on the implementation of // the underlying coins database. - std::set vInOutPoints; - for (const auto& txin : tx.vin) { - if (!vInOutPoints.insert(txin.prevout).second) - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); - } + if (tx.vin.size() == 1) { + if (tx.IsCoinBase()) { + if (tx.vin[0].scriptSig.size() < 2 || tx.vin[0].scriptSig.size() > 100) { + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cb-length"); + } + } + } else { + std::set vInOutPoints; + for (const auto& txin : tx.vin) { + if (!vInOutPoints.insert(txin.prevout).second) { + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); + } + } - if (tx.IsCoinBase()) - { - if (tx.vin[0].scriptSig.size() < 2 || tx.vin[0].scriptSig.size() > 100) - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cb-length"); - } - else - { - for (const auto& txin : tx.vin) - if (txin.prevout.IsNull()) + for (const auto& txin : tx.vin) { + if (txin.prevout.IsNull()) { return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-prevout-null"); + } + } } return true; From da9cf359a82b0cb95869d52f7fd29ee5ba954241 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Fri, 17 Jan 2025 22:39:59 +0100 Subject: [PATCH 5/7] optimization: simplify duplicate checks for trivial inputs No need to create a set for checking duplicates for two-input-transactions. > cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc) && build/src/bench/bench_bitcoin -filter='CheckBlockBench|DuplicateInputs|ProcessTransactionBench' -min-time=10000 > C++ compiler .......................... AppleClang 16.0.0.16000026 | ns/block | block/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 314,137.30 | 3,183.32 | 1.2% | 11.04 | `CheckBlockBench` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 3,220,592.73 | 310.50 | 1.3% | 10.92 | `DuplicateInputs` | 9,425.98 | 106,089.77 | 0.3% | 11.00 | `ProcessTransactionBench` --- src/consensus/tx_check.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index 050bca84d47..cd465e56d44 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -45,11 +45,17 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state) } } } else { - std::set vInOutPoints; - for (const auto& txin : tx.vin) { - if (!vInOutPoints.insert(txin.prevout).second) { + if (tx.vin.size() == 2) { + if (tx.vin[0].prevout == tx.vin[1].prevout) { return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); } + } else { + std::set vInOutPoints; + for (const auto& txin : tx.vin) { + if (!vInOutPoints.insert(txin.prevout).second) { + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); + } + } } for (const auto& txin : tx.vin) { From cd94cd7de418dfa87a48f7a8f03587c0f437de54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sat, 18 Jan 2025 11:54:47 +0100 Subject: [PATCH 6/7] optimization: replace tree with sorted vector A pre-sized vector retains locality (enabling SIMD operations), speeding up sorting and equality checks. It's also simpler (therefore more reliable) than a sorted set. It also causes less memory fragmentation. > cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc) && build/src/bench/bench_bitcoin -filter='CheckBlockBench|DuplicateInputs|ProcessTransactionBench' -min-time=10000 > C++ compiler .......................... AppleClang 16.0.0.16000026 | ns/block | block/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 181,922.54 | 5,496.85 | 0.2% | 10.98 | `CheckBlockBench` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 997,739.30 | 1,002.27 | 1.0% | 10.94 | `DuplicateInputs` | 9,449.28 | 105,828.15 | 0.3% | 10.99 | `ProcessTransactionBench` Co-authored-by: Pieter Wuille --- src/consensus/tx_check.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index cd465e56d44..9457596fbb9 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -50,11 +50,14 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state) return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); } } else { - std::set vInOutPoints; + std::vector sortedPrevouts; + sortedPrevouts.reserve(tx.vin.size()); for (const auto& txin : tx.vin) { - if (!vInOutPoints.insert(txin.prevout).second) { - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); - } + sortedPrevouts.push_back(txin.prevout); + } + std::sort(sortedPrevouts.begin(), sortedPrevouts.end()); + if (std::ranges::adjacent_find(sortedPrevouts) != sortedPrevouts.end()) { + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); } } From 9a3397456d169fd7e7a9f13ad6b752586829b5d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Sat, 25 Jan 2025 14:24:02 +0100 Subject: [PATCH 7/7] optimization: look for NULL prevouts in the sorted values For the 2 input case we simply check them both, like we did with equality. For the general case, we take advantage of sorting, making invalid value detection constant time instead of linear in the worst case. > cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc) && build/src/bench/bench_bitcoin -filter='CheckBlockBench|DuplicateInputs|ProcessTransactionBench' -min-time=10000 > C++ compiler .......................... AppleClang 16.0.0.16000026 | ns/block | block/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 179,971.00 | 5,556.45 | 0.3% | 11.02 | `CheckBlockBench` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 963,177.98 | 1,038.23 | 1.7% | 10.92 | `DuplicateInputs` | 9,410.90 | 106,259.75 | 0.3% | 11.01 | `ProcessTransactionBench` > C++ compiler .......................... GNU 13.3.0 | ns/block | block/s | err% | ins/block | cyc/block | IPC | bra/block | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 834,855.94 | 1,197.81 | 0.0% | 6,518,548.86 | 2,656,039.78 | 2.454 | 919,160.84 | 1.5% | 10.78 | `CheckBlockBench` | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 4,261,492.75 | 234.66 | 0.0% | 17,379,823.40 | 13,559,793.33 | 1.282 | 4,265,714.28 | 3.4% | 11.00 | `DuplicateInputs` | 55,819.53 | 17,914.88 | 0.1% | 227,828.15 | 177,520.09 | 1.283 | 15,184.36 | 0.4% | 10.91 | `ProcessTransactionBench` --- src/consensus/tx_check.cpp | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index 9457596fbb9..0a471deda24 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -44,25 +44,27 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state) return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cb-length"); } } + } else if (tx.vin.size() == 2) { + if (tx.vin[0].prevout == tx.vin[1].prevout) { + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); + } + if (tx.vin[0].prevout.IsNull() || tx.vin[1].prevout.IsNull()) { + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-prevout-null"); + } } else { - if (tx.vin.size() == 2) { - if (tx.vin[0].prevout == tx.vin[1].prevout) { - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); - } - } else { - std::vector sortedPrevouts; - sortedPrevouts.reserve(tx.vin.size()); - for (const auto& txin : tx.vin) { - sortedPrevouts.push_back(txin.prevout); - } - std::sort(sortedPrevouts.begin(), sortedPrevouts.end()); - if (std::ranges::adjacent_find(sortedPrevouts) != sortedPrevouts.end()) { - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); - } + std::vector sortedPrevouts; + sortedPrevouts.reserve(tx.vin.size()); + for (const auto& txin : tx.vin) { + sortedPrevouts.push_back(txin.prevout); + } + std::sort(sortedPrevouts.begin(), sortedPrevouts.end()); + if (std::ranges::adjacent_find(sortedPrevouts) != sortedPrevouts.end()) { + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); } - for (const auto& txin : tx.vin) { - if (txin.prevout.IsNull()) { + for (const auto& in : sortedPrevouts) { + if (!in.hash.IsNull()) break; // invalid values can only be at the beginning + if (in.IsNull()) { return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-prevout-null"); } }