From 02d4bc776bbe002ee624ec2c09d7c3f981be1b17 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Tue, 13 May 2025 14:54:29 +0100 Subject: [PATCH 1/5] interfaces: remove redundant coinbase fee check in `waitNext` - vTxFees now does not include the negative coinbase fee, hence this check can be removed. --- src/node/interfaces.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 8aec2758f8b..629db507ef4 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -1015,16 +1015,12 @@ public: if (current_fees == -1) { current_fees = 0; for (CAmount fee : m_block_template->vTxFees) { - // Skip coinbase - if (fee < 0) continue; current_fees += fee; } } CAmount new_fees = 0; for (CAmount fee : tmpl->m_block_template->vTxFees) { - // Skip coinbase - if (fee < 0) continue; new_fees += fee; Assume(options.fee_threshold != MAX_MONEY); if (new_fees >= current_fees + options.fee_threshold) return tmpl; From e6c2f4ce7a841153510971f0236c527d1a499649 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Mon, 28 Apr 2025 17:00:50 +0100 Subject: [PATCH 2/5] interfaces: refactor: move `submitSolution` implementation to miner - Create a new function `AddMerkleRootAndCoinbase` that compute the block's merkle root, insert the coinbase transaction and the merkle root into the block. --- src/node/interfaces.cpp | 18 ++---------------- src/node/miner.cpp | 13 +++++++++++++ src/node/miner.h | 3 +++ 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 629db507ef4..99a793e9a8a 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -929,22 +929,8 @@ public: bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) override { - CBlock block{m_block_template->block}; - - if (block.vtx.size() == 0) { - block.vtx.push_back(coinbase); - } else { - block.vtx[0] = coinbase; - } - - block.nVersion = version; - block.nTime = timestamp; - block.nNonce = nonce; - - block.hashMerkleRoot = BlockMerkleRoot(block); - - auto block_ptr = std::make_shared(block); - return chainman().ProcessNewBlock(block_ptr, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr); + AddMerkleRootAndCoinbase(m_block_template->block, std::move(coinbase), version, timestamp, nonce); + return chainman().ProcessNewBlock(std::make_shared(m_block_template->block), /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr); } std::unique_ptr waitNext(BlockWaitOptions options) override diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 68d71ab785b..0c982bedfe2 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -434,4 +434,17 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda nDescendantsUpdated += UpdatePackagesForAdded(mempool, ancestors, mapModifiedTx); } } + +void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t version, uint32_t timestamp, uint32_t nonce) +{ + if (block.vtx.size() == 0) { + block.vtx.emplace_back(coinbase); + } else { + block.vtx[0] = coinbase; + } + block.nVersion = version; + block.nTime = timestamp; + block.nNonce = nonce; + block.hashMerkleRoot = BlockMerkleRoot(block); +} } // namespace node diff --git a/src/node/miner.h b/src/node/miner.h index c09e9eb5d1c..641f8a031bb 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -229,6 +229,9 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman); /** Apply -blockmintxfee and -blockmaxweight options from ArgsManager to BlockAssembler options. */ void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& options); + +/* Compute the block's merkle root, insert or replace the coinbase transaction and the merkle root into the block */ +void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t version, uint32_t timestamp, uint32_t nonce); } // namespace node #endif // BITCOIN_NODE_MINER_H From 720f201e652885b9e0aec8e62a1bf9590052b320 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Mon, 28 Apr 2025 17:53:40 +0100 Subject: [PATCH 3/5] interfaces: refactor: move `waitNext` implementation to miner - We now assert for a valid chainman and notifications once when invoking WaitAndCreateNewBlock function. --- src/node/interfaces.cpp | 84 +------------------------------------ src/node/miner.cpp | 92 +++++++++++++++++++++++++++++++++++++++++ src/node/miner.h | 13 ++++++ 3 files changed, 107 insertions(+), 82 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 99a793e9a8a..08594fe5af3 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -935,87 +935,8 @@ public: std::unique_ptr waitNext(BlockWaitOptions options) override { - // Delay calculating the current template fees, just in case a new block - // comes in before the next tick. - CAmount current_fees = -1; - - // Alternate waiting for a new tip and checking if fees have risen. - // The latter check is expensive so we only run it once per second. - auto now{NodeClock::now()}; - const auto deadline = now + options.timeout; - const MillisecondsDouble tick{1000}; - const bool allow_min_difficulty{chainman().GetParams().GetConsensus().fPowAllowMinDifficultyBlocks}; - - do { - bool tip_changed{false}; - { - WAIT_LOCK(notifications().m_tip_block_mutex, lock); - // Note that wait_until() checks the predicate before waiting - notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) { - AssertLockHeld(notifications().m_tip_block_mutex); - const auto tip_block{notifications().TipBlock()}; - // We assume tip_block is set, because this is an instance - // method on BlockTemplate and no template could have been - // generated before a tip exists. - tip_changed = Assume(tip_block) && tip_block != m_block_template->block.hashPrevBlock; - return tip_changed || chainman().m_interrupt; - }); - } - - if (chainman().m_interrupt) return nullptr; - // At this point the tip changed, a full tick went by or we reached - // the deadline. - - // Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks. - LOCK(::cs_main); - - // On test networks return a minimum difficulty block after 20 minutes - if (!tip_changed && allow_min_difficulty) { - const NodeClock::time_point tip_time{std::chrono::seconds{chainman().ActiveChain().Tip()->GetBlockTime()}}; - if (now > tip_time + 20min) { - tip_changed = true; - } - } - - /** - * We determine if fees increased compared to the previous template by generating - * a fresh template. There may be more efficient ways to determine how much - * (approximate) fees for the next block increased, perhaps more so after - * Cluster Mempool. - * - * We'll also create a new template if the tip changed during this iteration. - */ - if (options.fee_threshold < MAX_MONEY || tip_changed) { - auto tmpl{std::make_unique(m_assemble_options, - BlockAssembler{ - chainman().ActiveChainstate(), - context()->mempool.get(), - m_assemble_options} - .CreateNewBlock(), - m_node)}; - - // If the tip changed, return the new template regardless of its fees. - if (tip_changed) return tmpl; - - // Calculate the original template total fees if we haven't already - if (current_fees == -1) { - current_fees = 0; - for (CAmount fee : m_block_template->vTxFees) { - current_fees += fee; - } - } - - CAmount new_fees = 0; - for (CAmount fee : tmpl->m_block_template->vTxFees) { - new_fees += fee; - Assume(options.fee_threshold != MAX_MONEY); - if (new_fees >= current_fees + options.fee_threshold) return tmpl; - } - } - - now = NodeClock::now(); - } while (now < deadline); - + auto new_template = WaitAndCreateNewBlock(chainman(), notifications(), m_node.mempool.get(), m_block_template, options, m_assemble_options); + if (new_template) return std::make_unique(m_assemble_options, std::move(new_template), m_node); return nullptr; } @@ -1023,7 +944,6 @@ public: const std::unique_ptr m_block_template; - NodeContext* context() { return &m_node; } ChainstateManager& chainman() { return *Assert(m_node.chainman); } KernelNotifications& notifications() { return *Assert(m_node.notifications); } NodeContext& m_node; diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 0c982bedfe2..213b46cd0bf 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -16,11 +16,14 @@ #include #include #include +#include +#include #include #include #include #include #include +#include #include #include @@ -447,4 +450,93 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t block.nNonce = nonce; block.hashMerkleRoot = BlockMerkleRoot(block); } + +std::unique_ptr WaitAndCreateNewBlock(ChainstateManager& chainman, + KernelNotifications& kernel_notifications, + CTxMemPool* mempool, + const std::unique_ptr& block_template, + const BlockWaitOptions& options, + const BlockAssembler::Options& assemble_options) +{ + // Delay calculating the current template fees, just in case a new block + // comes in before the next tick. + CAmount current_fees = -1; + + // Alternate waiting for a new tip and checking if fees have risen. + // The latter check is expensive so we only run it once per second. + auto now{NodeClock::now()}; + const auto deadline = now + options.timeout; + const MillisecondsDouble tick{1000}; + const bool allow_min_difficulty{chainman.GetParams().GetConsensus().fPowAllowMinDifficultyBlocks}; + + do { + bool tip_changed{false}; + { + WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock); + // Note that wait_until() checks the predicate before waiting + kernel_notifications.m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) { + AssertLockHeld(kernel_notifications.m_tip_block_mutex); + const auto tip_block{kernel_notifications.TipBlock()}; + // We assume tip_block is set, because this is an instance + // method on BlockTemplate and no template could have been + // generated before a tip exists. + tip_changed = Assume(tip_block) && tip_block != block_template->block.hashPrevBlock; + return tip_changed || chainman.m_interrupt; + }); + } + + if (chainman.m_interrupt) return nullptr; + // At this point the tip changed, a full tick went by or we reached + // the deadline. + + // Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks. + LOCK(::cs_main); + + // On test networks return a minimum difficulty block after 20 minutes + if (!tip_changed && allow_min_difficulty) { + const NodeClock::time_point tip_time{std::chrono::seconds{chainman.ActiveChain().Tip()->GetBlockTime()}}; + if (now > tip_time + 20min) { + tip_changed = true; + } + } + + /** + * We determine if fees increased compared to the previous template by generating + * a fresh template. There may be more efficient ways to determine how much + * (approximate) fees for the next block increased, perhaps more so after + * Cluster Mempool. + * + * We'll also create a new template if the tip changed during this iteration. + */ + if (options.fee_threshold < MAX_MONEY || tip_changed) { + auto new_tmpl{BlockAssembler{ + chainman.ActiveChainstate(), + mempool, + assemble_options} + .CreateNewBlock()}; + + // If the tip changed, return the new template regardless of its fees. + if (tip_changed) return new_tmpl; + + // Calculate the original template total fees if we haven't already + if (current_fees == -1) { + current_fees = 0; + for (CAmount fee : block_template->vTxFees) { + current_fees += fee; + } + } + + CAmount new_fees = 0; + for (CAmount fee : new_tmpl->vTxFees) { + new_fees += fee; + Assume(options.fee_threshold != MAX_MONEY); + if (new_fees >= current_fees + options.fee_threshold) return new_tmpl; + } + } + + now = NodeClock::now(); + } while (now < deadline); + + return nullptr; +} } // namespace node diff --git a/src/node/miner.h b/src/node/miner.h index 641f8a031bb..5dcdca437a3 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -32,6 +32,8 @@ class ChainstateManager; namespace Consensus { struct Params; }; namespace node { +class KernelNotifications; + static const bool DEFAULT_PRINT_MODIFIED_FEE = false; struct CBlockTemplate @@ -232,6 +234,17 @@ void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& opti /* Compute the block's merkle root, insert or replace the coinbase transaction and the merkle root into the block */ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t version, uint32_t timestamp, uint32_t nonce); + +/** + * Return a new block template when fees rise to a certain threshold or after a + * new tip; return nullopt if timeout is reached. + */ +std::unique_ptr WaitAndCreateNewBlock(ChainstateManager& chainman, + KernelNotifications& kernel_notifications, + CTxMemPool* mempool, + const std::unique_ptr& block_template, + const BlockWaitOptions& options, + const BlockAssembler::Options& assemble_options); } // namespace node #endif // BITCOIN_NODE_MINER_H From c39ca9d4f7bc9ca155692ac949be2e61c0598a97 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 14 May 2025 09:51:15 +0200 Subject: [PATCH 4/5] interfaces: move getTip implementation to miner --- src/node/interfaces.cpp | 5 +---- src/node/miner.cpp | 8 ++++++++ src/node/miner.h | 6 ++++++ 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 08594fe5af3..cf21787d7d1 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -966,10 +966,7 @@ public: std::optional getTip() override { - LOCK(::cs_main); - CBlockIndex* tip{chainman().ActiveChain().Tip()}; - if (!tip) return {}; - return BlockRef{tip->GetBlockHash(), tip->nHeight}; + return GetTip(chainman()); } std::optional waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 213b46cd0bf..426e065ff21 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -539,4 +539,12 @@ std::unique_ptr WaitAndCreateNewBlock(ChainstateManager& chainma return nullptr; } + +std::optional GetTip(ChainstateManager& chainman) +{ + LOCK(::cs_main); + CBlockIndex* tip{chainman.ActiveChain().Tip()}; + if (!tip) return {}; + return BlockRef{tip->GetBlockHash(), tip->nHeight}; +} } // namespace node diff --git a/src/node/miner.h b/src/node/miner.h index 5dcdca437a3..d0ae8312fb5 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -6,6 +6,7 @@ #ifndef BITCOIN_NODE_MINER_H #define BITCOIN_NODE_MINER_H +#include #include #include #include @@ -31,6 +32,8 @@ class ChainstateManager; namespace Consensus { struct Params; }; +using interfaces::BlockRef; + namespace node { class KernelNotifications; @@ -245,6 +248,9 @@ std::unique_ptr WaitAndCreateNewBlock(ChainstateManager& chainma const std::unique_ptr& block_template, const BlockWaitOptions& options, const BlockAssembler::Options& assemble_options); + +/* Locks cs_main and returns the block hash and block height of the active chain if it exists; otherwise, returns nullopt.*/ +std::optional GetTip(ChainstateManager& chainman); } // namespace node #endif // BITCOIN_NODE_MINER_H From 62fc42d475df4f23bd93313f95ee7b7eb0d4683f Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Tue, 29 Apr 2025 12:54:23 +0100 Subject: [PATCH 5/5] interfaces: refactor: move `waitTipChanged` implementation to miner - This commit creates a function `WaitTipChanged` that waits for the connected tip to change until timeout elapsed. - This function is now used by `waitTipChanged` Co-authored-by: Ryan Ofsky --- src/node/interfaces.cpp | 27 +-------------------------- src/node/miner.cpp | 29 +++++++++++++++++++++++++++++ src/node/miner.h | 4 ++++ 3 files changed, 34 insertions(+), 26 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index cf21787d7d1..b5c74a97f6d 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -971,32 +971,7 @@ public: std::optional waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override { - Assume(timeout >= 0ms); // No internal callers should use a negative timeout - if (timeout < 0ms) timeout = 0ms; - if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono - auto deadline{std::chrono::steady_clock::now() + timeout}; - { - WAIT_LOCK(notifications().m_tip_block_mutex, lock); - // For callers convenience, wait longer than the provided timeout - // during startup for the tip to be non-null. That way this function - // always returns valid tip information when possible and only - // returns null when shutting down, not when timing out. - notifications().m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) { - return notifications().TipBlock() || chainman().m_interrupt; - }); - if (chainman().m_interrupt) return {}; - // At this point TipBlock is set, so continue to wait until it is - // different then `current_tip` provided by caller. - notifications().m_tip_block_cv.wait_until(lock, deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) { - return Assume(notifications().TipBlock()) != current_tip || chainman().m_interrupt; - }); - } - - if (chainman().m_interrupt) return {}; - - // Must release m_tip_block_mutex before getTip() locks cs_main, to - // avoid deadlocks. - return getTip(); + return WaitTipChanged(chainman(), notifications(), current_tip, timeout); } std::unique_ptr createNewBlock(const BlockCreateOptions& options) override diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 426e065ff21..70567cf5b68 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -547,4 +547,33 @@ std::optional GetTip(ChainstateManager& chainman) if (!tip) return {}; return BlockRef{tip->GetBlockHash(), tip->nHeight}; } + +std::optional WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout) +{ + Assume(timeout >= 0ms); // No internal callers should use a negative timeout + if (timeout < 0ms) timeout = 0ms; + if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono + auto deadline{std::chrono::steady_clock::now() + timeout}; + { + WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock); + // For callers convenience, wait longer than the provided timeout + // during startup for the tip to be non-null. That way this function + // always returns valid tip information when possible and only + // returns null when shutting down, not when timing out. + kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) { + return kernel_notifications.TipBlock() || chainman.m_interrupt; + }); + if (chainman.m_interrupt) return {}; + // At this point TipBlock is set, so continue to wait until it is + // different then `current_tip` provided by caller. + kernel_notifications.m_tip_block_cv.wait_until(lock, deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) { + return Assume(kernel_notifications.TipBlock()) != current_tip || chainman.m_interrupt; + }); + } + if (chainman.m_interrupt) return {}; + + // Must release m_tip_block_mutex before getTip() locks cs_main, to + // avoid deadlocks. + return GetTip(chainman); +} } // namespace node diff --git a/src/node/miner.h b/src/node/miner.h index d0ae8312fb5..5073d69bb1b 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -251,6 +251,10 @@ std::unique_ptr WaitAndCreateNewBlock(ChainstateManager& chainma /* Locks cs_main and returns the block hash and block height of the active chain if it exists; otherwise, returns nullopt.*/ std::optional GetTip(ChainstateManager& chainman); + +/* Waits for the connected tip to change until timeout has elapsed. During node initialization, this will wait until the tip is connected (regardless of `timeout`). + * Returns the current tip, or nullopt if the node is shutting down. */ +std::optional WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout); } // namespace node #endif // BITCOIN_NODE_MINER_H