From be4ff3060b7b43b496dfb5a2c02b114b2b717106 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 7 Jul 2023 10:33:11 +0100 Subject: [PATCH 01/62] Move global `scriptcheckqueue` into `ChainstateManager` class --- src/bitcoin-chainstate.cpp | 2 +- src/init.cpp | 20 +------------------- src/kernel/chainstatemanager_opts.h | 2 ++ src/node/chainstatemanager_args.cpp | 13 +++++++++++++ src/test/util/setup_common.cpp | 6 ++---- src/validation.cpp | 25 ++++++++++++------------- src/validation.h | 12 +++++++----- 7 files changed, 38 insertions(+), 42 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index fc83a4ad3a..1b4a313029 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -290,7 +290,7 @@ int main(int argc, char* argv[]) // dereferencing and UB. scheduler.stop(); if (chainman.m_thread_load.joinable()) chainman.m_thread_load.join(); - StopScriptCheckWorkerThreads(); + chainman.StopScriptCheckWorkerThreads(); GetMainSignals().FlushBackgroundCallbacks(); { diff --git a/src/init.cpp b/src/init.cpp index a0b4425898..b37552d407 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -271,7 +271,7 @@ void Shutdown(NodeContext& node) // CScheduler/checkqueue, scheduler and load block thread. if (node.scheduler) node.scheduler->stop(); if (node.chainman && node.chainman->m_thread_load.joinable()) node.chainman->m_thread_load.join(); - StopScriptCheckWorkerThreads(); + if (node.chainman) node.chainman->StopScriptCheckWorkerThreads(); // After the threads that potentially access these pointers have been stopped, // destruct and reset all to nullptr. @@ -1109,24 +1109,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) return InitError(strprintf(_("Unable to allocate memory for -maxsigcachesize: '%s' MiB"), args.GetIntArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_BYTES >> 20))); } - int script_threads = args.GetIntArg("-par", DEFAULT_SCRIPTCHECK_THREADS); - if (script_threads <= 0) { - // -par=0 means autodetect (number of cores - 1 script threads) - // -par=-n means "leave n cores free" (number of cores - n - 1 script threads) - script_threads += GetNumCores(); - } - - // Subtract 1 because the main thread counts towards the par threads - script_threads = std::max(script_threads - 1, 0); - - // Number of script-checking threads <= MAX_SCRIPTCHECK_THREADS - script_threads = std::min(script_threads, MAX_SCRIPTCHECK_THREADS); - - LogPrintf("Script verification uses %d additional threads\n", script_threads); - if (script_threads >= 1) { - StartScriptCheckWorkerThreads(script_threads); - } - assert(!node.scheduler); node.scheduler = std::make_unique(); diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h index 917f7d226c..ee20eabd79 100644 --- a/src/kernel/chainstatemanager_opts.h +++ b/src/kernel/chainstatemanager_opts.h @@ -45,6 +45,8 @@ struct ChainstateManagerOpts { DBOptions coins_db{}; CoinsViewOptions coins_view{}; Notifications& notifications; + //! Number of script check worker threads. Zero means no parallel verification. + int worker_threads_num{0}; }; } // namespace kernel diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp index 87d9238c18..e61deca3ec 100644 --- a/src/node/chainstatemanager_args.cpp +++ b/src/node/chainstatemanager_args.cpp @@ -6,7 +6,9 @@ #include #include +#include #include +#include #include #include #include @@ -16,6 +18,7 @@ #include #include +#include #include #include @@ -41,6 +44,16 @@ util::Result ApplyArgsManOptions(const ArgsManager& args, ChainstateManage ReadDatabaseArgs(args, opts.coins_db); ReadCoinsViewArgs(args, opts.coins_view); + int script_threads = args.GetIntArg("-par", DEFAULT_SCRIPTCHECK_THREADS); + if (script_threads <= 0) { + // -par=0 means autodetect (number of cores - 1 script threads) + // -par=-n means "leave n cores free" (number of cores - n - 1 script threads) + script_threads += GetNumCores(); + } + // Subtract 1 because the main thread counts towards the par threads. + opts.worker_threads_num = std::clamp(script_threads - 1, 0, MAX_SCRIPTCHECK_THREADS); + LogPrintf("Script verification uses %d additional threads\n", opts.worker_threads_num); + return {}; } } // namespace node diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2947bc3fcb..f14f13fdda 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -176,6 +176,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto .adjusted_time_callback = GetAdjustedTime, .check_block_index = true, .notifications = *m_node.notifications, + .worker_threads_num = 2, }; const BlockManager::Options blockman_opts{ .chainparams = chainman_opts.chainparams, @@ -187,15 +188,12 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto .path = m_args.GetDataDirNet() / "blocks" / "index", .cache_bytes = static_cast(m_cache_sizes.block_tree_db), .memory_only = true}); - - constexpr int script_check_threads = 2; - StartScriptCheckWorkerThreads(script_check_threads); } ChainTestingSetup::~ChainTestingSetup() { if (m_node.scheduler) m_node.scheduler->stop(); - StopScriptCheckWorkerThreads(); + m_node.chainman->StopScriptCheckWorkerThreads(); GetMainSignals().FlushBackgroundCallbacks(); GetMainSignals().UnregisterBackgroundSignalScheduler(); m_node.connman.reset(); diff --git a/src/validation.cpp b/src/validation.cpp index 30b3dde74f..f744126c9f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2047,16 +2047,9 @@ DisconnectResult Chainstate::DisconnectBlock(const CBlock& block, const CBlockIn return fClean ? DISCONNECT_OK : DISCONNECT_UNCLEAN; } -static CCheckQueue scriptcheckqueue(128); - -void StartScriptCheckWorkerThreads(int threads_num) -{ - scriptcheckqueue.StartWorkerThreads(threads_num); -} - -void StopScriptCheckWorkerThreads() +void ChainstateManager::StopScriptCheckWorkerThreads() { - scriptcheckqueue.StopWorkerThreads(); + m_script_check_queue.StopWorkerThreads(); } /** @@ -2147,7 +2140,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, uint256 block_hash{block.GetHash()}; assert(*pindex->phashBlock == block_hash); - const bool parallel_script_checks{scriptcheckqueue.HasThreads()}; + const bool parallel_script_checks{m_chainman.GetCheckQueue().HasThreads()}; const auto time_start{SteadyClock::now()}; const CChainParams& params{m_chainman.GetParams()}; @@ -2336,7 +2329,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // in multiple threads). Preallocate the vector size so a new allocation // doesn't invalidate pointers into the vector, and keep txsdata in scope // for as long as `control`. - CCheckQueueControl control(fScriptChecks && parallel_script_checks ? &scriptcheckqueue : nullptr); + CCheckQueueControl control(fScriptChecks && parallel_script_checks ? &m_chainman.GetCheckQueue() : nullptr); std::vector txsdata(block.vtx.size()); std::vector prevheights; @@ -5751,12 +5744,18 @@ static ChainstateManager::Options&& Flatten(ChainstateManager::Options&& opts) } ChainstateManager::ChainstateManager(const util::SignalInterrupt& interrupt, Options options, node::BlockManager::Options blockman_options) - : m_interrupt{interrupt}, + : m_script_check_queue{/*nBatchSizeIn=*/128}, + m_interrupt{interrupt}, m_options{Flatten(std::move(options))}, - m_blockman{interrupt, std::move(blockman_options)} {} + m_blockman{interrupt, std::move(blockman_options)} +{ + m_script_check_queue.StartWorkerThreads(m_options.worker_threads_num); +} ChainstateManager::~ChainstateManager() { + StopScriptCheckWorkerThreads(); + LOCK(::cs_main); m_versionbitscache.Clear(); diff --git a/src/validation.h b/src/validation.h index 94a00e44a4..5fc4efae14 100644 --- a/src/validation.h +++ b/src/validation.h @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -98,11 +99,6 @@ extern uint256 g_best_block; /** Documentation for argument 'checklevel'. */ extern const std::vector CHECKLEVEL_DOC; -/** Run instances of script checking worker threads */ -void StartScriptCheckWorkerThreads(int threads_num); -/** Stop all of the script checking worker threads */ -void StopScriptCheckWorkerThreads(); - CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams); bool FatalError(kernel::Notifications& notifications, BlockValidationState& state, const std::string& strMessage, const bilingual_str& userMessage = {}); @@ -896,6 +892,9 @@ class ChainstateManager return cs && !cs->m_disabled; } + //! A queue for script verifications that have to be performed by worker threads. + CCheckQueue m_script_check_queue; + public: using Options = kernel::ChainstateManagerOpts; @@ -1246,6 +1245,9 @@ class ChainstateManager //! nullopt. std::optional GetSnapshotBaseHeight() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + CCheckQueue& GetCheckQueue() { return m_script_check_queue; } + void StopScriptCheckWorkerThreads(); + ~ChainstateManager(); }; From d03eaacbcfb276fb638db1b423113ff43bd7ec41 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 7 Jul 2023 10:37:36 +0100 Subject: [PATCH 02/62] Make `CCheckQueue` destructor stop worker threads --- src/bench/checkqueue.cpp | 1 - src/bitcoin-chainstate.cpp | 1 - src/checkqueue.h | 10 +--------- src/init.cpp | 3 +-- src/test/checkqueue_tests.cpp | 6 ------ src/test/transaction_tests.cpp | 1 - src/test/util/setup_common.cpp | 1 - src/validation.cpp | 7 ------- src/validation.h | 1 - 9 files changed, 2 insertions(+), 29 deletions(-) diff --git a/src/bench/checkqueue.cpp b/src/bench/checkqueue.cpp index 70e0b86eba..c7e1ad4f83 100644 --- a/src/bench/checkqueue.cpp +++ b/src/bench/checkqueue.cpp @@ -61,7 +61,6 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench) // it is done explicitly here for clarity control.Wait(); }); - queue.StopWorkerThreads(); ECC_Stop(); } BENCHMARK(CCheckQueueSpeedPrevectorJob, benchmark::PriorityLevel::HIGH); diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 1b4a313029..a8f6fec90a 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -290,7 +290,6 @@ int main(int argc, char* argv[]) // dereferencing and UB. scheduler.stop(); if (chainman.m_thread_load.joinable()) chainman.m_thread_load.join(); - chainman.StopScriptCheckWorkerThreads(); GetMainSignals().FlushBackgroundCallbacks(); { diff --git a/src/checkqueue.h b/src/checkqueue.h index a3299fb3fe..a89be2cfd5 100644 --- a/src/checkqueue.h +++ b/src/checkqueue.h @@ -179,24 +179,16 @@ class CCheckQueue } } - //! Stop all of the worker threads. - void StopWorkerThreads() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + ~CCheckQueue() { WITH_LOCK(m_mutex, m_request_stop = true); m_worker_cv.notify_all(); for (std::thread& t : m_worker_threads) { t.join(); } - m_worker_threads.clear(); - WITH_LOCK(m_mutex, m_request_stop = false); } bool HasThreads() const { return !m_worker_threads.empty(); } - - ~CCheckQueue() - { - assert(m_worker_threads.empty()); - } }; /** diff --git a/src/init.cpp b/src/init.cpp index b37552d407..a60e08a980 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -268,10 +268,9 @@ void Shutdown(NodeContext& node) StopTorControl(); // After everything has been shut down, but before things get flushed, stop the - // CScheduler/checkqueue, scheduler and load block thread. + // scheduler and load block thread. if (node.scheduler) node.scheduler->stop(); if (node.chainman && node.chainman->m_thread_load.joinable()) node.chainman->m_thread_load.join(); - if (node.chainman) node.chainman->StopScriptCheckWorkerThreads(); // After the threads that potentially access these pointers have been stopped, // destruct and reset all to nullptr. diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index cb3831071a..9c2a5d1ae6 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -176,7 +176,6 @@ static void Correct_Queue_range(std::vector range) BOOST_REQUIRE(control.Wait()); BOOST_REQUIRE_EQUAL(FakeCheckCheckCompletion::n_calls, i); } - small_queue->StopWorkerThreads(); } /** Test that 0 checks is correct @@ -240,7 +239,6 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Catches_Failure) BOOST_REQUIRE(success); } } - fail_queue->StopWorkerThreads(); } // Test that a block validation which fails does not interfere with // future blocks, ie, the bad state is cleared. @@ -262,7 +260,6 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Recovers_From_Failure) BOOST_REQUIRE(r != end_fails); } } - fail_queue->StopWorkerThreads(); } // Test that unique checks are actually all called individually, rather than @@ -294,7 +291,6 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_UniqueCheck) } BOOST_REQUIRE(r); } - queue->StopWorkerThreads(); } @@ -325,7 +321,6 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Memory) } BOOST_REQUIRE_EQUAL(MemoryCheck::fake_allocated_memory, 0U); } - queue->StopWorkerThreads(); } // Test that a new verification cannot occur until all checks @@ -361,7 +356,6 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup) // Wait for control to finish t0.join(); BOOST_REQUIRE(!fails); - queue->StopWorkerThreads(); } diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index a4c0db8aea..830cfeb25a 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -553,7 +553,6 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction) bool controlCheck = control.Wait(); assert(controlCheck); - scriptcheckqueue.StopWorkerThreads(); } SignatureData CombineSignatures(const CMutableTransaction& input1, const CMutableTransaction& input2, const CTransactionRef tx) diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index f14f13fdda..0124d85580 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -193,7 +193,6 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto ChainTestingSetup::~ChainTestingSetup() { if (m_node.scheduler) m_node.scheduler->stop(); - m_node.chainman->StopScriptCheckWorkerThreads(); GetMainSignals().FlushBackgroundCallbacks(); GetMainSignals().UnregisterBackgroundSignalScheduler(); m_node.connman.reset(); diff --git a/src/validation.cpp b/src/validation.cpp index f744126c9f..91027872b0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2047,11 +2047,6 @@ DisconnectResult Chainstate::DisconnectBlock(const CBlock& block, const CBlockIn return fClean ? DISCONNECT_OK : DISCONNECT_UNCLEAN; } -void ChainstateManager::StopScriptCheckWorkerThreads() -{ - m_script_check_queue.StopWorkerThreads(); -} - /** * Threshold condition checker that triggers when unknown versionbits are seen on the network. */ @@ -5754,8 +5749,6 @@ ChainstateManager::ChainstateManager(const util::SignalInterrupt& interrupt, Opt ChainstateManager::~ChainstateManager() { - StopScriptCheckWorkerThreads(); - LOCK(::cs_main); m_versionbitscache.Clear(); diff --git a/src/validation.h b/src/validation.h index 5fc4efae14..aacc9b828f 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1246,7 +1246,6 @@ class ChainstateManager std::optional GetSnapshotBaseHeight() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); CCheckQueue& GetCheckQueue() { return m_script_check_queue; } - void StopScriptCheckWorkerThreads(); ~ChainstateManager(); }; From 9cf89f7a5b81197e38f58b24be0793b28fe41477 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 7 Jul 2023 10:40:13 +0100 Subject: [PATCH 03/62] refactor: Make `CCheckQueue` constructor start worker threads --- src/bench/checkqueue.cpp | 5 +++-- src/checkqueue.h | 19 ++++--------------- src/test/checkqueue_tests.cpp | 23 +++++++---------------- src/test/fuzz/checkqueue.cpp | 4 ++-- src/test/transaction_tests.cpp | 4 +--- src/validation.cpp | 3 +-- 6 files changed, 18 insertions(+), 40 deletions(-) diff --git a/src/bench/checkqueue.cpp b/src/bench/checkqueue.cpp index c7e1ad4f83..114dd9d39c 100644 --- a/src/bench/checkqueue.cpp +++ b/src/bench/checkqueue.cpp @@ -37,10 +37,11 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench) return true; } }; - CCheckQueue queue {QUEUE_BATCH_SIZE}; + // The main thread should be counted to prevent thread oversubscription, and // to decrease the variance of benchmark results. - queue.StartWorkerThreads(GetNumCores() - 1); + int worker_threads_num{GetNumCores() - 1}; + CCheckQueue queue{QUEUE_BATCH_SIZE, worker_threads_num}; // create all the data once, then submit copies in the benchmark. FastRandomContext insecure_rand(true); diff --git a/src/checkqueue.h b/src/checkqueue.h index a89be2cfd5..f8b288c10a 100644 --- a/src/checkqueue.h +++ b/src/checkqueue.h @@ -130,22 +130,11 @@ class CCheckQueue Mutex m_control_mutex; //! Create a new check queue - explicit CCheckQueue(unsigned int nBatchSizeIn) - : nBatchSize(nBatchSizeIn) + explicit CCheckQueue(unsigned int batch_size, int worker_threads_num) + : nBatchSize(batch_size) { - } - - //! Create a pool of new worker threads. - void StartWorkerThreads(const int threads_num) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) - { - { - LOCK(m_mutex); - nIdle = 0; - nTotal = 0; - fAllOk = true; - } - assert(m_worker_threads.empty()); - for (int n = 0; n < threads_num; ++n) { + m_worker_threads.reserve(worker_threads_num); + for (int n = 0; n < worker_threads_num; ++n) { m_worker_threads.emplace_back([this, n]() { util::ThreadRename(strprintf("scriptch.%i", n)); Loop(false /* worker thread */); diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index 9c2a5d1ae6..023a5e8e70 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -158,8 +158,7 @@ typedef CCheckQueue FrozenCleanup_Queue; */ static void Correct_Queue_range(std::vector range) { - auto small_queue = std::make_unique(QUEUE_BATCH_SIZE); - small_queue->StartWorkerThreads(SCRIPT_CHECK_THREADS); + auto small_queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); // Make vChecks here to save on malloc (this test can be slow...) std::vector vChecks; vChecks.reserve(9); @@ -217,9 +216,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Correct_Random) /** Test that failing checks are caught */ BOOST_AUTO_TEST_CASE(test_CheckQueue_Catches_Failure) { - auto fail_queue = std::make_unique(QUEUE_BATCH_SIZE); - fail_queue->StartWorkerThreads(SCRIPT_CHECK_THREADS); - + auto fail_queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); for (size_t i = 0; i < 1001; ++i) { CCheckQueueControl control(fail_queue.get()); size_t remaining = i; @@ -244,9 +241,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Catches_Failure) // future blocks, ie, the bad state is cleared. BOOST_AUTO_TEST_CASE(test_CheckQueue_Recovers_From_Failure) { - auto fail_queue = std::make_unique(QUEUE_BATCH_SIZE); - fail_queue->StartWorkerThreads(SCRIPT_CHECK_THREADS); - + auto fail_queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); for (auto times = 0; times < 10; ++times) { for (const bool end_fails : {true, false}) { CCheckQueueControl control(fail_queue.get()); @@ -267,9 +262,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Recovers_From_Failure) // more than once as well BOOST_AUTO_TEST_CASE(test_CheckQueue_UniqueCheck) { - auto queue = std::make_unique(QUEUE_BATCH_SIZE); - queue->StartWorkerThreads(SCRIPT_CHECK_THREADS); - + auto queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); size_t COUNT = 100000; size_t total = COUNT; { @@ -301,8 +294,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_UniqueCheck) // time could leave the data hanging across a sequence of blocks. BOOST_AUTO_TEST_CASE(test_CheckQueue_Memory) { - auto queue = std::make_unique(QUEUE_BATCH_SIZE); - queue->StartWorkerThreads(SCRIPT_CHECK_THREADS); + auto queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); for (size_t i = 0; i < 1000; ++i) { size_t total = i; { @@ -327,9 +319,8 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Memory) // have been destructed BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup) { - auto queue = std::make_unique(QUEUE_BATCH_SIZE); + auto queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); bool fails = false; - queue->StartWorkerThreads(SCRIPT_CHECK_THREADS); std::thread t0([&]() { CCheckQueueControl control(queue.get()); std::vector vChecks(1); @@ -362,7 +353,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup) /** Test that CCheckQueueControl is threadsafe */ BOOST_AUTO_TEST_CASE(test_CheckQueueControl_Locks) { - auto queue = std::make_unique(QUEUE_BATCH_SIZE); + auto queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); { std::vector tg; std::atomic nThreads {0}; diff --git a/src/test/fuzz/checkqueue.cpp b/src/test/fuzz/checkqueue.cpp index 429570526f..6320b500b6 100644 --- a/src/test/fuzz/checkqueue.cpp +++ b/src/test/fuzz/checkqueue.cpp @@ -31,8 +31,8 @@ FUZZ_TARGET(checkqueue) FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); const unsigned int batch_size = fuzzed_data_provider.ConsumeIntegralInRange(0, 1024); - CCheckQueue check_queue_1{batch_size}; - CCheckQueue check_queue_2{batch_size}; + CCheckQueue check_queue_1{batch_size, /*worker_threads_num=*/0}; + CCheckQueue check_queue_2{batch_size, /*worker_threads_num=*/0}; std::vector checks_1; std::vector checks_2; const int size = fuzzed_data_provider.ConsumeIntegralInRange(0, 1024); diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 830cfeb25a..932c40cdec 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -530,11 +530,9 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction) // check all inputs concurrently, with the cache PrecomputedTransactionData txdata(tx); - CCheckQueue scriptcheckqueue(128); + CCheckQueue scriptcheckqueue(/*batch_size=*/128, /*worker_threads_num=*/20); CCheckQueueControl control(&scriptcheckqueue); - scriptcheckqueue.StartWorkerThreads(20); - std::vector coins; for(uint32_t i = 0; i < mtx.vin.size(); i++) { Coin coin; diff --git a/src/validation.cpp b/src/validation.cpp index 91027872b0..ba88ef2ec5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5739,12 +5739,11 @@ static ChainstateManager::Options&& Flatten(ChainstateManager::Options&& opts) } ChainstateManager::ChainstateManager(const util::SignalInterrupt& interrupt, Options options, node::BlockManager::Options blockman_options) - : m_script_check_queue{/*nBatchSizeIn=*/128}, + : m_script_check_queue{/*batch_size=*/128, options.worker_threads_num}, m_interrupt{interrupt}, m_options{Flatten(std::move(options))}, m_blockman{interrupt, std::move(blockman_options)} { - m_script_check_queue.StartWorkerThreads(m_options.worker_threads_num); } ChainstateManager::~ChainstateManager() From 8111e74653dc5c93cb510672d99048c3f741d8dc Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 7 Jul 2023 10:42:49 +0100 Subject: [PATCH 04/62] refactor: Drop unneeded declaration --- src/checkqueue.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/checkqueue.h b/src/checkqueue.h index f8b288c10a..f3c126e262 100644 --- a/src/checkqueue.h +++ b/src/checkqueue.h @@ -13,9 +13,6 @@ #include #include -template -class CCheckQueueControl; - /** * Queue for verifications that have to be performed. * The verifications are represented by a type T, which must provide an From 6e17b3168072ab77ed7170ab81327c017877133a Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 7 Jul 2023 10:43:09 +0100 Subject: [PATCH 05/62] refactor: Make `CCheckQueue` non-copyable and non-movable explicitly --- src/checkqueue.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/checkqueue.h b/src/checkqueue.h index f3c126e262..a1de000714 100644 --- a/src/checkqueue.h +++ b/src/checkqueue.h @@ -139,6 +139,13 @@ class CCheckQueue } } + // Since this class manages its own resources, which is a thread + // pool `m_worker_threads`, copy and move operations are not appropriate. + CCheckQueue(const CCheckQueue&) = delete; + CCheckQueue& operator=(const CCheckQueue&) = delete; + CCheckQueue(CCheckQueue&&) = delete; + CCheckQueue& operator=(CCheckQueue&&) = delete; + //! Wait until execution finishes, and return whether all evaluations were successful. bool Wait() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) { From 5b3ea5fa2e7f6dc1c9161ed8b74c9be4bd1e92dd Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 7 Jul 2023 10:43:24 +0100 Subject: [PATCH 06/62] refactor: Move `{MAX,DEFAULT}_SCRIPTCHECK_THREADS` constants --- src/node/chainstatemanager_args.cpp | 1 - src/node/chainstatemanager_args.h | 5 +++++ src/qt/optionsdialog.cpp | 2 +- src/qt/optionsmodel.cpp | 1 + src/validation.h | 4 ---- 5 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp index e61deca3ec..1cc126cb05 100644 --- a/src/node/chainstatemanager_args.cpp +++ b/src/node/chainstatemanager_args.cpp @@ -7,7 +7,6 @@ #include #include #include -#include #include #include #include diff --git a/src/node/chainstatemanager_args.h b/src/node/chainstatemanager_args.h index 701515953e..b2cdba68b8 100644 --- a/src/node/chainstatemanager_args.h +++ b/src/node/chainstatemanager_args.h @@ -10,6 +10,11 @@ class ArgsManager; +/** Maximum number of dedicated script-checking threads allowed */ +static constexpr int MAX_SCRIPTCHECK_THREADS{15}; +/** -par default (number of script-checking threads, 0 = auto) */ +static constexpr int DEFAULT_SCRIPTCHECK_THREADS{0}; + namespace node { [[nodiscard]] util::Result ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts); } // namespace node diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp index 512fce473d..6e1d36effb 100644 --- a/src/qt/optionsdialog.cpp +++ b/src/qt/optionsdialog.cpp @@ -17,9 +17,9 @@ #include #include +#include #include #include -#include #include diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index c1563fe1e2..43564dad16 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include // for -dbcache defaults #include #include // For DEFAULT_SCRIPTCHECK_THREADS diff --git a/src/validation.h b/src/validation.h index aacc9b828f..1165511ead 100644 --- a/src/validation.h +++ b/src/validation.h @@ -66,10 +66,6 @@ namespace util { class SignalInterrupt; } // namespace util -/** Maximum number of dedicated script-checking threads allowed */ -static const int MAX_SCRIPTCHECK_THREADS = 15; -/** -par default (number of script-checking threads, 0 = auto) */ -static const int DEFAULT_SCRIPTCHECK_THREADS = 0; /** Block files containing a block-height within MIN_BLOCKS_TO_KEEP of ActiveChain().Tip() will not be pruned. */ static const unsigned int MIN_BLOCKS_TO_KEEP = 288; static const signed int DEFAULT_CHECKBLOCKS = 6; From faa3d4f1d8ecff444be53215d72e32d71d9ce138 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 4 Oct 2023 11:06:24 +0200 Subject: [PATCH 07/62] Remove duplicate NDEBUG check from compat/assumptions.h The check is already done in util/check.h, which is more widely included. --- src/compat/assumptions.h | 7 ------- src/net_processing.cpp | 4 ++-- src/validation.cpp | 2 +- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/compat/assumptions.h b/src/compat/assumptions.h index 4488db0886..78bf3c743f 100644 --- a/src/compat/assumptions.h +++ b/src/compat/assumptions.h @@ -11,13 +11,6 @@ #include #include -// Assumption: We assume that the macro NDEBUG is not defined. -// Example(s): We use assert(...) extensively with the assumption of it never -// being a noop at runtime. -#if defined(NDEBUG) -# error "Bitcoin cannot be compiled without assertions." -#endif - // Assumption: We assume a C++17 (ISO/IEC 14882:2017) compiler (minimum requirement). // Example(s): We assume the presence of C++17 features everywhere :-) // ISO Standard C++17 [cpp.predefined]p1: diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 06086d6804..686da98739 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -16,9 +16,9 @@ #include #include #include +#include #include #include -#include #include #include #include @@ -39,7 +39,7 @@ #include #include #include -#include // For NDEBUG compile time check +#include #include #include #include diff --git a/src/validation.cpp b/src/validation.cpp index 30b3dde74f..55f2514202 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -47,7 +47,7 @@ #include #include #include -#include // For NDEBUG compile time check +#include #include #include #include From 77774110f4dd591a71441851813d59c03c9e3c78 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 4 Oct 2023 11:12:09 +0200 Subject: [PATCH 08/62] Remove __cplusplus from compat/assumptions.h It is unclear what the goal of this check is, given that the value may need to be set lower for the mimimum supported version of compilers that forgot to bump the value, see https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1745143612 . The minimum supported compiler versions are already documented in doc/dependencies.md and using an older compiler will already result in a compile failure, so this check can be removed as redundant. Especially given that it is only included in one file, where iwyu suggests to remove it. --- src/compat/assumptions.h | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/compat/assumptions.h b/src/compat/assumptions.h index 78bf3c743f..7b66ab1b15 100644 --- a/src/compat/assumptions.h +++ b/src/compat/assumptions.h @@ -11,13 +11,6 @@ #include #include -// Assumption: We assume a C++17 (ISO/IEC 14882:2017) compiler (minimum requirement). -// Example(s): We assume the presence of C++17 features everywhere :-) -// ISO Standard C++17 [cpp.predefined]p1: -// "The name __cplusplus is defined to the value 201703L when compiling a C++ -// translation unit." -static_assert(__cplusplus >= 201703L, "C++17 standard assumed"); - // Assumption: We assume the floating-point types to fulfill the requirements of // IEC 559 (IEEE 754) standard. // Example(s): Floating-point division by zero in ConnectBlock, CreateTransaction From 88887531b704f3943fdb33abbdd5378ecfeee14f Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 4 Oct 2023 11:29:23 +0200 Subject: [PATCH 09/62] Move compat/assumptions.h include to one place that actually needs it Also add the to avoid removing it by accident. --- src/common/system.h | 1 - src/serialize.h | 7 +++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/common/system.h b/src/common/system.h index 40206aaa01..4b6aa42150 100644 --- a/src/common/system.h +++ b/src/common/system.h @@ -10,7 +10,6 @@ #include #endif -#include #include #include diff --git a/src/serialize.h b/src/serialize.h index e53ff9fa4c..dbec620c64 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -7,7 +7,10 @@ #define BITCOIN_SERIALIZE_H #include +#include // IWYU pragma: keep #include +#include +#include #include #include @@ -18,13 +21,9 @@ #include #include #include -#include #include #include -#include -#include - /** * The maximum size of a serialized object in bytes or number of elements * (for eg vectors) when the size is encoded as CompactSize. From fa1a38470697796a1a67397a815c8f8256f59224 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 16 Oct 2023 14:18:28 +0200 Subject: [PATCH 10/62] Move compat.h include from system.h to system.cpp --- src/common/system.cpp | 3 ++- src/common/system.h | 7 ++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/common/system.cpp b/src/common/system.cpp index 1d1c5fa56a..ba42c6df50 100644 --- a/src/common/system.cpp +++ b/src/common/system.cpp @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2022 The Bitcoin Core developers +// Copyright (c) 2009-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -12,6 +12,7 @@ #ifndef WIN32 #include #else +#include #include #endif diff --git a/src/common/system.h b/src/common/system.h index 4b6aa42150..e8018f9b10 100644 --- a/src/common/system.h +++ b/src/common/system.h @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2022 The Bitcoin Core developers +// Copyright (c) 2009-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -10,10 +10,7 @@ #include #endif -#include - -#include -#include +#include #include // Application startup time (used for uptime calculation) From af1d2ff88344e1545ac8d9ad09f8e37e264da712 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Wed, 1 Nov 2023 14:04:44 +0000 Subject: [PATCH 11/62] [primitives] Precompute result of CTransaction::HasWitness --- src/primitives/transaction.cpp | 12 ++++++++++-- src/primitives/transaction.h | 13 ++++--------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index 1ad8345fcb..77a363f7b6 100644 --- a/src/primitives/transaction.cpp +++ b/src/primitives/transaction.cpp @@ -15,6 +15,7 @@ #include #include +#include #include #include @@ -71,6 +72,13 @@ Txid CMutableTransaction::GetHash() const return Txid::FromUint256((CHashWriter{SERIALIZE_TRANSACTION_NO_WITNESS} << *this).GetHash()); } +bool CTransaction::ComputeHasWitness() const +{ + return std::any_of(vin.begin(), vin.end(), [](const auto& input) { + return !input.scriptWitness.IsNull(); + }); +} + Txid CTransaction::ComputeHash() const { return Txid::FromUint256((CHashWriter{SERIALIZE_TRANSACTION_NO_WITNESS} << *this).GetHash()); @@ -85,8 +93,8 @@ Wtxid CTransaction::ComputeWitnessHash() const return Wtxid::FromUint256((CHashWriter{0} << *this).GetHash()); } -CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime), hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {} -CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), nVersion(tx.nVersion), nLockTime(tx.nLockTime), hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {} +CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime), m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {} +CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), nVersion(tx.nVersion), nLockTime(tx.nLockTime), m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {} CAmount CTransaction::GetValueOut() const { diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index 89deb9de4d..594168bbcc 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -310,12 +310,15 @@ class CTransaction private: /** Memory only. */ + const bool m_has_witness; const Txid hash; const Wtxid m_witness_hash; Txid ComputeHash() const; Wtxid ComputeWitnessHash() const; + bool ComputeHasWitness() const; + public: /** Convert a CMutableTransaction into a CTransaction. */ explicit CTransaction(const CMutableTransaction& tx); @@ -365,15 +368,7 @@ class CTransaction std::string ToString() const; - bool HasWitness() const - { - for (size_t i = 0; i < vin.size(); i++) { - if (!vin[i].scriptWitness.IsNull()) { - return true; - } - } - return false; - } + bool HasWitness() const { return m_has_witness; } }; /** A mutable version of CTransaction. */ From 7cb9367157eb42ee06bc6fa024522cc14a80138d Mon Sep 17 00:00:00 2001 From: Roman Zeyde Date: Fri, 3 Nov 2023 16:34:42 +0200 Subject: [PATCH 12/62] rpc: keep .cookie if it was not generated Otherwise, starting bitcoind twice may cause the `.cookie` file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock). --- src/rpc/request.cpp | 8 +++++++- test/functional/feature_filelock.py | 3 +++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp index 4c67da8b70..b7acd62ee3 100644 --- a/src/rpc/request.cpp +++ b/src/rpc/request.cpp @@ -80,6 +80,8 @@ static fs::path GetAuthCookieFile(bool temp=false) return AbsPathForConfigVal(gArgs, arg); } +static bool g_generated_cookie = false; + bool GenerateAuthCookie(std::string *cookie_out) { const size_t COOKIE_SIZE = 32; @@ -105,6 +107,7 @@ bool GenerateAuthCookie(std::string *cookie_out) LogPrintf("Unable to rename cookie authentication file %s to %s\n", fs::PathToString(filepath_tmp), fs::PathToString(filepath)); return false; } + g_generated_cookie = true; LogPrintf("Generated RPC authentication cookie %s\n", fs::PathToString(filepath)); if (cookie_out) @@ -131,7 +134,10 @@ bool GetAuthCookie(std::string *cookie_out) void DeleteAuthCookie() { try { - fs::remove(GetAuthCookieFile()); + if (g_generated_cookie) { + // Delete the cookie file if it was generated by this process + fs::remove(GetAuthCookieFile()); + } } catch (const fs::filesystem_error& e) { LogPrintf("%s: Unable to remove random auth cookie file: %s\n", __func__, fsbridge::get_filesystem_error_message(e)); } diff --git a/test/functional/feature_filelock.py b/test/functional/feature_filelock.py index 24a68a04bf..0f4be54d0e 100755 --- a/test/functional/feature_filelock.py +++ b/test/functional/feature_filelock.py @@ -30,6 +30,9 @@ def run_test(self): expected_msg = f"Error: Cannot obtain a lock on data directory {datadir}. {self.config['environment']['PACKAGE_NAME']} is probably already running." self.nodes[1].assert_start_raises_init_error(extra_args=[f'-datadir={self.nodes[0].datadir_path}', '-noserver'], expected_msg=expected_msg) + cookie_file = datadir / ".cookie" + assert cookie_file.exists() # should not be deleted during the second bitcoind instance shutdown + if self.is_wallet_compiled(): def check_wallet_filelock(descriptors): wallet_name = ''.join([random.choice(string.ascii_lowercase) for _ in range(6)]) From 9ac114e5cd9d8ade3a1d9f3d76a08ff59a3f1658 Mon Sep 17 00:00:00 2001 From: Jameson Lopp Date: Sat, 30 Sep 2023 15:06:36 -0400 Subject: [PATCH 13/62] Throw error if invalid parameters passed to getnetworkhashps RPC endpoint --- src/rpc/mining.cpp | 19 ++++++++++++----- test/functional/rpc_blockchain.py | 34 ++++++++++++++++++++++++++++--- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 76170c3201..a1894a3030 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -49,13 +49,22 @@ using node::UpdateTime; /** * Return average network hashes per second based on the last 'lookup' blocks, - * or from the last difficulty change if 'lookup' is nonpositive. - * If 'height' is nonnegative, compute the estimate at the time when a given block was found. + * or from the last difficulty change if 'lookup' is -1. + * If 'height' is -1, compute the estimate from current chain tip. + * If 'height' is a valid block height, compute the estimate at the time when a given block was found. */ static UniValue GetNetworkHashPS(int lookup, int height, const CChain& active_chain) { + if (lookup < -1 || lookup == 0) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid nblocks. Must be a positive number or -1."); + } + + if (height < -1 || height > active_chain.Height()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Block does not exist at specified height"); + } + const CBlockIndex* pb = active_chain.Tip(); - if (height >= 0 && height < active_chain.Height()) { + if (height >= 0) { pb = active_chain[height]; } @@ -63,7 +72,7 @@ static UniValue GetNetworkHashPS(int lookup, int height, const CChain& active_ch return 0; // If lookup is -1, then use blocks since last difficulty change. - if (lookup <= 0) + if (lookup == -1) lookup = pb->nHeight % Params().GetConsensus().DifficultyAdjustmentInterval() + 1; // If lookup is larger than chain, then set it to chain length. @@ -97,7 +106,7 @@ static RPCHelpMan getnetworkhashps() "Pass in [blocks] to override # of blocks, -1 specifies since last difficulty change.\n" "Pass in [height] to estimate the network speed at the time when a certain block was found.\n", { - {"nblocks", RPCArg::Type::NUM, RPCArg::Default{120}, "The number of blocks, or -1 for blocks since last difficulty change."}, + {"nblocks", RPCArg::Type::NUM, RPCArg::Default{120}, "The number of previous blocks to calculate estimate from, or -1 for blocks since last difficulty change."}, {"height", RPCArg::Type::NUM, RPCArg::Default{-1}, "To estimate at the time of the given height."}, }, RPCResult{ diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index 8eb9f3aeb1..9b7743cafa 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -437,7 +437,6 @@ def _test_getdifficulty(self): def _test_getnetworkhashps(self): self.log.info("Test getnetworkhashps") - hashes_per_second = self.nodes[0].getnetworkhashps() assert_raises_rpc_error( -3, textwrap.dedent(""" @@ -449,17 +448,46 @@ def _test_getnetworkhashps(self): """).strip(), lambda: self.nodes[0].getnetworkhashps("a", []), ) + assert_raises_rpc_error( + -8, + "Block does not exist at specified height", + lambda: self.nodes[0].getnetworkhashps(100, self.nodes[0].getblockcount() + 1), + ) + assert_raises_rpc_error( + -8, + "Block does not exist at specified height", + lambda: self.nodes[0].getnetworkhashps(100, -10), + ) + assert_raises_rpc_error( + -8, + "Invalid nblocks. Must be a positive number or -1.", + lambda: self.nodes[0].getnetworkhashps(-100), + ) + assert_raises_rpc_error( + -8, + "Invalid nblocks. Must be a positive number or -1.", + lambda: self.nodes[0].getnetworkhashps(0), + ) + + # Genesis block height estimate should return 0 + hashes_per_second = self.nodes[0].getnetworkhashps(100, 0) + assert_equal(hashes_per_second, 0) + # This should be 2 hashes every 10 minutes or 1/300 + hashes_per_second = self.nodes[0].getnetworkhashps() assert abs(hashes_per_second * 300 - 1) < 0.0001 - # Test setting the first param of getnetworkhashps to negative value returns the average network + # Test setting the first param of getnetworkhashps to -1 returns the average network # hashes per second from the last difficulty change. current_block_height = self.nodes[0].getmininginfo()['blocks'] blocks_since_last_diff_change = current_block_height % DIFFICULTY_ADJUSTMENT_INTERVAL + 1 expected_hashes_per_second_since_diff_change = self.nodes[0].getnetworkhashps(blocks_since_last_diff_change) assert_equal(self.nodes[0].getnetworkhashps(-1), expected_hashes_per_second_since_diff_change) - assert_equal(self.nodes[0].getnetworkhashps(-2), expected_hashes_per_second_since_diff_change) + + # Ensure long lookups get truncated to chain length + hashes_per_second = self.nodes[0].getnetworkhashps(self.nodes[0].getblockcount() + 1000) + assert hashes_per_second > 0.003 def _test_stopatheight(self): self.log.info("Test stopping at height") From 68a90017519874793e34e3b439a63e5aa3a6f6a7 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Fri, 3 Nov 2023 16:54:19 -0400 Subject: [PATCH 14/62] test: persist -v2transport over restarts and respect -v2transport=0 Before, a global -v2transport provided to the test would be dropped when restarting the node within a test and specifying any extra_args. Fix this by adding "v2transport=1" to args (not extra_args) based on the global parameter, and deciding for each (re)start of the node based on this default and test-specific extra_args (which take precedence over args) whether v2 should be used. --- test/functional/test_framework/test_framework.py | 5 ++--- test/functional/test_framework/test_node.py | 10 +++++++++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 70b3943478..4660d2fcde 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -508,8 +508,6 @@ def get_bin_from_version(version, bin_name, bin_default): assert_equal(len(binary_cli), num_nodes) for i in range(num_nodes): args = list(extra_args[i]) - if self.options.v2transport and ("-v2transport=0" not in args): - args.append("-v2transport=1") test_node_i = TestNode( i, get_datadir_path(self.options.tmpdir, i), @@ -528,6 +526,7 @@ def get_bin_from_version(version, bin_name, bin_default): start_perf=self.options.perf, use_valgrind=self.options.valgrind, descriptors=self.options.descriptors, + v2transport=self.options.v2transport, ) self.nodes.append(test_node_i) if not test_node_i.version_is_at_least(170000): @@ -602,7 +601,7 @@ def connect_nodes(self, a, b, *, peer_advertises_v2=None, wait_for_connect: bool ip_port = "127.0.0.1:" + str(p2p_port(b)) if peer_advertises_v2 is None: - peer_advertises_v2 = self.options.v2transport + peer_advertises_v2 = from_connection.use_v2transport if peer_advertises_v2: from_connection.addnode(node=ip_port, command="onetry", v2transport=True) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index b6af71d85c..435140cbeb 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -67,7 +67,7 @@ class TestNode(): To make things easier for the test writer, any unrecognised messages will be dispatched to the RPC connection.""" - def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor, bitcoind, bitcoin_cli, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False, use_valgrind=False, version=None, descriptors=False): + def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor, bitcoind, bitcoin_cli, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False, use_valgrind=False, version=None, descriptors=False, v2transport=False): """ Kwargs: start_perf (bool): If True, begin profiling the node with `perf` as soon as @@ -126,6 +126,12 @@ def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor, if self.version_is_at_least(239000): self.args.append("-loglevel=trace") + # Default behavior from global -v2transport flag is added to args to persist it over restarts. + # May be overwritten in individual tests, using extra_args. + self.default_to_v2 = v2transport + if self.default_to_v2: + self.args.append("-v2transport=1") + self.cli = TestNodeCLI(bitcoin_cli, self.datadir_path) self.use_cli = use_cli self.start_perf = start_perf @@ -198,6 +204,8 @@ def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, env=None if extra_args is None: extra_args = self.extra_args + self.use_v2transport = "-v2transport=1" in extra_args or (self.default_to_v2 and "-v2transport=0" not in extra_args) + # Add a new stdout and stderr file each time bitcoind is started if stderr is None: stderr = tempfile.NamedTemporaryFile(dir=self.stderr_dir, delete=False) From 3598a1b5c932634dc7ccb991cc83df5e1a1dcaa9 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 6 Nov 2023 16:03:47 -0500 Subject: [PATCH 15/62] test: enable --v2transport in combination with --usecli By renaming the "command" send_cli arg. The old name was unsuitable because the "addnode" RPC has its own "command" arg, leading to ambiguity when included in kwargs. Can be tested with "python3 wallet_multiwallet.py --usecli --v2transport" which fails on master because of this (python throws a TypeError). --- test/functional/test_framework/test_node.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 435140cbeb..c23119c6cb 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -787,15 +787,15 @@ def batch(self, requests): results.append(dict(error=e)) return results - def send_cli(self, command=None, *args, **kwargs): + def send_cli(self, clicommand=None, *args, **kwargs): """Run bitcoin-cli command. Deserializes returned string as python object.""" pos_args = [arg_to_cli(arg) for arg in args] named_args = [str(key) + "=" + arg_to_cli(value) for (key, value) in kwargs.items()] p_args = [self.binary, f"-datadir={self.datadir}"] + self.options if named_args: p_args += ["-named"] - if command is not None: - p_args += [command] + if clicommand is not None: + p_args += [clicommand] p_args += pos_args + named_args self.log.debug("Running bitcoin-cli {}".format(p_args[2:])) process = subprocess.Popen(p_args, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) From cc961c26956859850202f56191981b0306a65fcf Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Fri, 15 Sep 2023 02:18:38 +0200 Subject: [PATCH 16/62] test: enable v2 transport for p2p_node_network_limited.py --- test/functional/p2p_node_network_limited.py | 12 +++++++++++- test/functional/test_runner.py | 1 + 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/test/functional/p2p_node_network_limited.py b/test/functional/p2p_node_network_limited.py index a56afbcf7b..89c35e943b 100755 --- a/test/functional/p2p_node_network_limited.py +++ b/test/functional/p2p_node_network_limited.py @@ -8,7 +8,15 @@ and that it responds to getdata requests for blocks correctly: - send a block within 288 + 2 of the tip - disconnect peers who request blocks older than that.""" -from test_framework.messages import CInv, MSG_BLOCK, msg_getdata, msg_verack, NODE_NETWORK_LIMITED, NODE_WITNESS +from test_framework.messages import ( + CInv, + MSG_BLOCK, + NODE_NETWORK_LIMITED, + NODE_P2P_V2, + NODE_WITNESS, + msg_getdata, + msg_verack, +) from test_framework.p2p import P2PInterface from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -50,6 +58,8 @@ def run_test(self): node = self.nodes[0].add_p2p_connection(P2PIgnoreInv()) expected_services = NODE_WITNESS | NODE_NETWORK_LIMITED + if self.options.v2transport: + expected_services |= NODE_P2P_V2 self.log.info("Check that node has signalled expected services.") assert_equal(node.nServices, expected_services) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 341a31909b..cbc03a3e3a 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -365,6 +365,7 @@ 'wallet_orphanedreward.py', 'wallet_timelock.py', 'p2p_node_network_limited.py', + 'p2p_node_network_limited.py --v2transport', 'p2p_permissions.py', 'feature_blocksdir.py', 'wallet_startup.py', From 2c1669c37a9759e15ff5f4e340aeaa8778a81b9a Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Fri, 15 Sep 2023 03:20:07 +0200 Subject: [PATCH 17/62] test: enable v2 transport for rpc_net.py - "transport_protocol_type" of inbound peer before version handshake is "detecting" on p2p v2 nodes (as opposed to "v1" for p2p v1) - size of a ping/pong message is 29 bytes (as opposed to 32 for p2p v1) - for the sendmsgtopeer RPC sub-test, enforce p2p v1 connection to have a peer id of zero --- test/functional/rpc_net.py | 23 +++++++++++++++-------- test/functional/test_runner.py | 1 + 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index 50a022fc7e..a74a8cac2f 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -150,7 +150,7 @@ def test_getpeerinfo(self): "synced_blocks": -1, "synced_headers": -1, "timeoffset": 0, - "transport_protocol_type": "v1", + "transport_protocol_type": "v1" if not self.options.v2transport else "detecting", "version": 0, }, ) @@ -160,19 +160,23 @@ def test_getpeerinfo(self): def test_getnettotals(self): self.log.info("Test getnettotals") # Test getnettotals and getpeerinfo by doing a ping. The bytes - # sent/received should increase by at least the size of one ping (32 - # bytes) and one pong (32 bytes). + # sent/received should increase by at least the size of one ping + # and one pong. Both have a payload size of 8 bytes, but the total + # size depends on the used p2p version: + # - p2p v1: 24 bytes (header) + 8 bytes (payload) = 32 bytes + # - p2p v2: 21 bytes (header/tag with short-id) + 8 bytes (payload) = 29 bytes + ping_size = 32 if not self.options.v2transport else 29 net_totals_before = self.nodes[0].getnettotals() peer_info_before = self.nodes[0].getpeerinfo() self.nodes[0].ping() - self.wait_until(lambda: (self.nodes[0].getnettotals()['totalbytessent'] >= net_totals_before['totalbytessent'] + 32 * 2), timeout=1) - self.wait_until(lambda: (self.nodes[0].getnettotals()['totalbytesrecv'] >= net_totals_before['totalbytesrecv'] + 32 * 2), timeout=1) + self.wait_until(lambda: (self.nodes[0].getnettotals()['totalbytessent'] >= net_totals_before['totalbytessent'] + ping_size * 2), timeout=1) + self.wait_until(lambda: (self.nodes[0].getnettotals()['totalbytesrecv'] >= net_totals_before['totalbytesrecv'] + ping_size * 2), timeout=1) for peer_before in peer_info_before: peer_after = lambda: next(p for p in self.nodes[0].getpeerinfo() if p['id'] == peer_before['id']) - self.wait_until(lambda: peer_after()['bytesrecv_per_msg'].get('pong', 0) >= peer_before['bytesrecv_per_msg'].get('pong', 0) + 32, timeout=1) - self.wait_until(lambda: peer_after()['bytessent_per_msg'].get('ping', 0) >= peer_before['bytessent_per_msg'].get('ping', 0) + 32, timeout=1) + self.wait_until(lambda: peer_after()['bytesrecv_per_msg'].get('pong', 0) >= peer_before['bytesrecv_per_msg'].get('pong', 0) + ping_size, timeout=1) + self.wait_until(lambda: peer_after()['bytessent_per_msg'].get('ping', 0) >= peer_before['bytessent_per_msg'].get('ping', 0) + ping_size, timeout=1) def test_getnetworkinfo(self): self.log.info("Test getnetworkinfo") @@ -342,7 +346,10 @@ def test_sendmsgtopeer(self): node = self.nodes[0] self.restart_node(0) - self.connect_nodes(0, 1) + # we want to use a p2p v1 connection here in order to ensure + # a peer id of zero (a downgrade from v2 to v1 would lead + # to an increase of the peer id) + self.connect_nodes(0, 1, peer_advertises_v2=False) self.log.info("Test sendmsgtopeer") self.log.debug("Send a valid message") diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index cbc03a3e3a..320e0abf10 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -240,6 +240,7 @@ 'p2p_getdata.py', 'p2p_addrfetch.py', 'rpc_net.py', + 'rpc_net.py --v2transport', 'wallet_keypool.py --legacy-wallet', 'wallet_keypool.py --descriptors', 'wallet_descriptor.py --descriptors', From 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 6 Nov 2023 15:07:45 -0500 Subject: [PATCH 18/62] test: enable v2 transport for p2p_timeouts.py by skipping the part where we send a non-version message before the version - this message would be interpreted as part of the v2 handshake. --- test/functional/p2p_timeouts.py | 15 ++++++++++----- test/functional/test_runner.py | 1 + 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/test/functional/p2p_timeouts.py b/test/functional/p2p_timeouts.py index a308577c02..b4fa5099d8 100755 --- a/test/functional/p2p_timeouts.py +++ b/test/functional/p2p_timeouts.py @@ -68,11 +68,14 @@ def run_test(self): with self.nodes[0].assert_debug_log(['Unsupported message "ping" prior to verack from peer=0']): no_verack_node.send_message(msg_ping()) - with self.nodes[0].assert_debug_log(['non-version message before version handshake. Message "ping" from peer=1']): - no_version_node.send_message(msg_ping()) - self.mock_forward(1) + # With v2, non-version messages before the handshake would be interpreted as part of the key exchange. + # Therefore, don't execute this part of the test if v2transport is chosen. + if not self.options.v2transport: + with self.nodes[0].assert_debug_log(['non-version message before version handshake. Message "ping" from peer=1']): + no_version_node.send_message(msg_ping()) + self.mock_forward(1) assert "version" in no_verack_node.last_message assert no_verack_node.is_connected @@ -80,11 +83,12 @@ def run_test(self): assert no_send_node.is_connected no_verack_node.send_message(msg_ping()) - no_version_node.send_message(msg_ping()) + if not self.options.v2transport: + no_version_node.send_message(msg_ping()) expected_timeout_logs = [ "version handshake timeout peer=0", - "socket no message in first 3 seconds, 1 0 peer=1", + f"socket no message in first 3 seconds, {'0' if self.options.v2transport else '1'} 0 peer=1", "socket no message in first 3 seconds, 0 0 peer=2", ] @@ -100,5 +104,6 @@ def run_test(self): extra_args=['-peertimeout=0'], ) + if __name__ == '__main__': TimeoutsTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 320e0abf10..a935442595 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -153,6 +153,7 @@ 'p2p_invalid_messages.py', 'rpc_createmultisig.py', 'p2p_timeouts.py', + 'p2p_timeouts.py --v2transport', 'wallet_dump.py --legacy-wallet', 'rpc_signer.py', 'wallet_signer.py --descriptors', From 05aca093819be276ac7d648472c6ed5c7d235cc5 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 10 Nov 2023 23:25:13 +0000 Subject: [PATCH 19/62] build: Patch Qt to handle minimum macOS version properly This change is required to switch to macOS 14 SDK (Xcode 15). --- depends/packages/qt.mk | 2 ++ depends/patches/qt/fix-minimum-macos.patch | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+) create mode 100644 depends/patches/qt/fix-minimum-macos.patch diff --git a/depends/packages/qt.mk b/depends/packages/qt.mk index b6d8864383..3e3b78987a 100644 --- a/depends/packages/qt.mk +++ b/depends/packages/qt.mk @@ -23,6 +23,7 @@ $(package)_patches += guix_cross_lib_path.patch $(package)_patches += fix-macos-linker.patch $(package)_patches += memory_resource.patch $(package)_patches += windows_lto.patch +$(package)_patches += fix-minimum-macos.patch $(package)_qttranslations_file_name=qttranslations-$($(package)_suffix) $(package)_qttranslations_sha256_hash=a31785948c640b7c66d9fe2db4993728ca07f64e41c560b3625ad191b276ff20 @@ -239,6 +240,7 @@ endef define $(package)_preprocess_cmds cp $($(package)_patch_dir)/qt.pro qt.pro && \ cp $($(package)_patch_dir)/qttools_src.pro qttools/src/src.pro && \ + patch -p1 -i $($(package)_patch_dir)/fix-minimum-macos.patch && \ patch -p1 -i $($(package)_patch_dir)/fix-macos-linker.patch && \ patch -p1 -i $($(package)_patch_dir)/dont_hardcode_pwd.patch && \ patch -p1 -i $($(package)_patch_dir)/fix_qt_pkgconfig.patch && \ diff --git a/depends/patches/qt/fix-minimum-macos.patch b/depends/patches/qt/fix-minimum-macos.patch new file mode 100644 index 0000000000..ecaa2ca308 --- /dev/null +++ b/depends/patches/qt/fix-minimum-macos.patch @@ -0,0 +1,18 @@ +Ensure that Qt handles the minimum macOS version properly + +This patch can be dropped for LLVM Clang 17+, after commit +https://github.com/llvm/llvm-project/commit/c8e2dd8c6f490b68e41fe663b44535a8a21dfeab + + +--- a/qtbase/src/corelib/global/qsystemdetection.h ++++ b/qtbase/src/corelib/global/qsystemdetection.h +@@ -220,6 +220,9 @@ + # include + # include + # ++# undef __MAC_OS_X_VERSION_MIN_REQUIRED ++# define __MAC_OS_X_VERSION_MIN_REQUIRED MAC_OS_X_VERSION_MIN_REQUIRED ++# + # ifdef Q_OS_MACOS + # if !defined(__MAC_OS_X_VERSION_MIN_REQUIRED) || __MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_6 + # undef __MAC_OS_X_VERSION_MIN_REQUIRED From fa0ed0794161d937d2d3385963c1aa5624b60d17 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 16 Nov 2023 12:59:43 +0100 Subject: [PATCH 20/62] refactor: VectorWriter without nVersion The field is unused, so remove it. This is also required for future commits. --- src/blockfilter.cpp | 4 ++-- src/net.cpp | 2 +- src/netmessagemaker.h | 9 +++------ src/psbt.h | 6 +++--- src/signet.cpp | 2 +- src/streams.h | 19 ++++++------------- src/test/fuzz/golomb_rice.cpp | 4 ++-- src/test/streams_tests.cpp | 28 ++++++++++++++-------------- src/wallet/test/wallet_tests.cpp | 2 +- 9 files changed, 33 insertions(+), 43 deletions(-) diff --git a/src/blockfilter.cpp b/src/blockfilter.cpp index dd3824fb1c..404c9ecc4f 100644 --- a/src/blockfilter.cpp +++ b/src/blockfilter.cpp @@ -81,7 +81,7 @@ GCSFilter::GCSFilter(const Params& params, const ElementSet& elements) } m_F = static_cast(m_N) * static_cast(m_params.m_M); - CVectorWriter stream(GCS_SER_VERSION, m_encoded, 0); + VectorWriter stream{m_encoded, 0}; WriteCompactSize(stream, m_N); @@ -89,7 +89,7 @@ GCSFilter::GCSFilter(const Params& params, const ElementSet& elements) return; } - BitStreamWriter bitwriter(stream); + BitStreamWriter bitwriter{stream}; uint64_t last_value = 0; for (uint64_t value : BuildHashedSet(elements)) { diff --git a/src/net.cpp b/src/net.cpp index a2f80cbcf7..075a7d9839 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -818,7 +818,7 @@ bool V1Transport::SetMessageToSend(CSerializedNetMsg& msg) noexcept // serialize header m_header_to_send.clear(); - CVectorWriter{INIT_PROTO_VERSION, m_header_to_send, 0, hdr}; + VectorWriter{m_header_to_send, 0, hdr}; // update state m_message_to_send = std::move(msg); diff --git a/src/netmessagemaker.h b/src/netmessagemaker.h index a121183aab..60b8e579d6 100644 --- a/src/netmessagemaker.h +++ b/src/netmessagemaker.h @@ -12,14 +12,14 @@ class CNetMsgMaker { public: - explicit CNetMsgMaker(int nVersionIn) : nVersion(nVersionIn){} + explicit CNetMsgMaker(int /*unused*/) {} template - CSerializedNetMsg Make(int nFlags, std::string msg_type, Args&&... args) const + CSerializedNetMsg Make(int /*unused*/, std::string msg_type, Args&&... args) const { CSerializedNetMsg msg; msg.m_type = std::move(msg_type); - CVectorWriter{nFlags | nVersion, msg.data, 0, std::forward(args)...}; + VectorWriter{msg.data, 0, std::forward(args)...}; return msg; } @@ -28,9 +28,6 @@ class CNetMsgMaker { return Make(0, std::move(msg_type), std::forward(args)...); } - -private: - const int nVersion; }; #endif // BITCOIN_NETMESSAGEMAKER_H diff --git a/src/psbt.h b/src/psbt.h index a14df03837..ac2d246a27 100644 --- a/src/psbt.h +++ b/src/psbt.h @@ -316,7 +316,7 @@ struct PSBTInput const auto& [leaf_hashes, origin] = leaf_origin; SerializeToVector(s, PSBT_IN_TAP_BIP32_DERIVATION, xonly); std::vector value; - CVectorWriter s_value{s.GetVersion(), value, 0}; + VectorWriter s_value{value, 0}; s_value << leaf_hashes; SerializeKeyOrigin(s_value, origin); s << value; @@ -757,7 +757,7 @@ struct PSBTOutput if (!m_tap_tree.empty()) { SerializeToVector(s, PSBT_OUT_TAP_TREE); std::vector value; - CVectorWriter s_value{s.GetVersion(), value, 0}; + VectorWriter s_value{value, 0}; for (const auto& [depth, leaf_ver, script] : m_tap_tree) { s_value << depth; s_value << leaf_ver; @@ -771,7 +771,7 @@ struct PSBTOutput const auto& [leaf_hashes, origin] = leaf; SerializeToVector(s, PSBT_OUT_TAP_BIP32_DERIVATION, xonly); std::vector value; - CVectorWriter s_value{s.GetVersion(), value, 0}; + VectorWriter s_value{value, 0}; s_value << leaf_hashes; SerializeKeyOrigin(s_value, origin); s << value; diff --git a/src/signet.cpp b/src/signet.cpp index d6a3a44d91..ef9913218a 100644 --- a/src/signet.cpp +++ b/src/signet.cpp @@ -110,7 +110,7 @@ std::optional SignetTxs::Create(const CBlock& block, const CScript& c uint256 signet_merkle = ComputeModifiedMerkleRoot(modified_cb, block); std::vector block_data; - CVectorWriter writer{INIT_PROTO_VERSION, block_data, 0}; + VectorWriter writer{block_data, 0}; writer << block.nVersion; writer << block.hashPrevBlock; writer << signet_merkle; diff --git a/src/streams.h b/src/streams.h index a3f8028da7..88a5f3da54 100644 --- a/src/streams.h +++ b/src/streams.h @@ -49,17 +49,15 @@ inline void Xor(Span write, Span key, size_t key_off * * The referenced vector will grow as necessary */ -class CVectorWriter +class VectorWriter { - public: - +public: /* - * @param[in] nVersionIn Serialization Version (including any flags) * @param[in] vchDataIn Referenced byte vector to overwrite/append * @param[in] nPosIn Starting position. Vector index where writes should start. The vector will initially * grow as necessary to max(nPosIn, vec.size()). So to append, use vec.size(). */ - CVectorWriter(int nVersionIn, std::vector& vchDataIn, size_t nPosIn) : nVersion{nVersionIn}, vchData{vchDataIn}, nPos{nPosIn} + VectorWriter(std::vector& vchDataIn, size_t nPosIn) : vchData{vchDataIn}, nPos{nPosIn} { if(nPos > vchData.size()) vchData.resize(nPos); @@ -69,7 +67,7 @@ class CVectorWriter * @param[in] args A list of items to serialize starting at nPosIn. */ template - CVectorWriter(int nVersionIn, std::vector& vchDataIn, size_t nPosIn, Args&&... args) : CVectorWriter{nVersionIn, vchDataIn, nPosIn} + VectorWriter(std::vector& vchDataIn, size_t nPosIn, Args&&... args) : VectorWriter{vchDataIn, nPosIn} { ::SerializeMany(*this, std::forward(args)...); } @@ -85,19 +83,14 @@ class CVectorWriter } nPos += src.size(); } - template - CVectorWriter& operator<<(const T& obj) + template + VectorWriter& operator<<(const T& obj) { ::Serialize(*this, obj); return (*this); } - int GetVersion() const - { - return nVersion; - } private: - const int nVersion; std::vector& vchData; size_t nPos; }; diff --git a/src/test/fuzz/golomb_rice.cpp b/src/test/fuzz/golomb_rice.cpp index f3073c5c97..8b395f3ea9 100644 --- a/src/test/fuzz/golomb_rice.cpp +++ b/src/test/fuzz/golomb_rice.cpp @@ -51,9 +51,9 @@ FUZZ_TARGET(golomb_rice) for (int i = 0; i < n; ++i) { elements.insert(ConsumeRandomLengthByteVector(fuzzed_data_provider, 16)); } - CVectorWriter stream{0, golomb_rice_data, 0}; + VectorWriter stream{golomb_rice_data, 0}; WriteCompactSize(stream, static_cast(elements.size())); - BitStreamWriter bitwriter(stream); + BitStreamWriter bitwriter{stream}; if (!elements.empty()) { uint64_t last_value = 0; for (const uint64_t value : BuildHashedSet(elements, static_cast(elements.size()) * static_cast(BASIC_FILTER_M))) { diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index f03f7c1da2..208993a76d 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -74,49 +74,49 @@ BOOST_AUTO_TEST_CASE(streams_vector_writer) // point should yield the same results, even if the first test grew the // vector. - CVectorWriter{INIT_PROTO_VERSION, vch, 0, a, b}; + VectorWriter{vch, 0, a, b}; BOOST_CHECK((vch == std::vector{{1, 2}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 0, a, b}; + VectorWriter{vch, 0, a, b}; BOOST_CHECK((vch == std::vector{{1, 2}})); vch.clear(); - CVectorWriter{INIT_PROTO_VERSION, vch, 2, a, b}; + VectorWriter{vch, 2, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 1, 2}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 2, a, b}; + VectorWriter{vch, 2, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 1, 2}})); vch.clear(); vch.resize(5, 0); - CVectorWriter{INIT_PROTO_VERSION, vch, 2, a, b}; + VectorWriter{vch, 2, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 1, 2, 0}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 2, a, b}; + VectorWriter{vch, 2, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 1, 2, 0}})); vch.clear(); vch.resize(4, 0); - CVectorWriter{INIT_PROTO_VERSION, vch, 3, a, b}; + VectorWriter{vch, 3, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 0, 1, 2}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 3, a, b}; + VectorWriter{vch, 3, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 0, 1, 2}})); vch.clear(); vch.resize(4, 0); - CVectorWriter{INIT_PROTO_VERSION, vch, 4, a, b}; + VectorWriter{vch, 4, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 0, 0, 1, 2}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 4, a, b}; + VectorWriter{vch, 4, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 0, 0, 1, 2}})); vch.clear(); - CVectorWriter{INIT_PROTO_VERSION, vch, 0, bytes}; + VectorWriter{vch, 0, bytes}; BOOST_CHECK((vch == std::vector{{3, 4, 5, 6}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 0, bytes}; + VectorWriter{vch, 0, bytes}; BOOST_CHECK((vch == std::vector{{3, 4, 5, 6}})); vch.clear(); vch.resize(4, 8); - CVectorWriter{INIT_PROTO_VERSION, vch, 2, a, bytes, b}; + VectorWriter{vch, 2, a, bytes, b}; BOOST_CHECK((vch == std::vector{{8, 8, 1, 3, 4, 5, 6, 2}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 2, a, bytes, b}; + VectorWriter{vch, 2, a, bytes, b}; BOOST_CHECK((vch == std::vector{{8, 8, 1, 3, 4, 5, 6, 2}})); vch.clear(); } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index bcbc31ed3e..c5452c3b0d 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -752,7 +752,7 @@ bool malformed_descriptor(std::ios_base::failure e) BOOST_FIXTURE_TEST_CASE(wallet_descriptor_test, BasicTestingSetup) { std::vector malformed_record; - CVectorWriter vw{0, malformed_record, 0}; + VectorWriter vw{malformed_record, 0}; vw << std::string("notadescriptor"); vw << uint64_t{0}; vw << int32_t{0}; From 66669da4a5ca9edf2a40d20879d9a8aaf2b9e2ee Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 16 Nov 2023 14:46:51 +0100 Subject: [PATCH 21/62] Remove unused Make() overload in netmessagemaker.h --- src/netmessagemaker.h | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/netmessagemaker.h b/src/netmessagemaker.h index 60b8e579d6..ede03aa204 100644 --- a/src/netmessagemaker.h +++ b/src/netmessagemaker.h @@ -15,19 +15,13 @@ class CNetMsgMaker explicit CNetMsgMaker(int /*unused*/) {} template - CSerializedNetMsg Make(int /*unused*/, std::string msg_type, Args&&... args) const + CSerializedNetMsg Make(std::string msg_type, Args&&... args) const { CSerializedNetMsg msg; msg.m_type = std::move(msg_type); VectorWriter{msg.data, 0, std::forward(args)...}; return msg; } - - template - CSerializedNetMsg Make(std::string msg_type, Args&&... args) const - { - return Make(0, std::move(msg_type), std::forward(args)...); - } }; #endif // BITCOIN_NETMESSAGEMAKER_H From 705e3f1de00bf30d728addd52a790a139d948e32 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 17 Nov 2023 20:28:14 +0100 Subject: [PATCH 22/62] refactor: Make CTxMemPoolEntry only explicitly copyable This has the goal of prohibiting users from accidentally creating runtime failures, e.g. by interacting with iterator_to with a copied entry. CTxMemPoolEntry is already implicitly not move-constructable. So be explicit about this and use a std::list to collect the values in the policy_estimator fuzz test instead of a std::vector. Co-authored-by: Anthony Towns --- src/kernel/mempool_entry.h | 12 ++++++++++++ src/test/fuzz/policy_estimator.cpp | 4 ++-- src/txmempool.cpp | 2 +- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h index 7c905ca4f4..b5c0499012 100644 --- a/src/kernel/mempool_entry.h +++ b/src/kernel/mempool_entry.h @@ -71,6 +71,11 @@ class CTxMemPoolEntry typedef std::set Children; private: + CTxMemPoolEntry(const CTxMemPoolEntry&) = default; + struct ExplicitCopyTag { + explicit ExplicitCopyTag() = default; + }; + const CTransactionRef tx; mutable Parents m_parents; mutable Children m_children; @@ -122,6 +127,13 @@ class CTxMemPoolEntry nModFeesWithAncestors{nFee}, nSigOpCostWithAncestors{sigOpCost} {} + CTxMemPoolEntry(ExplicitCopyTag, const CTxMemPoolEntry& entry) : CTxMemPoolEntry(entry) {} + CTxMemPoolEntry& operator=(const CTxMemPoolEntry&) = delete; + CTxMemPoolEntry(CTxMemPoolEntry&&) = delete; + CTxMemPoolEntry& operator=(CTxMemPoolEntry&&) = delete; + + static constexpr ExplicitCopyTag ExplicitCopy{}; + const CTransaction& GetTx() const { return *this->tx; } CTransactionRef GetSharedTx() const { return this->tx; } const CAmount& GetFee() const { return nFee; } diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp index a6275d2fd2..ab885043d7 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -49,7 +49,7 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) } }, [&] { - std::vector mempool_entries; + std::list mempool_entries; LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) { const std::optional mtx = ConsumeDeserializable(fuzzed_data_provider, TX_WITH_WITNESS); @@ -58,7 +58,7 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) break; } const CTransaction tx{*mtx}; - mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx)); + mempool_entries.emplace_back(CTxMemPoolEntry::ExplicitCopy, ConsumeTxMemPoolEntry(fuzzed_data_provider, tx)); } std::vector ptrs; ptrs.reserve(mempool_entries.size()); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index e057d7ece1..f6b8051451 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -438,7 +438,7 @@ void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAnces // Add to memory pool without checking anything. // Used by AcceptToMemoryPool(), which DOES do // all the appropriate checks. - indexed_transaction_set::iterator newit = mapTx.insert(entry).first; + indexed_transaction_set::iterator newit = mapTx.emplace(CTxMemPoolEntry::ExplicitCopy, entry).first; // Update transaction for any feeDelta created by PrioritiseTransaction CAmount delta{0}; From fa9b5f4fe32c0cfe2e477bb11912756f84a52cfe Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 16 Nov 2023 15:43:15 +0100 Subject: [PATCH 23/62] refactor: NetMsg::Make() without nVersion The nVersion field is unused, so remove it. This is also required for future commits. Also, add PushMessage aliases in PeerManagerImpl to make calling code less verbose. Co-Authored-By: Anthony Towns --- src/net_processing.cpp | 149 ++++++++---------- src/netmessagemaker.h | 10 +- src/test/fuzz/p2p_transport_serialization.cpp | 2 +- src/test/net_tests.cpp | 5 +- src/test/util/net.cpp | 5 +- 5 files changed, 74 insertions(+), 97 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1556dc7e67..0cd20680b0 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -51,6 +51,7 @@ #include #include #include +#include /** Headers download timeout. * Timeout = base + per_header * (expected number of headers) */ @@ -669,6 +670,14 @@ class PeerManagerImpl final : public PeerManager void AddTxAnnouncement(const CNode& node, const GenTxid& gtxid, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + /** Send a message to a peer */ + void PushMessage(CNode& node, CSerializedNetMsg&& msg) const { m_connman.PushMessage(&node, std::move(msg)); } + template + void MakeAndPushMessage(CNode& node, std::string msg_type, Args&&... args) const + { + m_connman.PushMessage(&node, NetMsg::Make(std::move(msg_type), std::forward(args)...)); + } + /** Send a version message to a peer */ void PushNodeVersion(CNode& pnode, const Peer& peer); @@ -1277,14 +1286,14 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) // As per BIP152, we only get 3 of our peers to announce // blocks using compact encodings. m_connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [this](CNode* pnodeStop){ - m_connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION)); + MakeAndPushMessage(*pnodeStop, NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION); // save BIP152 bandwidth state: we select peer to be low-bandwidth pnodeStop->m_bip152_highbandwidth_to = false; return true; }); lNodesAnnouncingHeaderAndIDs.pop_front(); } - m_connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/true, /*version=*/CMPCTBLOCKS_VERSION)); + MakeAndPushMessage(*pfrom, NetMsgType::SENDCMPCT, /*high_bandwidth=*/true, /*version=*/CMPCTBLOCKS_VERSION); // save BIP152 bandwidth state: we select peer to be high-bandwidth pfrom->m_bip152_highbandwidth_to = true; lNodesAnnouncingHeaderAndIDs.push_back(pfrom->GetId()); @@ -1487,10 +1496,10 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer) uint64_t your_services{addr.nServices}; const bool tx_relay{!RejectIncomingTxs(pnode)}; - m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime, + MakeAndPushMessage(pnode, NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime, your_services, CNetAddr::V1(addr_you), // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime) my_services, CNetAddr::V1(CService{}), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime) - nonce, strSubVersion, nNodeStartingHeight, tx_relay)); + nonce, strSubVersion, nNodeStartingHeight, tx_relay); if (fLogIPs) { LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, them=%s, txrelay=%d, peer=%d\n", PROTOCOL_VERSION, nNodeStartingHeight, addr_you.ToStringAddrPort(), tx_relay, nodeid); @@ -1861,8 +1870,7 @@ std::optional PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl // Send block request message to the peer bool success = m_connman.ForNode(peer_id, [this, &invs](CNode* node) { - const CNetMsgMaker msgMaker(node->GetCommonVersion()); - this->m_connman.PushMessage(node, msgMaker.Make(NetMsgType::GETDATA, invs)); + this->MakeAndPushMessage(*node, NetMsgType::GETDATA, invs); return true; }); @@ -1985,7 +1993,6 @@ void PeerManagerImpl::BlockDisconnected(const std::shared_ptr &blo void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) { auto pcmpctblock = std::make_shared(*pblock); - const CNetMsgMaker msgMaker(PROTOCOL_VERSION); LOCK(cs_main); @@ -1997,7 +2004,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha uint256 hashBlock(pblock->GetHash()); const std::shared_future lazy_ser{ - std::async(std::launch::deferred, [&] { return msgMaker.Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })}; + std::async(std::launch::deferred, [&] { return NetMsg::Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })}; { auto most_recent_block_txs = std::make_unique>(); @@ -2028,7 +2035,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha hashBlock.ToString(), pnode->GetId()); const CSerializedNetMsg& ser_cmpctblock{lazy_ser.get()}; - m_connman.PushMessage(pnode, ser_cmpctblock.Copy()); + PushMessage(*pnode, ser_cmpctblock.Copy()); state.pindexBestHeaderSent = pindex; } }); @@ -2262,7 +2269,6 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& LogPrint(BCLog::NET, "%s: ignoring request from peer=%i for old block that isn't in the main chain\n", __func__, pfrom.GetId()); return; } - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); // disconnect node in case we have reached the outbound limit for serving historical blocks if (m_connman.OutboundTargetReached(true) && (((m_chainman.m_best_header != nullptr) && (m_chainman.m_best_header->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.IsMsgFilteredBlk()) && @@ -2296,7 +2302,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& if (!m_chainman.m_blockman.ReadRawBlockFromDisk(block_data, pindex->GetBlockPos())) { assert(!"cannot load block from disk"); } - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, Span{block_data})); + MakeAndPushMessage(pfrom, NetMsgType::BLOCK, Span{block_data}); // Don't set pblock as we've sent the block } else { // Send block from disk @@ -2308,9 +2314,9 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } if (pblock) { if (inv.IsMsgBlk()) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, TX_NO_WITNESS(*pblock))); + MakeAndPushMessage(pfrom, NetMsgType::BLOCK, TX_NO_WITNESS(*pblock)); } else if (inv.IsMsgWitnessBlk()) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, TX_WITH_WITNESS(*pblock))); + MakeAndPushMessage(pfrom, NetMsgType::BLOCK, TX_WITH_WITNESS(*pblock)); } else if (inv.IsMsgFilteredBlk()) { bool sendMerkleBlock = false; CMerkleBlock merkleBlock; @@ -2322,7 +2328,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } } if (sendMerkleBlock) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MERKLEBLOCK, merkleBlock)); + MakeAndPushMessage(pfrom, NetMsgType::MERKLEBLOCK, merkleBlock); // CMerkleBlock just contains hashes, so also push any transactions in the block the client did not see // This avoids hurting performance by pointlessly requiring a round-trip // Note that there is currently no way for a node to request any single transactions we didn't send here - @@ -2331,7 +2337,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& // however we MUST always provide at least what the remote peer needs typedef std::pair PairType; for (PairType& pair : merkleBlock.vMatchedTxn) - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::TX, TX_NO_WITNESS(*pblock->vtx[pair.first]))); + MakeAndPushMessage(pfrom, NetMsgType::TX, TX_NO_WITNESS(*pblock->vtx[pair.first])); } // else // no response @@ -2342,13 +2348,13 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& // instead we respond with the full, non-compact block. if (CanDirectFetch() && pindex->nHeight >= m_chainman.ActiveChain().Height() - MAX_CMPCTBLOCK_DEPTH) { if (a_recent_compact_block && a_recent_compact_block->header.GetHash() == pindex->GetBlockHash()) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CMPCTBLOCK, *a_recent_compact_block)); + MakeAndPushMessage(pfrom, NetMsgType::CMPCTBLOCK, *a_recent_compact_block); } else { CBlockHeaderAndShortTxIDs cmpctblock{*pblock}; - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CMPCTBLOCK, cmpctblock)); + MakeAndPushMessage(pfrom, NetMsgType::CMPCTBLOCK, cmpctblock); } } else { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, TX_WITH_WITNESS(*pblock))); + MakeAndPushMessage(pfrom, NetMsgType::BLOCK, TX_WITH_WITNESS(*pblock)); } } } @@ -2362,7 +2368,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& // wait for other stuff first. std::vector vInv; vInv.emplace_back(MSG_BLOCK, m_chainman.ActiveChain().Tip()->GetBlockHash()); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(pfrom, NetMsgType::INV, vInv); peer.m_continuation_block.SetNull(); } } @@ -2396,7 +2402,6 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic std::deque::iterator it = peer.m_getdata_requests.begin(); std::vector vNotFound; - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); // Process as many TX items from the front of the getdata queue as // possible, since they're common and it's efficient to batch process @@ -2419,7 +2424,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic if (tx) { // WTX and WITNESS_TX imply we serialize with witness const auto maybe_with_witness = (inv.IsMsgTx() ? TX_NO_WITNESS : TX_WITH_WITNESS); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::TX, maybe_with_witness(*tx))); + MakeAndPushMessage(pfrom, NetMsgType::TX, maybe_with_witness(*tx)); m_mempool.RemoveUnbroadcastTx(tx->GetHash()); } else { vNotFound.push_back(inv); @@ -2454,7 +2459,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic // In normal operation, we often send NOTFOUND messages for parents of // transactions that we relay; if a peer is missing a parent, they may // assume we have them and request the parents from us. - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::NOTFOUND, vNotFound)); + MakeAndPushMessage(pfrom, NetMsgType::NOTFOUND, vNotFound); } } @@ -2478,8 +2483,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo resp.txn[i] = block.vtx[req.indexes[i]]; } - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCKTXN, resp)); + MakeAndPushMessage(pfrom, NetMsgType::BLOCKTXN, resp); } bool PeerManagerImpl::CheckHeadersPoW(const std::vector& headers, const Consensus::Params& consensusParams, Peer& peer) @@ -2707,14 +2711,12 @@ bool PeerManagerImpl::IsAncestorOfBestHeaderOrTip(const CBlockIndex* header) bool PeerManagerImpl::MaybeSendGetHeaders(CNode& pfrom, const CBlockLocator& locator, Peer& peer) { - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); - const auto current_time = NodeClock::now(); // Only allow a new getheaders message to go out if we don't have a recent // one already in-flight if (current_time - peer.m_last_getheaders_timestamp > HEADERS_RESPONSE_TIME) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, locator, uint256())); + MakeAndPushMessage(pfrom, NetMsgType::GETHEADERS, locator, uint256()); peer.m_last_getheaders_timestamp = current_time; return true; } @@ -2728,8 +2730,6 @@ bool PeerManagerImpl::MaybeSendGetHeaders(CNode& pfrom, const CBlockLocator& loc */ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, const CBlockIndex& last_header) { - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); - LOCK(cs_main); CNodeState *nodestate = State(pfrom.GetId()); @@ -2782,7 +2782,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c // In any case, we want to download using a compact block, not a regular one vGetData[0] = CInv(MSG_CMPCT_BLOCK, vGetData[0].hash); } - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vGetData)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vGetData); } } } @@ -3155,9 +3155,7 @@ void PeerManagerImpl::ProcessGetCFilters(CNode& node,Peer& peer, CDataStream& vR } for (const auto& filter : filters) { - CSerializedNetMsg msg = CNetMsgMaker(node.GetCommonVersion()) - .Make(NetMsgType::CFILTER, filter); - m_connman.PushMessage(&node, std::move(msg)); + MakeAndPushMessage(node, NetMsgType::CFILTER, filter); } } @@ -3196,13 +3194,11 @@ void PeerManagerImpl::ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream& return; } - CSerializedNetMsg msg = CNetMsgMaker(node.GetCommonVersion()) - .Make(NetMsgType::CFHEADERS, + MakeAndPushMessage(node, NetMsgType::CFHEADERS, filter_type_ser, stop_index->GetBlockHash(), prev_header, filter_hashes); - m_connman.PushMessage(&node, std::move(msg)); } void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& vRecv) @@ -3237,12 +3233,10 @@ void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& } } - CSerializedNetMsg msg = CNetMsgMaker(node.GetCommonVersion()) - .Make(NetMsgType::CFCHECKPT, + MakeAndPushMessage(node, NetMsgType::CFCHECKPT, filter_type_ser, stop_index->GetBlockHash(), headers); - m_connman.PushMessage(&node, std::move(msg)); } void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr& block, bool force_processing, bool min_pow_checked) @@ -3266,7 +3260,6 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl { std::shared_ptr pblock = std::make_shared(); bool fBlockRead{false}; - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); { LOCK(cs_main); @@ -3302,7 +3295,7 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl // Might have collided, fall back to getdata now :( std::vector invs; invs.emplace_back(MSG_BLOCK | GetFetchFlags(peer), block_transactions.blockhash); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, invs)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, invs); } else { RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); LogPrint(BCLog::NET, "Peer %d sent us a compact block but it failed to reconstruct, waiting on first download to complete\n", pfrom.GetId()); @@ -3441,10 +3434,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, pfrom.SetCommonVersion(greatest_common_version); pfrom.nVersion = nVersion; - const CNetMsgMaker msg_maker(greatest_common_version); - if (greatest_common_version >= WTXID_RELAY_VERSION) { - m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::WTXIDRELAY)); + MakeAndPushMessage(pfrom, NetMsgType::WTXIDRELAY); } // Signal ADDRv2 support (BIP155). @@ -3453,7 +3444,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // implementations reject messages they don't know. As a courtesy, don't send // it to nodes with a version before 70016, as no software is known to support // BIP155 that doesn't announce at least that protocol version number. - m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDADDRV2)); + MakeAndPushMessage(pfrom, NetMsgType::SENDADDRV2); } pfrom.m_has_all_wanted_services = HasAllDesirableServiceFlags(nServices); @@ -3493,12 +3484,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (tx_relay && WITH_LOCK(tx_relay->m_bloom_filter_mutex, return tx_relay->m_relay_txs) && !pfrom.IsAddrFetchConn() && !m_opts.ignore_incoming_txs) { const uint64_t recon_salt = m_txreconciliation->PreRegisterPeer(pfrom.GetId()); - m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDTXRCNCL, - TXRECONCILIATION_VERSION, recon_salt)); + MakeAndPushMessage(pfrom, NetMsgType::SENDTXRCNCL, + TXRECONCILIATION_VERSION, recon_salt); } } - m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::VERACK)); + MakeAndPushMessage(pfrom, NetMsgType::VERACK); // Potentially mark this peer as a preferred download peer. { @@ -3522,7 +3513,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // We skip this for block-relay-only peers. We want to avoid // potentially leaking addr information and we do not want to // indicate to the peer that we will participate in addr relay. - m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::GETADDR)); + MakeAndPushMessage(pfrom, NetMsgType::GETADDR); peer->m_getaddr_sent = true; // When requesting a getaddr, accept an additional MAX_ADDR_TO_SEND addresses in response // (bypassing the MAX_ADDR_PROCESSING_TOKEN_BUCKET limit). @@ -3568,7 +3559,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // If the peer is old enough to have the old alert system, send it the final alert. if (greatest_common_version <= 70012) { const auto finalAlert{ParseHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f00000000ffffff7f00ffffff7f002f555247454e543a20416c657274206b657920636f6d70726f6d697365642c2075706772616465207265717569726564004630440220653febd6410f470f6bae11cad19c48413becb1ac2c17f908fd0fd53bdc3abd5202206d0e9c96fe88d4a0f01ed9dedae2b6f9e00da94cad0fecaae66ecf689bf71b50")}; - m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make("alert", Span{finalAlert})); + MakeAndPushMessage(pfrom, "alert", Span{finalAlert}); } // Feeler connections exist only to verify if address is online. @@ -3585,9 +3576,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, return; } - // At this point, the outgoing message serialization version can't change. - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); - if (msg_type == NetMsgType::VERACK) { if (pfrom.fSuccessfullyConnected) { LogPrint(BCLog::NET, "ignoring redundant verack message from peer=%d\n", pfrom.GetId()); @@ -3612,7 +3600,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // cmpctblock messages. // We send this to non-NODE NETWORK peers as well, because // they may wish to request compact blocks from us - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION)); + MakeAndPushMessage(pfrom, NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION); } if (m_txreconciliation) { @@ -4119,7 +4107,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d because active chain has too little work; sending empty response\n", pfrom.GetId()); // Just respond with an empty headers message, to tell the peer to // go away but not treat us as unresponsive. - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::HEADERS, std::vector())); + MakeAndPushMessage(pfrom, NetMsgType::HEADERS, std::vector()); return; } @@ -4169,7 +4157,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // will re-announce the new block via headers (or compact blocks again) // in the SendMessages logic. nodestate->pindexBestHeaderSent = pindex ? pindex : m_chainman.ActiveChain().Tip(); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders))); + MakeAndPushMessage(pfrom, NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders)); return; } @@ -4471,7 +4459,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // so we just grab the block via normal getdata std::vector vInv(1); vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(*peer), blockhash); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vInv); } return; } @@ -4508,7 +4496,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // Duplicate txindexes, the block is now in-flight, so just request it std::vector vInv(1); vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(*peer), blockhash); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vInv); } else { // Give up for this peer and wait for other peer(s) RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); @@ -4527,7 +4515,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // We will try to round-trip any compact blocks we get on failure, // as long as it's first... req.blockhash = pindex->GetBlockHash(); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETBLOCKTXN, req)); + MakeAndPushMessage(pfrom, NetMsgType::GETBLOCKTXN, req); } else if (pfrom.m_bip152_highbandwidth_to && (!pfrom.IsInboundConn() || IsBlockRequestedFromOutbound(blockhash) || @@ -4537,7 +4525,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // - we already have an outbound attempt in flight(so we'll take what we can get), or // - it's not the final parallel download slot (which we may reserve for first outbound) req.blockhash = pindex->GetBlockHash(); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETBLOCKTXN, req)); + MakeAndPushMessage(pfrom, NetMsgType::GETBLOCKTXN, req); } else { // Give up for this peer and wait for other peer(s) RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); @@ -4566,7 +4554,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // mempool will probably be useless - request the block normally std::vector vInv(1); vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(*peer), blockhash); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vInv); return; } else { // If this was an announce-cmpctblock, we want the same treatment as a header message @@ -4796,7 +4784,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // it, if the remote node sends a ping once per second and this node takes 5 // seconds to respond to each, the 5th ping the remote sends would appear to // return very quickly. - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::PONG, nonce)); + MakeAndPushMessage(pfrom, NetMsgType::PONG, nonce); } return; } @@ -5297,7 +5285,6 @@ void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::mic return; } - const CNetMsgMaker msgMaker(node_to.GetCommonVersion()); bool pingSend = false; if (peer.m_ping_queued) { @@ -5319,11 +5306,11 @@ void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::mic peer.m_ping_start = now; if (node_to.GetCommonVersion() > BIP0031_VERSION) { peer.m_ping_nonce_sent = nonce; - m_connman.PushMessage(&node_to, msgMaker.Make(NetMsgType::PING, nonce)); + MakeAndPushMessage(node_to, NetMsgType::PING, nonce); } else { // Peer is too old to support ping command with nonce, pong will never arrive. peer.m_ping_nonce_sent = 0; - m_connman.PushMessage(&node_to, msgMaker.Make(NetMsgType::PING)); + MakeAndPushMessage(node_to, NetMsgType::PING); } } } @@ -5377,11 +5364,10 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros // No addr messages to send if (peer.m_addrs_to_send.empty()) return; - CNetMsgMaker mm(node.GetCommonVersion()); if (peer.m_wants_addrv2) { - m_connman.PushMessage(&node, mm.Make(NetMsgType::ADDRV2, CAddress::V2_NETWORK(peer.m_addrs_to_send))); + MakeAndPushMessage(node, NetMsgType::ADDRV2, CAddress::V2_NETWORK(peer.m_addrs_to_send)); } else { - m_connman.PushMessage(&node, mm.Make(NetMsgType::ADDR, CAddress::V1_NETWORK(peer.m_addrs_to_send))); + MakeAndPushMessage(node, NetMsgType::ADDR, CAddress::V1_NETWORK(peer.m_addrs_to_send)); } peer.m_addrs_to_send.clear(); @@ -5406,7 +5392,7 @@ void PeerManagerImpl::MaybeSendSendHeaders(CNode& node, Peer& peer) // We send this to non-NODE NETWORK peers as well, because even // non-NODE NETWORK peers can announce blocks (such as pruning // nodes) - m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(NetMsgType::SENDHEADERS)); + MakeAndPushMessage(node, NetMsgType::SENDHEADERS); peer.m_sent_sendheaders = true; } } @@ -5441,7 +5427,7 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi // We always have a fee filter of at least the min relay fee filterToSend = std::max(filterToSend, m_mempool.m_min_relay_feerate.GetFeePerK()); if (filterToSend != peer.m_fee_filter_sent) { - m_connman.PushMessage(&pto, CNetMsgMaker(pto.GetCommonVersion()).Make(NetMsgType::FEEFILTER, filterToSend)); + MakeAndPushMessage(pto, NetMsgType::FEEFILTER, filterToSend); peer.m_fee_filter_sent = filterToSend; } peer.m_next_send_feefilter = GetExponentialRand(current_time, AVG_FEEFILTER_BROADCAST_INTERVAL); @@ -5518,9 +5504,6 @@ bool PeerManagerImpl::SendMessages(CNode* pto) if (!pto->fSuccessfullyConnected || pto->fDisconnect) return true; - // If we get here, the outgoing message serialization version is set and can't change. - const CNetMsgMaker msgMaker(pto->GetCommonVersion()); - const auto current_time{GetTime()}; if (pto->IsAddrFetchConn() && current_time - pto->m_connected > 10 * AVG_ADDRESS_BROADCAST_INTERVAL) { @@ -5675,17 +5658,17 @@ bool PeerManagerImpl::SendMessages(CNode* pto) { LOCK(m_most_recent_block_mutex); if (m_most_recent_block_hash == pBestIndex->GetBlockHash()) { - cached_cmpctblock_msg = msgMaker.Make(NetMsgType::CMPCTBLOCK, *m_most_recent_compact_block); + cached_cmpctblock_msg = NetMsg::Make(NetMsgType::CMPCTBLOCK, *m_most_recent_compact_block); } } if (cached_cmpctblock_msg.has_value()) { - m_connman.PushMessage(pto, std::move(cached_cmpctblock_msg.value())); + PushMessage(*pto, std::move(cached_cmpctblock_msg.value())); } else { CBlock block; const bool ret{m_chainman.m_blockman.ReadBlockFromDisk(block, *pBestIndex)}; assert(ret); CBlockHeaderAndShortTxIDs cmpctblock{block}; - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::CMPCTBLOCK, cmpctblock)); + MakeAndPushMessage(*pto, NetMsgType::CMPCTBLOCK, cmpctblock); } state.pindexBestHeaderSent = pBestIndex; } else if (peer->m_prefers_headers) { @@ -5698,7 +5681,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) LogPrint(BCLog::NET, "%s: sending header %s to peer=%d\n", __func__, vHeaders.front().GetHash().ToString(), pto->GetId()); } - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders))); + MakeAndPushMessage(*pto, NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders)); state.pindexBestHeaderSent = pBestIndex; } else fRevertToInv = true; @@ -5743,7 +5726,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) for (const uint256& hash : peer->m_blocks_for_inv_relay) { vInv.emplace_back(MSG_BLOCK, hash); if (vInv.size() == MAX_INV_SZ) { - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(*pto, NetMsgType::INV, vInv); vInv.clear(); } } @@ -5796,7 +5779,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) tx_relay->m_tx_inventory_known_filter.insert(inv.hash); vInv.push_back(inv); if (vInv.size() == MAX_INV_SZ) { - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(*pto, NetMsgType::INV, vInv); vInv.clear(); } } @@ -5848,7 +5831,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) vInv.push_back(inv); nRelayedTransactions++; if (vInv.size() == MAX_INV_SZ) { - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(*pto, NetMsgType::INV, vInv); vInv.clear(); } tx_relay->m_tx_inventory_known_filter.insert(hash); @@ -5860,7 +5843,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) } } if (!vInv.empty()) - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(*pto, NetMsgType::INV, vInv); // Detect whether we're stalling auto stalling_timeout = m_block_stalling_timeout.load(); @@ -5980,7 +5963,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) gtxid.GetHash().ToString(), pto->GetId()); vGetData.emplace_back(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*peer)), gtxid.GetHash()); if (vGetData.size() >= MAX_GETDATA_SZ) { - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData)); + MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData); vGetData.clear(); } m_txrequest.RequestedTx(pto->GetId(), gtxid.GetHash(), current_time + GETDATA_TX_INTERVAL); @@ -5993,7 +5976,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) if (!vGetData.empty()) - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData)); + MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData); } // release cs_main MaybeSendFeefilter(*pto, *peer, current_time); return true; diff --git a/src/netmessagemaker.h b/src/netmessagemaker.h index ede03aa204..d04867e45e 100644 --- a/src/netmessagemaker.h +++ b/src/netmessagemaker.h @@ -9,19 +9,15 @@ #include #include -class CNetMsgMaker -{ -public: - explicit CNetMsgMaker(int /*unused*/) {} - +namespace NetMsg { template - CSerializedNetMsg Make(std::string msg_type, Args&&... args) const + CSerializedNetMsg Make(std::string msg_type, Args&&... args) { CSerializedNetMsg msg; msg.m_type = std::move(msg_type); VectorWriter{msg.data, 0, std::forward(args)...}; return msg; } -}; +} // namespace NetMsg #endif // BITCOIN_NETMESSAGEMAKER_H diff --git a/src/test/fuzz/p2p_transport_serialization.cpp b/src/test/fuzz/p2p_transport_serialization.cpp index 21d8dab536..6af6aa1d18 100644 --- a/src/test/fuzz/p2p_transport_serialization.cpp +++ b/src/test/fuzz/p2p_transport_serialization.cpp @@ -88,7 +88,7 @@ FUZZ_TARGET(p2p_transport_serialization, .init = initialize_p2p_transport_serial assert(msg.m_time == m_time); std::vector header; - auto msg2 = CNetMsgMaker{msg.m_recv.GetVersion()}.Make(msg.m_type, Span{msg.m_recv}); + auto msg2 = NetMsg::Make(msg.m_type, Span{msg.m_recv}); bool queued = send_transport.SetMessageToSend(msg2); assert(queued); std::optional known_more; diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 48e0706a53..3c9227cfc4 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -845,7 +845,6 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) const uint64_t services{NODE_NETWORK | NODE_WITNESS}; const int64_t time{0}; - const CNetMsgMaker msg_maker{PROTOCOL_VERSION}; // Force ChainstateManager::IsInitialBlockDownload() to return false. // Otherwise PushAddress() isn't called by PeerManager::ProcessMessage(). @@ -858,13 +857,13 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) std::chrono::microseconds time_received_dummy{0}; const auto msg_version = - msg_maker.Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, CAddress::V1_NETWORK(peer_us)); + NetMsg::Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, CAddress::V1_NETWORK(peer_us)); CDataStream msg_version_stream{msg_version.data, SER_NETWORK, PROTOCOL_VERSION}; m_node.peerman->ProcessMessage( peer, NetMsgType::VERSION, msg_version_stream, time_received_dummy, interrupt_dummy); - const auto msg_verack = msg_maker.Make(NetMsgType::VERACK); + const auto msg_verack = NetMsg::Make(NetMsgType::VERACK); CDataStream msg_verack_stream{msg_verack.data, SER_NETWORK, PROTOCOL_VERSION}; // Will set peer.fSuccessfullyConnected to true (necessary in SendMessages()). diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index e0404e33ed..9257a4964a 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -26,13 +26,12 @@ void ConnmanTestMsg::Handshake(CNode& node, { auto& peerman{static_cast(*m_msgproc)}; auto& connman{*this}; - const CNetMsgMaker mm{0}; peerman.InitializeNode(node, local_services); FlushSendBuffer(node); // Drop the version message added by InitializeNode. CSerializedNetMsg msg_version{ - mm.Make(NetMsgType::VERSION, + NetMsg::Make(NetMsgType::VERSION, version, // Using>(remote_services), // int64_t{}, // dummy time @@ -59,7 +58,7 @@ void ConnmanTestMsg::Handshake(CNode& node, assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn())); assert(statestats.their_services == remote_services); if (successfully_connected) { - CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)}; + CSerializedNetMsg msg_verack{NetMsg::Make(NetMsgType::VERACK)}; (void)connman.ReceiveMsgFrom(node, std::move(msg_verack)); node.fPauseSend = false; connman.ProcessMessagesOnce(node); From a0e3eb7549d2ba4dd3af12b9ce65e29158f59078 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 3 Nov 2023 11:01:52 +0100 Subject: [PATCH 24/62] tx fees, policy: bugfix: move `removeTx` into reason != `BLOCK` condition If the removal reason of a transaction is BLOCK, then the `removeTx` boolean argument should be true. Before this PR, `CBlockPolicyEstimator` have to complete updating the fee stats before the mempool clears that's why having removeTx call outside reason!= `BLOCK` in `addUnchecked` was not a bug. But in a case where the `CBlockPolicyEstimator` update is asynchronous, the mempool might clear before we update the `CBlockPolicyEstimator` fee stats. Transactions that are removed for `BLOCK` reasons will also be incorrectly removed from `CBlockPolicyEstimator` stats as failures. --- src/txmempool.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index e057d7ece1..7fd9c1cc25 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -497,12 +497,16 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) // even if not directly reported below. uint64_t mempool_sequence = GetAndIncrementSequence(); + const auto& hash = it->GetTx().GetHash(); if (reason != MemPoolRemovalReason::BLOCK) { // Notify clients that a transaction has been removed from the mempool // for any reason except being included in a block. Clients interested // in transactions included in blocks can subscribe to the BlockConnected // notification. GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx(), reason, mempool_sequence); + if (minerPolicyEstimator) { + minerPolicyEstimator->removeTx(hash, false); + } } TRACE5(mempool, removed, it->GetTx().GetHash().data(), @@ -512,7 +516,6 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) std::chrono::duration_cast>(it->GetTime()).count() ); - const uint256 hash = it->GetTx().GetHash(); for (const CTxIn& txin : it->GetTx().vin) mapNextTx.erase(txin.prevout); @@ -535,7 +538,6 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) cachedInnerUsage -= memusage::DynamicUsage(it->GetMemPoolParentsConst()) + memusage::DynamicUsage(it->GetMemPoolChildrenConst()); mapTx.erase(it); nTransactionsUpdated++; - if (minerPolicyEstimator) {minerPolicyEstimator->removeTx(hash, false);} } // Calculates descendants of entry that are not already in setDescendants, and adds to From 0889e07987294d4ef2814abfca16d8e2a0c5f541 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 1 Sep 2023 11:03:21 +0100 Subject: [PATCH 25/62] tx fees, policy: cast with static_cast instead of C-Style cast --- src/policy/fees.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 654c4cf0ce..df5885b059 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -610,11 +610,11 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo CFeeRate feeRate(entry.GetFee(), entry.GetTxSize()); mapMemPoolTxs[hash].blockHeight = txHeight; - unsigned int bucketIndex = feeStats->NewTx(txHeight, (double)feeRate.GetFeePerK()); + unsigned int bucketIndex = feeStats->NewTx(txHeight, static_cast(feeRate.GetFeePerK())); mapMemPoolTxs[hash].bucketIndex = bucketIndex; - unsigned int bucketIndex2 = shortStats->NewTx(txHeight, (double)feeRate.GetFeePerK()); + unsigned int bucketIndex2 = shortStats->NewTx(txHeight, static_cast(feeRate.GetFeePerK())); assert(bucketIndex == bucketIndex2); - unsigned int bucketIndex3 = longStats->NewTx(txHeight, (double)feeRate.GetFeePerK()); + unsigned int bucketIndex3 = longStats->NewTx(txHeight, static_cast(feeRate.GetFeePerK())); assert(bucketIndex == bucketIndex3); } @@ -640,9 +640,9 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM // Feerates are stored and reported as BTC-per-kb: CFeeRate feeRate(entry->GetFee(), entry->GetTxSize()); - feeStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK()); - shortStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK()); - longStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK()); + feeStats->Record(blocksToConfirm, static_cast(feeRate.GetFeePerK())); + shortStats->Record(blocksToConfirm, static_cast(feeRate.GetFeePerK())); + longStats->Record(blocksToConfirm, static_cast(feeRate.GetFeePerK())); return true; } From bfcd401368fc0dc43827a8969a37b7e038d5ca79 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 3 Nov 2023 12:34:29 +0100 Subject: [PATCH 26/62] CValidationInterface, mempool: add new callback to `CValidationInterface` This commit adds a new callback `MempoolTransactionsRemovedForBlock` which notify its listeners of the transactions that are removed from the mempool because a new block is connected, along with the block height the transactions were removed. The transactions are in `RemovedMempoolTransactionInfo` format. `CTransactionRef`, base fee, virtual size, and height which the transaction was added to the mempool are all members of the struct called `RemovedMempoolTransactionInfo`. A struct `NewMempoolTransactionInfo`, which has fields similar to `RemovedMempoolTransactionInfo`, will be added in a later commit, create a struct `TransactionInfo` with all similar fields. They can both have a member with type `TransactionInfo`. --- src/kernel/mempool_entry.h | 30 ++++++++++++++++++++++++++++++ src/txmempool.cpp | 4 ++++ src/validationinterface.cpp | 11 +++++++++++ src/validationinterface.h | 17 ++++++++++++++--- 4 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h index 7c905ca4f4..ce32fe20dc 100644 --- a/src/kernel/mempool_entry.h +++ b/src/kernel/mempool_entry.h @@ -178,4 +178,34 @@ class CTxMemPoolEntry using CTxMemPoolEntryRef = CTxMemPoolEntry::CTxMemPoolEntryRef; +struct TransactionInfo { + const CTransactionRef m_tx; + /* The fee the transaction paid */ + const CAmount m_fee; + /** + * The virtual transaction size. + * + * This is a policy field which considers the sigop cost of the + * transaction as well as its weight, and reinterprets it as bytes. + * + * It is the primary metric by which the mining algorithm selects + * transactions. + */ + const int64_t m_virtual_transaction_size; + /* The block height the transaction entered the mempool */ + const unsigned int txHeight; + + TransactionInfo(const CTransactionRef& tx, const CAmount& fee, const int64_t vsize, const unsigned int height) + : m_tx{tx}, + m_fee{fee}, + m_virtual_transaction_size{vsize}, + txHeight{height} {} +}; + +struct RemovedMempoolTransactionInfo { + TransactionInfo info; + explicit RemovedMempoolTransactionInfo(const CTxMemPoolEntry& entry) + : info{entry.GetSharedTx(), entry.GetFee(), entry.GetTxSize(), entry.GetHeight()} {} +}; + #endif // BITCOIN_KERNEL_MEMPOOL_ENTRY_H diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 7fd9c1cc25..f75dd7efea 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -654,17 +654,21 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne } // Before the txs in the new block have been removed from the mempool, update policy estimates if (minerPolicyEstimator) {minerPolicyEstimator->processBlock(nBlockHeight, entries);} + std::vector txs_removed_for_block; + txs_removed_for_block.reserve(vtx.size()); for (const auto& tx : vtx) { txiter it = mapTx.find(tx->GetHash()); if (it != mapTx.end()) { setEntries stage; stage.insert(it); + txs_removed_for_block.emplace_back(*it); RemoveStaged(stage, true, MemPoolRemovalReason::BLOCK); } removeConflicts(*tx); ClearPrioritisation(tx->GetHash()); } + GetMainSignals().MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; } diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 9241395ad5..893ef69582 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -233,6 +234,16 @@ void CMainSignals::BlockConnected(ChainstateRole role, const std::shared_ptrnHeight); } +void CMainSignals::MempoolTransactionsRemovedForBlock(const std::vector& txs_removed_for_block, unsigned int nBlockHeight) +{ + auto event = [txs_removed_for_block, nBlockHeight, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); }); + }; + ENQUEUE_AND_LOG_EVENT(event, "%s: block height=%s txs removed=%s", __func__, + nBlockHeight, + txs_removed_for_block.size()); +} + void CMainSignals::BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) { auto event = [pblock, pindex, this] { diff --git a/src/validationinterface.h b/src/validationinterface.h index eb15aa4d5f..ea49a45aa8 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -21,6 +21,7 @@ struct CBlockLocator; class CValidationInterface; class CScheduler; enum class MemPoolRemovalReason; +struct RemovedMempoolTransactionInfo; /** Register subscriber */ void RegisterValidationInterface(CValidationInterface* callbacks); @@ -60,10 +61,10 @@ void CallFunctionInValidationInterfaceQueue(std::function func); void SyncWithValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main); /** - * Implement this to subscribe to events generated in validation + * Implement this to subscribe to events generated in validation and mempool * * Each CValidationInterface() subscriber will receive event callbacks - * in the order in which the events were generated by validation. + * in the order in which the events were generated by validation and mempool. * Furthermore, each ValidationInterface() subscriber may assume that * callbacks effectively run in a single thread with single-threaded * memory consistency. That is, for a given ValidationInterface() @@ -113,7 +114,7 @@ class CValidationInterface { * This does not fire for transactions that are removed from the mempool * because they have been included in a block. Any client that is interested * in transactions removed from the mempool for inclusion in a block can learn - * about those transactions from the BlockConnected notification. + * about those transactions from the MempoolTransactionsRemovedForBlock notification. * * Transactions that are removed from the mempool because they conflict * with a transaction in the new block will have @@ -131,6 +132,14 @@ class CValidationInterface { * Called on a background thread. */ virtual void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) {} + /* + * Notifies listeners of transactions removed from the mempool as + * as a result of new block being connected. + * MempoolTransactionsRemovedForBlock will be fired before BlockConnected. + * + * Called on a background thread. + */ + virtual void MempoolTransactionsRemovedForBlock(const std::vector& txs_removed_for_block, unsigned int nBlockHeight) {} /** * Notifies listeners of a block being connected. * Provides a vector of transactions evicted from the mempool as a result. @@ -140,6 +149,7 @@ class CValidationInterface { virtual void BlockConnected(ChainstateRole role, const std::shared_ptr &block, const CBlockIndex *pindex) {} /** * Notifies listeners of a block being disconnected + * Provides the block that was connected. * * Called on a background thread. Only called for the active chainstate, since * background chainstates should never disconnect blocks. @@ -202,6 +212,7 @@ class CMainSignals { void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); void TransactionAddedToMempool(const CTransactionRef&, uint64_t mempool_sequence); void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason, uint64_t mempool_sequence); + void MempoolTransactionsRemovedForBlock(const std::vector&, unsigned int nBlockHeight); void BlockConnected(ChainstateRole, const std::shared_ptr &, const CBlockIndex *pindex); void BlockDisconnected(const std::shared_ptr &, const CBlockIndex* pindex); void ChainStateFlushed(ChainstateRole, const CBlockLocator &); From 91532bd38223d7d04166e05de11d0d0b55e60f13 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 3 Nov 2023 13:23:30 +0100 Subject: [PATCH 27/62] tx fees, policy: update `CBlockPolicyEstimator::processBlock` parameter Update `processBlock` parameter to reference to a vector of `RemovedMempoolTransactionInfo`. --- src/policy/fees.cpp | 18 +++++++++--------- src/policy/fees.h | 7 ++++--- src/test/fuzz/policy_estimator.cpp | 8 ++++---- src/txmempool.cpp | 15 ++++----------- 4 files changed, 21 insertions(+), 27 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index df5885b059..6d550b1d5a 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -618,10 +618,10 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo assert(bucketIndex == bucketIndex3); } -bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry) +bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const RemovedMempoolTransactionInfo& tx) { AssertLockHeld(m_cs_fee_estimator); - if (!_removeTx(entry->GetTx().GetHash(), true)) { + if (!_removeTx(tx.info.m_tx->GetHash(), true)) { // This transaction wasn't being tracked for fee estimation return false; } @@ -629,7 +629,7 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM // How many blocks did it take for miners to include this transaction? // blocksToConfirm is 1-based, so a transaction included in the earliest // possible block has confirmation count of 1 - int blocksToConfirm = nBlockHeight - entry->GetHeight(); + int blocksToConfirm = nBlockHeight - tx.info.txHeight; if (blocksToConfirm <= 0) { // This can't happen because we don't process transactions from a block with a height // lower than our greatest seen height @@ -638,7 +638,7 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM } // Feerates are stored and reported as BTC-per-kb: - CFeeRate feeRate(entry->GetFee(), entry->GetTxSize()); + CFeeRate feeRate(tx.info.m_fee, tx.info.m_virtual_transaction_size); feeStats->Record(blocksToConfirm, static_cast(feeRate.GetFeePerK())); shortStats->Record(blocksToConfirm, static_cast(feeRate.GetFeePerK())); @@ -646,8 +646,8 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM return true; } -void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, - std::vector& entries) +void CBlockPolicyEstimator::processBlock(const std::vector& txs_removed_for_block, + unsigned int nBlockHeight) { LOCK(m_cs_fee_estimator); if (nBlockHeight <= nBestSeenHeight) { @@ -676,8 +676,8 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, unsigned int countedTxs = 0; // Update averages with data points from current block - for (const auto& entry : entries) { - if (processBlockTx(nBlockHeight, entry)) + for (const auto& tx : txs_removed_for_block) { + if (processBlockTx(nBlockHeight, tx)) countedTxs++; } @@ -688,7 +688,7 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, LogPrint(BCLog::ESTIMATEFEE, "Blockpolicy estimates updated by %u of %u block txs, since last block %u of %u tracked, mempool map size %u, max target %u from %s\n", - countedTxs, entries.size(), trackedTxs, trackedTxs + untrackedTxs, mapMemPoolTxs.size(), + countedTxs, txs_removed_for_block.size(), trackedTxs, trackedTxs + untrackedTxs, mapMemPoolTxs.size(), MaxUsableEstimate(), HistoricalBlockSpan() > BlockSpan() ? "historical" : "current"); trackedTxs = 0; diff --git a/src/policy/fees.h b/src/policy/fees.h index 69bda195be..cff0c4dbe2 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -37,6 +37,7 @@ static constexpr bool DEFAULT_ACCEPT_STALE_FEE_ESTIMATES{false}; class AutoFile; class CTxMemPoolEntry; class TxConfirmStats; +struct RemovedMempoolTransactionInfo; /* Identifier for each of the 3 different TxConfirmStats which will track * history over different time horizons. */ @@ -201,8 +202,8 @@ class CBlockPolicyEstimator ~CBlockPolicyEstimator(); /** Process all the transactions that have been included in a block */ - void processBlock(unsigned int nBlockHeight, - std::vector& entries) + void processBlock(const std::vector& txs_removed_for_block, + unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Process a transaction accepted to the mempool*/ @@ -290,7 +291,7 @@ class CBlockPolicyEstimator std::map bucketMap GUARDED_BY(m_cs_fee_estimator); // Map of bucket upper-bound to index into all vectors by bucket /** Process a transaction confirmed in a block*/ - bool processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry) EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator); + bool processBlockTx(unsigned int nBlockHeight, const RemovedMempoolTransactionInfo& tx) EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator); /** Helper for estimateSmartFee */ double estimateCombinedFee(unsigned int confTarget, double successThreshold, bool checkShorterHorizon, EstimationResult *result) const EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator); diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp index 220799be41..e5e0b0d8da 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -61,12 +61,12 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) const CTransaction tx{*mtx}; mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx)); } - std::vector ptrs; - ptrs.reserve(mempool_entries.size()); + std::vector txs; + txs.reserve(mempool_entries.size()); for (const CTxMemPoolEntry& mempool_entry : mempool_entries) { - ptrs.push_back(&mempool_entry); + txs.emplace_back(mempool_entry); } - block_policy_estimator.processBlock(fuzzed_data_provider.ConsumeIntegral(), ptrs); + block_policy_estimator.processBlock(txs, fuzzed_data_provider.ConsumeIntegral()); }, [&] { (void)block_policy_estimator.removeTx(ConsumeUInt256(fuzzed_data_provider), /*inBlock=*/fuzzed_data_provider.ConsumeBool()); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index f75dd7efea..f5036a9301 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -643,17 +643,6 @@ void CTxMemPool::removeConflicts(const CTransaction &tx) void CTxMemPool::removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) { AssertLockHeld(cs); - std::vector entries; - for (const auto& tx : vtx) - { - uint256 hash = tx->GetHash(); - - indexed_transaction_set::iterator i = mapTx.find(hash); - if (i != mapTx.end()) - entries.push_back(&*i); - } - // Before the txs in the new block have been removed from the mempool, update policy estimates - if (minerPolicyEstimator) {minerPolicyEstimator->processBlock(nBlockHeight, entries);} std::vector txs_removed_for_block; txs_removed_for_block.reserve(vtx.size()); for (const auto& tx : vtx) @@ -668,6 +657,10 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne removeConflicts(*tx); ClearPrioritisation(tx->GetHash()); } + // Update policy estimates + if (minerPolicyEstimator) { + minerPolicyEstimator->processBlock(txs_removed_for_block, nBlockHeight); + } GetMainSignals().MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; From dff5ad3b9944cbb56126ba37a8da180d1327ba39 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 3 Nov 2023 17:04:30 +0100 Subject: [PATCH 28/62] CValidationInterface: modify the parameter of `TransactionAddedToMempool` Create a new struct `NewMempoolTransactionInfo` that will be used as the new parameter of `TransactionAddedToMempool` callback. --- src/kernel/mempool_entry.h | 29 ++++++++++++++++++++++++++++ src/node/interfaces.cpp | 4 ++-- src/test/fuzz/package_eval.cpp | 10 +++++----- src/test/fuzz/tx_pool.cpp | 4 ++-- src/validation.cpp | 16 +++++++++++++-- src/validationinterface.cpp | 7 ++++--- src/validationinterface.h | 5 +++-- src/zmq/zmqnotificationinterface.cpp | 5 +++-- src/zmq/zmqnotificationinterface.h | 3 ++- 9 files changed, 64 insertions(+), 19 deletions(-) diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h index ce32fe20dc..5824a4576b 100644 --- a/src/kernel/mempool_entry.h +++ b/src/kernel/mempool_entry.h @@ -208,4 +208,33 @@ struct RemovedMempoolTransactionInfo { : info{entry.GetSharedTx(), entry.GetFee(), entry.GetTxSize(), entry.GetHeight()} {} }; +struct NewMempoolTransactionInfo { + TransactionInfo info; + /* + * This boolean indicates whether the transaction was added + * without enforcing mempool fee limits. + */ + const bool m_from_disconnected_block; + /* This boolean indicates whether the transaction is part of a package. */ + const bool m_submitted_in_package; + /* + * This boolean indicates whether the blockchain is up to date when the + * transaction is added to the mempool. + */ + const bool m_chainstate_is_current; + /* Indicates whether the transaction has unconfirmed parents. */ + const bool m_has_no_mempool_parents; + + explicit NewMempoolTransactionInfo(const CTransactionRef& tx, const CAmount& fee, + const int64_t vsize, const unsigned int height, + const bool from_disconnected_block, const bool submitted_in_package, + const bool chainstate_is_current, + const bool has_no_mempool_parents) + : info{tx, fee, vsize, height}, + m_from_disconnected_block{from_disconnected_block}, + m_submitted_in_package{submitted_in_package}, + m_chainstate_is_current{chainstate_is_current}, + m_has_no_mempool_parents{has_no_mempool_parents} {} +}; + #endif // BITCOIN_KERNEL_MEMPOOL_ENTRY_H diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index f4ecfeb9d5..b5691f0e57 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -428,9 +428,9 @@ class NotificationsProxy : public CValidationInterface explicit NotificationsProxy(std::shared_ptr notifications) : m_notifications(std::move(notifications)) {} virtual ~NotificationsProxy() = default; - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) override + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) override { - m_notifications->transactionAddedToMempool(tx); + m_notifications->transactionAddedToMempool(tx.info.m_tx); } void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override { diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 8658c0b45a..021acc02f2 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -55,13 +55,13 @@ struct OutpointsUpdater final : public CValidationInterface { explicit OutpointsUpdater(std::set& r) : m_mempool_outpoints{r} {} - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t /* mempool_sequence */) override + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /* mempool_sequence */) override { // for coins spent we always want to be able to rbf so they're not removed // outputs from this tx can now be spent - for (uint32_t index{0}; index < tx->vout.size(); ++index) { - m_mempool_outpoints.insert(COutPoint{tx->GetHash(), index}); + for (uint32_t index{0}; index < tx.info.m_tx->vout.size(); ++index) { + m_mempool_outpoints.insert(COutPoint{tx.info.m_tx->GetHash(), index}); } } @@ -85,10 +85,10 @@ struct TransactionsDelta final : public CValidationInterface { explicit TransactionsDelta(std::set& a) : m_added{a} {} - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t /* mempool_sequence */) override + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /* mempool_sequence */) override { // Transactions may be entered and booted any number of times - m_added.insert(tx); + m_added.insert(tx.info.m_tx); } void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 96095539ec..001b452722 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -59,9 +59,9 @@ struct TransactionsDelta final : public CValidationInterface { explicit TransactionsDelta(std::set& r, std::set& a) : m_removed{r}, m_added{a} {} - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t /* mempool_sequence */) override + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /* mempool_sequence */) override { - Assert(m_added.insert(tx).second); + Assert(m_added.insert(tx.info.m_tx).second); } void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override diff --git a/src/validation.cpp b/src/validation.cpp index ed72a7c97a..1c95ba08c5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1222,7 +1222,13 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate, effective_feerate_wtxids)); - GetMainSignals().TransactionAddedToMempool(ws.m_ptx, m_pool.GetAndIncrementSequence()); + const CTransaction& tx = *ws.m_ptx; + const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, + ws.m_vsize, ws.m_entry->GetHeight(), + args.m_bypass_limits, args.m_package_submission, + IsCurrentForFeeEstimation(m_active_chainstate), + m_pool.HasNoInputsOf(tx)); + GetMainSignals().TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); } return all_submitted; } @@ -1265,7 +1271,13 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), {ws.m_ptx->GetWitnessHash()}); } - GetMainSignals().TransactionAddedToMempool(ptx, m_pool.GetAndIncrementSequence()); + const CTransaction& tx = *ws.m_ptx; + const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, + ws.m_vsize, ws.m_entry->GetHeight(), + args.m_bypass_limits, args.m_package_submission, + IsCurrentForFeeEstimation(m_active_chainstate), + m_pool.HasNoInputsOf(tx)); + GetMainSignals().TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate, single_wtxid); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 893ef69582..5e944a7c47 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -206,13 +206,14 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd fInitialDownload); } -void CMainSignals::TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) { +void CMainSignals::TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) +{ auto event = [tx, mempool_sequence, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(tx, mempool_sequence); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s wtxid=%s", __func__, - tx->GetHash().ToString(), - tx->GetWitnessHash().ToString()); + tx.info.m_tx->GetHash().ToString(), + tx.info.m_tx->GetWitnessHash().ToString()); } void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) { diff --git a/src/validationinterface.h b/src/validationinterface.h index ea49a45aa8..e1d6869fab 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -22,6 +22,7 @@ class CValidationInterface; class CScheduler; enum class MemPoolRemovalReason; struct RemovedMempoolTransactionInfo; +struct NewMempoolTransactionInfo; /** Register subscriber */ void RegisterValidationInterface(CValidationInterface* callbacks); @@ -97,7 +98,7 @@ class CValidationInterface { * * Called on a background thread. */ - virtual void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) {} + virtual void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) {} /** * Notifies listeners of a transaction leaving mempool. @@ -210,7 +211,7 @@ class CMainSignals { void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); - void TransactionAddedToMempool(const CTransactionRef&, uint64_t mempool_sequence); + void TransactionAddedToMempool(const NewMempoolTransactionInfo&, uint64_t mempool_sequence); void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason, uint64_t mempool_sequence); void MempoolTransactionsRemovedForBlock(const std::vector&, unsigned int nBlockHeight); void BlockConnected(ChainstateRole, const std::shared_ptr &, const CBlockIndex *pindex); diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 03aae86577..63c2737706 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -152,9 +153,9 @@ void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, co }); } -void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx, uint64_t mempool_sequence) +void CZMQNotificationInterface::TransactionAddedToMempool(const NewMempoolTransactionInfo& ptx, uint64_t mempool_sequence) { - const CTransaction& tx = *ptx; + const CTransaction& tx = *(ptx.info.m_tx); TryForEachAndRemoveFailed(notifiers, [&tx, mempool_sequence](CZMQAbstractNotifier* notifier) { return notifier->NotifyTransaction(tx) && notifier->NotifyTransactionAcceptance(tx, mempool_sequence); diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index 4246c53bd3..45d0982bd3 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -16,6 +16,7 @@ class CBlock; class CBlockIndex; class CZMQAbstractNotifier; +struct NewMempoolTransactionInfo; class CZMQNotificationInterface final : public CValidationInterface { @@ -31,7 +32,7 @@ class CZMQNotificationInterface final : public CValidationInterface void Shutdown(); // CValidationInterface - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) override; + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) override; void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override; void BlockConnected(ChainstateRole role, const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override; void BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex* pindexDisconnected) override; From 714523918ba2b853fc69bee6b04a33ba0c828bf5 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 3 Nov 2023 18:34:58 +0100 Subject: [PATCH 29/62] tx fees, policy: CBlockPolicyEstimator update from `CValidationInterface` notifications `CBlockPolicyEstimator` will implement `CValidationInterface` and subscribe to its notification to process transactions added and removed from the mempool. Re-delegate calculation of `validForFeeEstimation` from validation to fee estimator. Also clean up the validForFeeEstimation arg thats no longer needed in `CTxMempool`. Co-authored-by: Matt Corallo --- src/Makefile.am | 1 - src/init.cpp | 10 ++- src/kernel/mempool_options.h | 4 -- src/policy/fees.cpp | 40 ++++++++---- src/policy/fees.h | 22 +++++-- src/test/fuzz/package_eval.cpp | 1 - src/test/fuzz/policy_estimator.cpp | 13 +++- src/test/fuzz/tx_pool.cpp | 1 - src/test/policyestimator_tests.cpp | 97 +++++++++++++++++++++++++++--- src/test/util/txmempool.cpp | 1 - src/txmempool.cpp | 24 ++------ src/txmempool.h | 7 +-- src/validation.cpp | 11 +--- 13 files changed, 158 insertions(+), 74 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 99b2184cf2..b746a931c1 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -954,7 +954,6 @@ libbitcoinkernel_la_SOURCES = \ node/chainstate.cpp \ node/utxo_snapshot.cpp \ policy/feerate.cpp \ - policy/fees.cpp \ policy/packages.cpp \ policy/policy.cpp \ policy/rbf.cpp \ diff --git a/src/init.cpp b/src/init.cpp index d6dc62f707..92aa187626 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -285,8 +285,12 @@ void Shutdown(NodeContext& node) DumpMempool(*node.mempool, MempoolPath(*node.args)); } - // Drop transactions we were still watching, and record fee estimations. - if (node.fee_estimator) node.fee_estimator->Flush(); + // Drop transactions we were still watching, record fee estimations and Unregister + // fee estimator from validation interface. + if (node.fee_estimator) { + node.fee_estimator->Flush(); + UnregisterValidationInterface(node.fee_estimator.get()); + } // FlushStateToDisk generates a ChainStateFlushed callback, which we should avoid missing if (node.chainman) { @@ -1258,6 +1262,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // Flush estimates to disk periodically CBlockPolicyEstimator* fee_estimator = node.fee_estimator.get(); node.scheduler->scheduleEvery([fee_estimator] { fee_estimator->FlushFeeEstimates(); }, FEE_FLUSH_INTERVAL); + RegisterValidationInterface(fee_estimator); } // Check port numbers @@ -1471,7 +1476,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) assert(!node.chainman); CTxMemPool::Options mempool_opts{ - .estimator = node.fee_estimator.get(), .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0, }; auto result{ApplyArgsManOptions(args, chainparams, mempool_opts)}; diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h index d09fd2ba35..753aebd455 100644 --- a/src/kernel/mempool_options.h +++ b/src/kernel/mempool_options.h @@ -13,8 +13,6 @@ #include #include -class CBlockPolicyEstimator; - /** Default for -maxmempool, maximum megabytes of mempool memory usage */ static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300}; /** Default for -maxmempool when blocksonly is set */ @@ -37,8 +35,6 @@ namespace kernel { * Most of the time, this struct should be referenced as CTxMemPool::Options. */ struct MemPoolOptions { - /* Used to estimate appropriate transaction fees. */ - CBlockPolicyEstimator* estimator{nullptr}; /* The ratio used to determine how often sanity checks will run. */ int check_ratio{0}; int64_t max_size_bytes{DEFAULT_MAX_MEMPOOL_SIZE_MB * 1'000'000}; diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 6d550b1d5a..74c688060d 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -515,15 +515,10 @@ void TxConfirmStats::removeTx(unsigned int entryHeight, unsigned int nBestSeenHe } } -// This function is called from CTxMemPool::removeUnchecked to ensure -// txs removed from the mempool for any reason are no longer -// tracked. Txs that were part of a block have already been removed in -// processBlockTx to ensure they are never double tracked, but it is -// of no harm to try to remove them again. -bool CBlockPolicyEstimator::removeTx(uint256 hash, bool inBlock) +bool CBlockPolicyEstimator::removeTx(uint256 hash) { LOCK(m_cs_fee_estimator); - return _removeTx(hash, inBlock); + return _removeTx(hash, /*inBlock=*/false); } bool CBlockPolicyEstimator::_removeTx(const uint256& hash, bool inBlock) @@ -579,11 +574,26 @@ CBlockPolicyEstimator::CBlockPolicyEstimator(const fs::path& estimation_filepath CBlockPolicyEstimator::~CBlockPolicyEstimator() = default; -void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate) +void CBlockPolicyEstimator::TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /*unused*/) +{ + processTransaction(tx); +} + +void CBlockPolicyEstimator::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason /*unused*/, uint64_t /*unused*/) +{ + removeTx(tx->GetHash()); +} + +void CBlockPolicyEstimator::MempoolTransactionsRemovedForBlock(const std::vector& txs_removed_for_block, unsigned int nBlockHeight) +{ + processBlock(txs_removed_for_block, nBlockHeight); +} + +void CBlockPolicyEstimator::processTransaction(const NewMempoolTransactionInfo& tx) { LOCK(m_cs_fee_estimator); - unsigned int txHeight = entry.GetHeight(); - uint256 hash = entry.GetTx().GetHash(); + const unsigned int txHeight = tx.info.txHeight; + const auto& hash = tx.info.m_tx->GetHash(); if (mapMemPoolTxs.count(hash)) { LogPrint(BCLog::ESTIMATEFEE, "Blockpolicy error mempool tx %s already being tracked\n", hash.ToString()); @@ -597,17 +607,23 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo // It will be synced next time a block is processed. return; } + // This transaction should only count for fee estimation if: + // - it's not being re-added during a reorg which bypasses typical mempool fee limits + // - the node is not behind + // - the transaction is not dependent on any other transactions in the mempool + // - it's not part of a package. + const bool validForFeeEstimation = !tx.m_from_disconnected_block && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents; // Only want to be updating estimates when our blockchain is synced, // otherwise we'll miscalculate how many blocks its taking to get included. - if (!validFeeEstimate) { + if (!validForFeeEstimation) { untrackedTxs++; return; } trackedTxs++; // Feerates are stored and reported as BTC-per-kb: - CFeeRate feeRate(entry.GetFee(), entry.GetTxSize()); + const CFeeRate feeRate(tx.info.m_fee, tx.info.m_virtual_transaction_size); mapMemPoolTxs[hash].blockHeight = txHeight; unsigned int bucketIndex = feeStats->NewTx(txHeight, static_cast(feeRate.GetFeePerK())); diff --git a/src/policy/fees.h b/src/policy/fees.h index cff0c4dbe2..f34f66d3f0 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -35,9 +36,9 @@ static constexpr std::chrono::hours MAX_FILE_AGE{60}; static constexpr bool DEFAULT_ACCEPT_STALE_FEE_ESTIMATES{false}; class AutoFile; -class CTxMemPoolEntry; class TxConfirmStats; struct RemovedMempoolTransactionInfo; +struct NewMempoolTransactionInfo; /* Identifier for each of the 3 different TxConfirmStats which will track * history over different time horizons. */ @@ -144,7 +145,7 @@ struct FeeCalculation * a certain number of blocks. Every time a block is added to the best chain, this class records * stats on the transactions included in that block */ -class CBlockPolicyEstimator +class CBlockPolicyEstimator : public CValidationInterface { private: /** Track confirm delays up to 12 blocks for short horizon */ @@ -199,7 +200,7 @@ class CBlockPolicyEstimator public: /** Create new BlockPolicyEstimator and initialize stats tracking classes with default values */ CBlockPolicyEstimator(const fs::path& estimation_filepath, const bool read_stale_estimates); - ~CBlockPolicyEstimator(); + virtual ~CBlockPolicyEstimator(); /** Process all the transactions that have been included in a block */ void processBlock(const std::vector& txs_removed_for_block, @@ -207,11 +208,11 @@ class CBlockPolicyEstimator EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Process a transaction accepted to the mempool*/ - void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate) + void processTransaction(const NewMempoolTransactionInfo& tx) EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); - /** Remove a transaction from the mempool tracking stats*/ - bool removeTx(uint256 hash, bool inBlock) + /** Remove a transaction from the mempool tracking stats for non BLOCK removal reasons*/ + bool removeTx(uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** DEPRECATED. Return a feerate estimate */ @@ -261,6 +262,15 @@ class CBlockPolicyEstimator /** Calculates the age of the file, since last modified */ std::chrono::hours GetFeeEstimatorFileAge(); +protected: + /** Overridden from CValidationInterface. */ + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /*unused*/) override + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); + void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason /*unused*/, uint64_t /*unused*/) override + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); + void MempoolTransactionsRemovedForBlock(const std::vector& txs_removed_for_block, unsigned int nBlockHeight) override + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); + private: mutable Mutex m_cs_fee_estimator; diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 021acc02f2..08b8be26e7 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -121,7 +121,6 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte mempool_opts.expiry = std::chrono::hours{fuzzed_data_provider.ConsumeIntegralInRange(0, 999)}; nBytesPerSigOp = fuzzed_data_provider.ConsumeIntegralInRange(1, 999); - mempool_opts.estimator = nullptr; mempool_opts.check_ratio = 1; mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool(); diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp index e5e0b0d8da..4a41707edf 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -44,9 +44,16 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) return; } const CTransaction tx{*mtx}; - block_policy_estimator.processTransaction(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx), fuzzed_data_provider.ConsumeBool()); + const CTxMemPoolEntry& entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, tx); + const auto tx_info = NewMempoolTransactionInfo(entry.GetSharedTx(), entry.GetFee(), + entry.GetTxSize(), entry.GetHeight(), + /* m_from_disconnected_block */ false, + /* m_submitted_in_package */ false, + /* m_chainstate_is_current */ true, + /* m_has_no_mempool_parents */ fuzzed_data_provider.ConsumeBool()); + block_policy_estimator.processTransaction(tx_info); if (fuzzed_data_provider.ConsumeBool()) { - (void)block_policy_estimator.removeTx(tx.GetHash(), /*inBlock=*/fuzzed_data_provider.ConsumeBool()); + (void)block_policy_estimator.removeTx(tx.GetHash()); } }, [&] { @@ -69,7 +76,7 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) block_policy_estimator.processBlock(txs, fuzzed_data_provider.ConsumeIntegral()); }, [&] { - (void)block_policy_estimator.removeTx(ConsumeUInt256(fuzzed_data_provider), /*inBlock=*/fuzzed_data_provider.ConsumeBool()); + (void)block_policy_estimator.removeTx(ConsumeUInt256(fuzzed_data_provider)); }, [&] { block_policy_estimator.FlushUnconfirmed(); diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 001b452722..cc816f213b 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -123,7 +123,6 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)}; // ...override specific options for this specific fuzz suite - mempool_opts.estimator = nullptr; mempool_opts.check_ratio = 1; mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool(); diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index bc9c672560..13ec89663a 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include @@ -19,7 +20,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) { CBlockPolicyEstimator& feeEst = *Assert(m_node.fee_estimator); CTxMemPool& mpool = *Assert(m_node.mempool); - LOCK2(cs_main, mpool.cs); + RegisterValidationInterface(&feeEst); TestMemPoolEntryHelper entry; CAmount basefee(2000); CAmount deltaFee(100); @@ -59,8 +60,23 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (int j = 0; j < 10; j++) { // For each fee for (int k = 0; k < 4; k++) { // add 4 fee txs tx.vin[0].prevout.n = 10000*blocknum+100*j+k; // make transaction unique + { + LOCK2(cs_main, mpool.cs); + mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); + // Since TransactionAddedToMempool callbacks are generated in ATMP, + // not addUnchecked, we cheat and create one manually here + const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); + const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), + feeV[j], + virtual_size, + entry.nHeight, + /* m_from_disconnected_block */ false, + /* m_submitted_in_package */ false, + /* m_chainstate_is_current */ true, + /* m_has_no_mempool_parents */ true)}; + GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); + } uint256 hash = tx.GetHash(); - mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); txHashes[j].push_back(hash); } } @@ -76,10 +92,17 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) txHashes[9-h].pop_back(); } } - mpool.removeForBlock(block, ++blocknum); + + { + LOCK(mpool.cs); + mpool.removeForBlock(block, ++blocknum); + } + block.clear(); // Check after just a few txs that combining buckets works as expected if (blocknum == 3) { + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); // At this point we should need to combine 3 buckets to get enough data points // So estimateFee(1) should fail and estimateFee(2) should return somewhere around // 9*baserate. estimateFee(2) %'s are 100,100,90 = average 97% @@ -114,8 +137,13 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) // Mine 50 more blocks with no transactions happening, estimates shouldn't change // We haven't decayed the moving average enough so we still have enough data points in every bucket - while (blocknum < 250) + while (blocknum < 250) { + LOCK(mpool.cs); mpool.removeForBlock(block, ++blocknum); + } + + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); BOOST_CHECK(feeEst.estimateFee(1) == CFeeRate(0)); for (int i = 2; i < 10;i++) { @@ -130,14 +158,35 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (int j = 0; j < 10; j++) { // For each fee multiple for (int k = 0; k < 4; k++) { // add 4 fee txs tx.vin[0].prevout.n = 10000*blocknum+100*j+k; + { + LOCK2(cs_main, mpool.cs); + mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); + // Since TransactionAddedToMempool callbacks are generated in ATMP, + // not addUnchecked, we cheat and create one manually here + const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); + const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), + feeV[j], + virtual_size, + entry.nHeight, + /* m_from_disconnected_block */ false, + /* m_submitted_in_package */ false, + /* m_chainstate_is_current */ true, + /* m_has_no_mempool_parents */ true)}; + GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); + } uint256 hash = tx.GetHash(); - mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); txHashes[j].push_back(hash); } } - mpool.removeForBlock(block, ++blocknum); + { + LOCK(mpool.cs); + mpool.removeForBlock(block, ++blocknum); + } } + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); + for (int i = 1; i < 10;i++) { BOOST_CHECK(feeEst.estimateFee(i) == CFeeRate(0) || feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee); } @@ -152,8 +201,16 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) txHashes[j].pop_back(); } } - mpool.removeForBlock(block, 266); + + { + LOCK(mpool.cs); + mpool.removeForBlock(block, 266); + } block.clear(); + + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); + BOOST_CHECK(feeEst.estimateFee(1) == CFeeRate(0)); for (int i = 2; i < 10;i++) { BOOST_CHECK(feeEst.estimateFee(i) == CFeeRate(0) || feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee); @@ -165,17 +222,39 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (int j = 0; j < 10; j++) { // For each fee multiple for (int k = 0; k < 4; k++) { // add 4 fee txs tx.vin[0].prevout.n = 10000*blocknum+100*j+k; + { + LOCK2(cs_main, mpool.cs); + mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); + // Since TransactionAddedToMempool callbacks are generated in ATMP, + // not addUnchecked, we cheat and create one manually here + const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); + const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), + feeV[j], + virtual_size, + entry.nHeight, + /* m_from_disconnected_block */ false, + /* m_submitted_in_package */ false, + /* m_chainstate_is_current */ true, + /* m_has_no_mempool_parents */ true)}; + GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); + } uint256 hash = tx.GetHash(); - mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); CTransactionRef ptx = mpool.get(hash); if (ptx) block.push_back(ptx); } } - mpool.removeForBlock(block, ++blocknum); + + { + LOCK(mpool.cs); + mpool.removeForBlock(block, ++blocknum); + } + block.clear(); } + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); BOOST_CHECK(feeEst.estimateFee(1) == CFeeRate(0)); for (int i = 2; i < 9; i++) { // At 9, the original estimate was already at the bottom (b/c scale = 2) BOOST_CHECK(feeEst.estimateFee(i).GetFeePerK() < origFeeEst[i-1] - deltaFee); diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index 6d3bb01be8..379c3c9329 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -18,7 +18,6 @@ using node::NodeContext; CTxMemPool::Options MemPoolOptionsForTest(const NodeContext& node) { CTxMemPool::Options mempool_opts{ - .estimator = node.fee_estimator.get(), // Default to always checking mempool regardless of // chainparams.DefaultConsistencyChecks for tests .check_ratio = 1, diff --git a/src/txmempool.cpp b/src/txmempool.cpp index f5036a9301..8a2af807df 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -12,9 +12,9 @@ #include #include #include -#include #include #include +#include #include #include #include @@ -402,7 +402,6 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee, CTxMemPool::CTxMemPool(const Options& opts) : m_check_ratio{opts.check_ratio}, - minerPolicyEstimator{opts.estimator}, m_max_size_bytes{opts.max_size_bytes}, m_expiry{opts.expiry}, m_incremental_relay_feerate{opts.incremental_relay_feerate}, @@ -433,7 +432,7 @@ void CTxMemPool::AddTransactionsUpdated(unsigned int n) nTransactionsUpdated += n; } -void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAncestors, bool validFeeEstimate) +void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAncestors) { // Add to memory pool without checking anything. // Used by AcceptToMemoryPool(), which DOES do @@ -477,9 +476,6 @@ void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAnces nTransactionsUpdated++; totalTxSize += entry.GetTxSize(); m_total_fee += entry.GetFee(); - if (minerPolicyEstimator) { - minerPolicyEstimator->processTransaction(entry, validFeeEstimate); - } txns_randomized.emplace_back(newit->GetSharedTx()); newit->idx_randomized = txns_randomized.size() - 1; @@ -497,16 +493,12 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) // even if not directly reported below. uint64_t mempool_sequence = GetAndIncrementSequence(); - const auto& hash = it->GetTx().GetHash(); if (reason != MemPoolRemovalReason::BLOCK) { // Notify clients that a transaction has been removed from the mempool // for any reason except being included in a block. Clients interested // in transactions included in blocks can subscribe to the BlockConnected // notification. GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx(), reason, mempool_sequence); - if (minerPolicyEstimator) { - minerPolicyEstimator->removeTx(hash, false); - } } TRACE5(mempool, removed, it->GetTx().GetHash().data(), @@ -519,7 +511,7 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) for (const CTxIn& txin : it->GetTx().vin) mapNextTx.erase(txin.prevout); - RemoveUnbroadcastTx(hash, true /* add logging because unchecked */ ); + RemoveUnbroadcastTx(it->GetTx().GetHash(), true /* add logging because unchecked */); if (txns_randomized.size() > 1) { // Update idx_randomized of the to-be-moved entry. @@ -638,7 +630,7 @@ void CTxMemPool::removeConflicts(const CTransaction &tx) } /** - * Called when a block is connected. Removes from mempool and updates the miner fee estimator. + * Called when a block is connected. Removes from mempool. */ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) { @@ -657,10 +649,6 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne removeConflicts(*tx); ClearPrioritisation(tx->GetHash()); } - // Update policy estimates - if (minerPolicyEstimator) { - minerPolicyEstimator->processBlock(txs_removed_for_block, nBlockHeight); - } GetMainSignals().MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; @@ -1091,10 +1079,10 @@ int CTxMemPool::Expire(std::chrono::seconds time) return stage.size(); } -void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, bool validFeeEstimate) +void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry) { auto ancestors{AssumeCalculateMemPoolAncestors(__func__, entry, Limits::NoLimits())}; - return addUnchecked(entry, ancestors, validFeeEstimate); + return addUnchecked(entry, ancestors); } void CTxMemPool::UpdateChild(txiter entry, txiter child, bool add) diff --git a/src/txmempool.h b/src/txmempool.h index aed6acd9da..bac7a445d6 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -202,8 +202,6 @@ struct entry_time {}; struct ancestor_score {}; struct index_by_wtxid {}; -class CBlockPolicyEstimator; - /** * Information about a mempool transaction. */ @@ -303,7 +301,6 @@ class CTxMemPool protected: const int m_check_ratio; //!< Value n means that 1 times in n we check. std::atomic nTransactionsUpdated{0}; //!< Used by getblocktemplate to trigger CreateNewBlock() invocation - CBlockPolicyEstimator* const minerPolicyEstimator; uint64_t totalTxSize GUARDED_BY(cs){0}; //!< sum of all mempool tx's virtual sizes. Differs from serialized tx size since witness data is discounted. Defined in BIP 141. CAmount m_total_fee GUARDED_BY(cs){0}; //!< sum of all mempool tx's fees (NOT modified fee) @@ -472,8 +469,8 @@ class CTxMemPool // Note that addUnchecked is ONLY called from ATMP outside of tests // and any other callers may break wallet's in-mempool tracking (due to // lack of CValidationInterface::TransactionAddedToMempool callbacks). - void addUnchecked(const CTxMemPoolEntry& entry, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); - void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); + void addUnchecked(const CTxMemPoolEntry& entry) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); + void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); /** After reorg, filter the entries that would no longer be valid in the next block, and update diff --git a/src/validation.cpp b/src/validation.cpp index 1c95ba08c5..f9e5d1db82 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1126,17 +1126,8 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) ws.m_replaced_transactions.push_back(it->GetSharedTx()); } m_pool.RemoveStaged(ws.m_all_conflicting, false, MemPoolRemovalReason::REPLACED); - - // This transaction should only count for fee estimation if: - // - it's not being re-added during a reorg which bypasses typical mempool fee limits - // - the node is not behind - // - the transaction is not dependent on any other transactions in the mempool - // - it's not part of a package. Since package relay is not currently supported, this - // transaction has not necessarily been accepted to miners' mempools. - bool validForFeeEstimation = !bypass_limits && !args.m_package_submission && IsCurrentForFeeEstimation(m_active_chainstate) && m_pool.HasNoInputsOf(tx); - // Store transaction in memory - m_pool.addUnchecked(*entry, ws.m_ancestors, validForFeeEstimation); + m_pool.addUnchecked(*entry, ws.m_ancestors); // trim mempool and check if tx was trimmed // If we are validating a package, don't trim here because we could evict a previous transaction From 91504cbe0de2b74ef1aa2709761aaf0597ec66a2 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Wed, 8 Nov 2023 11:21:49 +0100 Subject: [PATCH 30/62] rpc: `SyncWithValidationInterfaceQueue` on fee estimation RPC's This ensures that the most recent fee estimation data is used for the fee estimation with `estimateSmartfee` and `estimaterawfee` RPC's. --- src/rpc/fees.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/rpc/fees.cpp b/src/rpc/fees.cpp index 62396d4c58..57ba486ed9 100644 --- a/src/rpc/fees.cpp +++ b/src/rpc/fees.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -67,6 +68,7 @@ static RPCHelpMan estimatesmartfee() const NodeContext& node = EnsureAnyNodeContext(request.context); const CTxMemPool& mempool = EnsureMemPool(node); + SyncWithValidationInterfaceQueue(); unsigned int max_target = fee_estimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE); unsigned int conf_target = ParseConfirmTarget(request.params[0], max_target); bool conservative = true; @@ -155,6 +157,7 @@ static RPCHelpMan estimaterawfee() { CBlockPolicyEstimator& fee_estimator = EnsureAnyFeeEstimator(request.context); + SyncWithValidationInterfaceQueue(); unsigned int max_target = fee_estimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE); unsigned int conf_target = ParseConfirmTarget(request.params[0], max_target); double threshold = 0.95; From fa79a881ce0537e1d74da296a7513730438d2a02 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 16 Nov 2023 12:53:31 +0100 Subject: [PATCH 31/62] refactor: P2P transport without serialize version and type --- src/net.cpp | 29 +++++++++---------- src/net.h | 28 ++++++------------ src/net_processing.cpp | 18 +++++------- src/net_processing.h | 2 +- src/test/fuzz/p2p_transport_serialization.cpp | 8 ++--- src/test/net_tests.cpp | 8 ++--- 6 files changed, 40 insertions(+), 53 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 075a7d9839..e1772233b2 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -683,8 +683,8 @@ bool CNode::ReceiveMsgBytes(Span msg_bytes, bool& complete) return true; } -V1Transport::V1Transport(const NodeId node_id, int nTypeIn, int nVersionIn) noexcept : - m_magic_bytes{Params().MessageStart()}, m_node_id(node_id), hdrbuf(nTypeIn, nVersionIn), vRecv(nTypeIn, nVersionIn) +V1Transport::V1Transport(const NodeId node_id) noexcept + : m_magic_bytes{Params().MessageStart()}, m_node_id{node_id} { LOCK(m_recv_mutex); Reset(); @@ -968,12 +968,12 @@ void V2Transport::StartSendingHandshake() noexcept // We cannot wipe m_send_garbage as it will still be used as AAD later in the handshake. } -V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span ent32, std::vector garbage) noexcept : - m_cipher{key, ent32}, m_initiating{initiating}, m_nodeid{nodeid}, - m_v1_fallback{nodeid, type_in, version_in}, m_recv_type{type_in}, m_recv_version{version_in}, - m_recv_state{initiating ? RecvState::KEY : RecvState::KEY_MAYBE_V1}, - m_send_garbage{std::move(garbage)}, - m_send_state{initiating ? SendState::AWAITING_KEY : SendState::MAYBE_V1} +V2Transport::V2Transport(NodeId nodeid, bool initiating, const CKey& key, Span ent32, std::vector garbage) noexcept + : m_cipher{key, ent32}, m_initiating{initiating}, m_nodeid{nodeid}, + m_v1_fallback{nodeid}, + m_recv_state{initiating ? RecvState::KEY : RecvState::KEY_MAYBE_V1}, + m_send_garbage{std::move(garbage)}, + m_send_state{initiating ? SendState::AWAITING_KEY : SendState::MAYBE_V1} { Assume(m_send_garbage.size() <= MAX_GARBAGE_LEN); // Start sending immediately if we're the initiator of the connection. @@ -983,9 +983,9 @@ V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int versio } } -V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept : - V2Transport{nodeid, initiating, type_in, version_in, GenerateRandomKey(), - MakeByteSpan(GetRandHash()), GenerateRandomGarbage()} { } +V2Transport::V2Transport(NodeId nodeid, bool initiating) noexcept + : V2Transport{nodeid, initiating, GenerateRandomKey(), + MakeByteSpan(GetRandHash()), GenerateRandomGarbage()} {} void V2Transport::SetReceiveState(RecvState recv_state) noexcept { @@ -1429,8 +1429,7 @@ CNetMessage V2Transport::GetReceivedMessage(std::chrono::microseconds time, bool Assume(m_recv_state == RecvState::APP_READY); Span contents{m_recv_decode_buffer}; auto msg_type = GetMessageType(contents); - CDataStream ret(m_recv_type, m_recv_version); - CNetMessage msg{std::move(ret)}; + CNetMessage msg{DataStream{}}; // Note that BIP324Cipher::EXPANSION also includes the length descriptor size. msg.m_raw_message_size = m_recv_decode_buffer.size() + BIP324Cipher::EXPANSION; if (msg_type) { @@ -3638,9 +3637,9 @@ ServiceFlags CConnman::GetLocalServices() const static std::unique_ptr MakeTransport(NodeId id, bool use_v2transport, bool inbound) noexcept { if (use_v2transport) { - return std::make_unique(id, /*initiating=*/!inbound, SER_NETWORK, INIT_PROTO_VERSION); + return std::make_unique(id, /*initiating=*/!inbound); } else { - return std::make_unique(id, SER_NETWORK, INIT_PROTO_VERSION); + return std::make_unique(id); } } diff --git a/src/net.h b/src/net.h index aa75df075d..b673df69b7 100644 --- a/src/net.h +++ b/src/net.h @@ -232,15 +232,16 @@ class CNodeStats * Ideally it should only contain receive time, payload, * type and size. */ -class CNetMessage { +class CNetMessage +{ public: - CDataStream m_recv; //!< received message data + DataStream m_recv; //!< received message data std::chrono::microseconds m_time{0}; //!< time of message receipt uint32_t m_message_size{0}; //!< size of the payload uint32_t m_raw_message_size{0}; //!< used wire size of the message (including header/checksum) std::string m_type; - CNetMessage(CDataStream&& recv_in) : m_recv(std::move(recv_in)) {} + explicit CNetMessage(DataStream&& recv_in) : m_recv(std::move(recv_in)) {} // Only one CNetMessage object will exist for the same message on either // the receive or processing queue. For performance reasons we therefore // delete the copy constructor and assignment operator to avoid the @@ -249,11 +250,6 @@ class CNetMessage { CNetMessage(const CNetMessage&) = delete; CNetMessage& operator=(CNetMessage&&) = default; CNetMessage& operator=(const CNetMessage&) = delete; - - void SetVersion(int nVersionIn) - { - m_recv.SetVersion(nVersionIn); - } }; /** The Transport converts one connection's sent messages to wire bytes, and received bytes back. */ @@ -379,9 +375,9 @@ class V1Transport final : public Transport mutable CHash256 hasher GUARDED_BY(m_recv_mutex); mutable uint256 data_hash GUARDED_BY(m_recv_mutex); bool in_data GUARDED_BY(m_recv_mutex); // parsing header (false) or data (true) - CDataStream hdrbuf GUARDED_BY(m_recv_mutex); // partially received header + DataStream hdrbuf GUARDED_BY(m_recv_mutex){}; // partially received header CMessageHeader hdr GUARDED_BY(m_recv_mutex); // complete header - CDataStream vRecv GUARDED_BY(m_recv_mutex); // received message data + DataStream vRecv GUARDED_BY(m_recv_mutex){}; // received message data unsigned int nHdrPos GUARDED_BY(m_recv_mutex); unsigned int nDataPos GUARDED_BY(m_recv_mutex); @@ -420,7 +416,7 @@ class V1Transport final : public Transport size_t m_bytes_sent GUARDED_BY(m_send_mutex) {0}; public: - V1Transport(const NodeId node_id, int nTypeIn, int nVersionIn) noexcept; + explicit V1Transport(const NodeId node_id) noexcept; bool ReceivedMessageComplete() const override EXCLUSIVE_LOCKS_REQUIRED(!m_recv_mutex) { @@ -598,10 +594,6 @@ class V2Transport final : public Transport std::vector m_recv_aad GUARDED_BY(m_recv_mutex); /** Buffer to put decrypted contents in, for converting to CNetMessage. */ std::vector m_recv_decode_buffer GUARDED_BY(m_recv_mutex); - /** Deserialization type. */ - const int m_recv_type; - /** Deserialization version number. */ - const int m_recv_version; /** Current receiver state. */ RecvState m_recv_state GUARDED_BY(m_recv_mutex); @@ -647,13 +639,11 @@ class V2Transport final : public Transport * * @param[in] nodeid the node's NodeId (only for debug log output). * @param[in] initiating whether we are the initiator side. - * @param[in] type_in the serialization type of returned CNetMessages. - * @param[in] version_in the serialization version of returned CNetMessages. */ - V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept; + V2Transport(NodeId nodeid, bool initiating) noexcept; /** Construct a V2 transport with specified keys and garbage (test use only). */ - V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span ent32, std::vector garbage) noexcept; + V2Transport(NodeId nodeid, bool initiating, const CKey& key, Span ent32, std::vector garbage) noexcept; // Receive side functions. bool ReceivedMessageComplete() const noexcept override EXCLUSIVE_LOCKS_REQUIRED(!m_recv_mutex); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0cd20680b0..533346ac81 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -515,7 +515,7 @@ class PeerManagerImpl final : public PeerManager void RelayTransaction(const uint256& txid, const uint256& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void SetBestHeight(int height) override { m_best_height = height; }; void UnitTestMisbehaving(NodeId peer_id, int howmuch) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex) { Misbehaving(*Assert(GetPeerRef(peer_id)), howmuch, ""); }; - void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, + void ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex); void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) override; @@ -1033,7 +1033,7 @@ class PeerManagerImpl final : public PeerManager * @param[in] peer The peer that we received the request from * @param[in] vRecv The raw message received */ - void ProcessGetCFilters(CNode& node, Peer& peer, CDataStream& vRecv); + void ProcessGetCFilters(CNode& node, Peer& peer, DataStream& vRecv); /** * Handle a cfheaders request. @@ -1044,7 +1044,7 @@ class PeerManagerImpl final : public PeerManager * @param[in] peer The peer that we received the request from * @param[in] vRecv The raw message received */ - void ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream& vRecv); + void ProcessGetCFHeaders(CNode& node, Peer& peer, DataStream& vRecv); /** * Handle a getcfcheckpt request. @@ -1055,7 +1055,7 @@ class PeerManagerImpl final : public PeerManager * @param[in] peer The peer that we received the request from * @param[in] vRecv The raw message received */ - void ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& vRecv); + void ProcessGetCFCheckPt(CNode& node, Peer& peer, DataStream& vRecv); /** Checks if address relay is permitted with peer. If needed, initializes * the m_addr_known bloom filter and sets m_addr_relay_enabled to true. @@ -3130,7 +3130,7 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& node, Peer& peer, return true; } -void PeerManagerImpl::ProcessGetCFilters(CNode& node,Peer& peer, CDataStream& vRecv) +void PeerManagerImpl::ProcessGetCFilters(CNode& node, Peer& peer, DataStream& vRecv) { uint8_t filter_type_ser; uint32_t start_height; @@ -3159,7 +3159,7 @@ void PeerManagerImpl::ProcessGetCFilters(CNode& node,Peer& peer, CDataStream& vR } } -void PeerManagerImpl::ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream& vRecv) +void PeerManagerImpl::ProcessGetCFHeaders(CNode& node, Peer& peer, DataStream& vRecv) { uint8_t filter_type_ser; uint32_t start_height; @@ -3201,7 +3201,7 @@ void PeerManagerImpl::ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream& filter_hashes); } -void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& vRecv) +void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, DataStream& vRecv) { uint8_t filter_type_ser; uint256 stop_hash; @@ -3342,7 +3342,7 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl return; } -void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, +void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) { @@ -5056,8 +5056,6 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt CaptureMessage(pfrom->addr, msg.m_type, MakeUCharSpan(msg.m_recv), /*is_incoming=*/true); } - msg.SetVersion(pfrom->GetCommonVersion()); - try { ProcessMessage(*pfrom, msg.m_type, msg.m_recv, msg.m_time, interruptMsgProc); if (interruptMsgProc) return false; diff --git a/src/net_processing.h b/src/net_processing.h index 80d07648a4..afdef00067 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -105,7 +105,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface virtual void CheckForStaleTipAndEvictPeers() = 0; /** Process a single message from a peer. Public for fuzz testing */ - virtual void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, + virtual void ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0; /** This function is used for testing the stale tip eviction logic, see denialofservice_tests.cpp */ diff --git a/src/test/fuzz/p2p_transport_serialization.cpp b/src/test/fuzz/p2p_transport_serialization.cpp index 6af6aa1d18..a205ce19f4 100644 --- a/src/test/fuzz/p2p_transport_serialization.cpp +++ b/src/test/fuzz/p2p_transport_serialization.cpp @@ -36,8 +36,8 @@ void initialize_p2p_transport_serialization() FUZZ_TARGET(p2p_transport_serialization, .init = initialize_p2p_transport_serialization) { // Construct transports for both sides, with dummy NodeIds. - V1Transport recv_transport{NodeId{0}, SER_NETWORK, INIT_PROTO_VERSION}; - V1Transport send_transport{NodeId{1}, SER_NETWORK, INIT_PROTO_VERSION}; + V1Transport recv_transport{NodeId{0}}; + V1Transport send_transport{NodeId{1}}; FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; @@ -335,7 +335,7 @@ void SimulationTest(Transport& initiator, Transport& responder, R& rng, FuzzedDa std::unique_ptr MakeV1Transport(NodeId nodeid) noexcept { - return std::make_unique(nodeid, SER_NETWORK, INIT_PROTO_VERSION); + return std::make_unique(nodeid); } template @@ -369,7 +369,7 @@ std::unique_ptr MakeV2Transport(NodeId nodeid, bool initiator, RNG& r .Write(garb.data(), garb.size()) .Finalize(UCharCast(ent.data())); - return std::make_unique(nodeid, initiator, SER_NETWORK, INIT_PROTO_VERSION, key, ent, std::move(garb)); + return std::make_unique(nodeid, initiator, key, ent, std::move(garb)); } } // namespace diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 3c9227cfc4..508c42cabc 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -1046,10 +1046,10 @@ class V2TransportTester public: /** Construct a tester object. test_initiator: whether the tested transport is initiator. */ - V2TransportTester(bool test_initiator) : - m_transport(0, test_initiator, SER_NETWORK, INIT_PROTO_VERSION), - m_cipher{GenerateRandomTestKey(), MakeByteSpan(InsecureRand256())}, - m_test_initiator(test_initiator) {} + explicit V2TransportTester(bool test_initiator) + : m_transport{0, test_initiator}, + m_cipher{GenerateRandomTestKey(), MakeByteSpan(InsecureRand256())}, + m_test_initiator(test_initiator) {} /** Data type returned by Interact: * From fa971c09f24887d4848082c551d4eed98e7f4edc Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 24 Nov 2023 11:09:41 +0100 Subject: [PATCH 32/62] Export assert from util/check.h This avoids having to include both headers when assert and Assert are used at the same time. --- src/util/check.h | 1 + src/wallet/test/fuzz/notifications.cpp | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/check.h b/src/util/check.h index 00951dec89..a02a1de8dc 100644 --- a/src/util/check.h +++ b/src/util/check.h @@ -7,6 +7,7 @@ #include +#include // IWYU pragma: export #include #include #include diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index abd788f96f..bd12f14a77 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -14,7 +15,6 @@ #include #include -#include #include #include #include From 4335e553596a0ad5b136d3aa9fc898c906d226b4 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 24 Nov 2023 15:40:46 +0000 Subject: [PATCH 33/62] ci: Run vcpkg with path prefix The GHA VS installation includes its own vcpkg package manager, which is available since VS 17.6. This change avoids any ambiguity about which copy of vcpkg we run. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1381d8a23e..d2e609cd31 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -279,7 +279,7 @@ jobs: run: | Set-Location "$env:VCPKG_INSTALLATION_ROOT" Add-Content -Path "triplets\x64-windows-static.cmake" -Value "set(VCPKG_BUILD_TYPE release)" - vcpkg --vcpkg-root "$env:VCPKG_INSTALLATION_ROOT" integrate install + .\vcpkg.exe --vcpkg-root "$env:VCPKG_INSTALLATION_ROOT" integrate install git rev-parse HEAD | Out-File -FilePath "$env:GITHUB_WORKSPACE\vcpkg_commit" Get-Content -Path "$env:GITHUB_WORKSPACE\vcpkg_commit" From 1a889f7ea043fc61865511a0d143321bbdc540a5 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 24 Nov 2023 15:47:28 +0000 Subject: [PATCH 34/62] ci: Set MSVC toolset version explicitly This change avoids toolset incompatibilities that cause linker errors. --- .github/workflows/ci.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d2e609cd31..9419a0f19a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -197,6 +197,8 @@ jobs: run: | msbuild -version | Out-File -FilePath "$env:GITHUB_WORKSPACE\msbuild_version" Get-Content -Path "$env:GITHUB_WORKSPACE\msbuild_version" + $env:VCToolsVersion | Out-File -FilePath "$env:GITHUB_WORKSPACE\toolset_version" + Get-Content -Path "$env:GITHUB_WORKSPACE\toolset_version" $env:CI_QT_URL | Out-File -FilePath "$env:GITHUB_WORKSPACE\qt_url" $env:CI_QT_CONF | Out-File -FilePath "$env:GITHUB_WORKSPACE\qt_conf" @@ -279,6 +281,7 @@ jobs: run: | Set-Location "$env:VCPKG_INSTALLATION_ROOT" Add-Content -Path "triplets\x64-windows-static.cmake" -Value "set(VCPKG_BUILD_TYPE release)" + Add-Content -Path "triplets\x64-windows-static.cmake" -Value "set(VCPKG_PLATFORM_TOOLSET_VERSION $env:VCToolsVersion)" .\vcpkg.exe --vcpkg-root "$env:VCPKG_INSTALLATION_ROOT" integrate install git rev-parse HEAD | Out-File -FilePath "$env:GITHUB_WORKSPACE\vcpkg_commit" Get-Content -Path "$env:GITHUB_WORKSPACE\vcpkg_commit" @@ -293,7 +296,7 @@ jobs: uses: actions/cache@v3 with: path: ~/AppData/Local/vcpkg/archives - key: ${{ github.job }}-vcpkg-binary-${{ hashFiles('vcpkg_commit', 'msbuild_version', 'build_msvc/vcpkg.json') }} + key: ${{ github.job }}-vcpkg-binary-${{ hashFiles('vcpkg_commit', 'msbuild_version', 'toolset_version', 'build_msvc/vcpkg.json') }} - name: Generate project files run: py -3 build_msvc\msvc-autogen.py From 70100f8584c762cbc6cbc09d05cc4e9d22a3ec75 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 24 Nov 2023 15:47:49 +0000 Subject: [PATCH 35/62] Revert "ci: Avoid toolset ambiguity that MSVC can't handle" This reverts commit 91d5bd8ac9a28725c735f8e6900bc85673bb190a. --- .github/workflows/ci.yml | 49 ---------------------------------------- 1 file changed, 49 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9419a0f19a..cdf8e8e6c5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -138,55 +138,6 @@ jobs: - name: Checkout uses: actions/checkout@v4 - - name: Fix Visual Studio installation - # Avoid toolset ambiguity that MSVC can't handle. - run: | - Set-Location "C:\Program Files (x86)\Microsoft Visual Studio\Installer\" - $InstallPath = "C:\Program Files\Microsoft Visual Studio\2022\Enterprise" - $componentsToRemove= @( - "Microsoft.VisualStudio.Component.VC.14.37.17.7.ARM.Spectre" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.ARM" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.ARM64.Spectre" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.ARM64" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.ATL.ARM.Spectre" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.ATL.ARM" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.ATL.ARM64.Spectre" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.ATL.ARM64" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.ATL.Spectre" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.ATL" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.MFC.ARM.Spectre" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.MFC.ARM" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.MFC.ARM64.Spectre" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.MFC.ARM64" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.MFC.Spectre" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.MFC" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.x86.x64.Spectre" - "Microsoft.VisualStudio.Component.VC.14.37.17.7.x86.x64" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.ARM" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.ARM.Spectre" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.ARM64" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.ARM64.Spectre" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.ATL" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.ATL.ARM" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.ATL.ARM.Spectre" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.ATL.ARM64" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.ATL.ARM64.Spectre" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.ATL.Spectre" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.MFC" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.MFC.ARM" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.MFC.ARM.Spectre" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.MFC.ARM64" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.MFC.ARM64.Spectre" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.MFC.Spectre" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.x86.x64" - "Microsoft.VisualStudio.Component.VC.14.38.17.8.x86.x64.Spectre" - ) - [string]$workloadArgs = $componentsToRemove | ForEach-Object {" --remove " + $_} - $Arguments = ('/c', "vs_installer.exe", 'modify', '--installPath', "`"$InstallPath`"",$workloadArgs, '--quiet', '--norestart', '--nocache') - # should be run twice - $process = Start-Process -FilePath cmd.exe -ArgumentList $Arguments -Wait -PassThru -WindowStyle Hidden - $process = Start-Process -FilePath cmd.exe -ArgumentList $Arguments -Wait -PassThru -WindowStyle Hidden - - name: Configure Developer Command Prompt for Microsoft Visual C++ # Using microsoft/setup-msbuild is not enough. uses: ilammy/msvc-dev-cmd@v1 From ecb46837e7f4fc3a2860ba14f2d3034859e34900 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Sat, 25 Nov 2023 13:59:41 +0000 Subject: [PATCH 36/62] Change petertodd seeds to petertodd.net I changed my DNS seeds to .net from .org to avoid issues with DNS blacklisting, that falsely thinks my domain name is pointing to IP addresses with malware and similar things. Right now there are CNAME records, so the .org addresses still work. But eventually, if needed, I'll remove those CNAME's. --- src/kernel/chainparams.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp index 73ba330ff0..56cb3a63a0 100644 --- a/src/kernel/chainparams.cpp +++ b/src/kernel/chainparams.cpp @@ -136,7 +136,7 @@ class CMainParams : public CChainParams { vSeeds.emplace_back("dnsseed.bitcoin.dashjr.org."); // Luke Dashjr vSeeds.emplace_back("seed.bitcoinstats.com."); // Christian Decker, supports x1 - xf vSeeds.emplace_back("seed.bitcoin.jonasschnelli.ch."); // Jonas Schnelli, only supports x1, x5, x9, and xd - vSeeds.emplace_back("seed.btc.petertodd.org."); // Peter Todd, only supports x1, x5, x9, and xd + vSeeds.emplace_back("seed.btc.petertodd.net."); // Peter Todd, only supports x1, x5, x9, and xd vSeeds.emplace_back("seed.bitcoin.sprovoost.nl."); // Sjors Provoost vSeeds.emplace_back("dnsseed.emzy.de."); // Stephan Oeste vSeeds.emplace_back("seed.bitcoin.wiz.biz."); // Jason Maurice @@ -243,7 +243,7 @@ class CTestNetParams : public CChainParams { vSeeds.clear(); // nodes with support for servicebits filtering should be at the top vSeeds.emplace_back("testnet-seed.bitcoin.jonasschnelli.ch."); - vSeeds.emplace_back("seed.tbtc.petertodd.org."); + vSeeds.emplace_back("seed.tbtc.petertodd.net."); vSeeds.emplace_back("seed.testnet.bitcoin.sprovoost.nl."); vSeeds.emplace_back("testnet-seed.bluematt.me."); // Just a static list of stable node(s), only supports x9 From 2d2ef2f14f352438afab2e3251d07fd100be7076 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sat, 25 Nov 2023 18:27:33 +0000 Subject: [PATCH 37/62] msvc: No need to specify the default feature for `libevent` package --- build_msvc/vcpkg.json | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/build_msvc/vcpkg.json b/build_msvc/vcpkg.json index 3557269be0..25ef4a24af 100644 --- a/build_msvc/vcpkg.json +++ b/build_msvc/vcpkg.json @@ -7,11 +7,8 @@ "boost-process", "boost-signals2", "boost-test", + "libevent", "sqlite3", - { - "name": "libevent", - "features": ["thread"] - }, "zeromq" ], "builtin-baseline": "f14984af3738e69f197bf0e647a8dca12de92996", From 1f97e51d737c1a5a3e4503db3373a792f87f3bc1 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sat, 25 Nov 2023 18:27:40 +0000 Subject: [PATCH 38/62] msvc: Update vcpkg manifest baseline up to "2023.08.09 Release" Dependency changes (2023.01.09 --> 2023.08.09): - berkeleydb: 4.8.30#8 --> 4.8.30#9 - boost: 1.81.0 --> 1.82.0#2 - sqlite3: 3.40.0#1 --> 3.42.0#1 - zeromq: 4.3.4#6 --> 2023-06-20#1 --- build_msvc/vcpkg.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build_msvc/vcpkg.json b/build_msvc/vcpkg.json index 25ef4a24af..701adaec74 100644 --- a/build_msvc/vcpkg.json +++ b/build_msvc/vcpkg.json @@ -11,7 +11,7 @@ "sqlite3", "zeromq" ], - "builtin-baseline": "f14984af3738e69f197bf0e647a8dca12de92996", + "builtin-baseline": "9edb1b8e590cc086563301d735cae4b6e732d2d2", "overrides": [ { "name": "libevent", From 6d05c4fd138b80168d14a8cf1dbcca43782851af Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sat, 25 Nov 2023 18:27:50 +0000 Subject: [PATCH 39/62] msvc: Specify `boost-date-time` package explicitly Compilation now succeeds only by coincidence, as the `boost-date-time` package is installed as a dependency of the `boost-process` one. --- build_msvc/vcpkg.json | 1 + 1 file changed, 1 insertion(+) diff --git a/build_msvc/vcpkg.json b/build_msvc/vcpkg.json index 701adaec74..18ac8f2f09 100644 --- a/build_msvc/vcpkg.json +++ b/build_msvc/vcpkg.json @@ -3,6 +3,7 @@ "version-string": "1", "dependencies": [ "berkeleydb", + "boost-date-time", "boost-multi-index", "boost-process", "boost-signals2", From fa15861763df71e788849b587883b3c16bb12229 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 24 Nov 2023 13:13:55 +0100 Subject: [PATCH 40/62] fuzz: Faster wallet_notifications target --- src/wallet/test/fuzz/notifications.cpp | 162 +++++++++++++++++++------ 1 file changed, 127 insertions(+), 35 deletions(-) diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index bd12f14a77..66e14354c4 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -2,21 +2,46 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include +#include +#include +#include #include +#include +#include +#include +#include +#include +#include