Skip to content

Commit e7a9372

Browse files
achow101achow101
committed
Merge bitcoin/bitcoin#32378: interfaces: refactor: move Mining and BlockTemplate implementation to miner
62fc42d interfaces: refactor: move `waitTipChanged` implementation to miner (ismaelsadeeq) c39ca9d interfaces: move getTip implementation to miner (Sjors Provoost) 720f201 interfaces: refactor: move `waitNext` implementation to miner (ismaelsadeeq) e6c2f4c interfaces: refactor: move `submitSolution` implementation to miner (ismaelsadeeq) 02d4bc7 interfaces: remove redundant coinbase fee check in `waitNext` (ismaelsadeeq) Pull request description: #### Motivation In [Internal interface guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines) It's stated that > Interface method definitions should wrap existing functionality instead of implementing new functionality. Any substantial new node or wallet functionality should be implemented in [src/node/](https://github.com/bitcoin/bitcoin/blob/master/src/node) or [src/wallet/](https://github.com/bitcoin/bitcoin/blob/master/src/wallet) and just exposed in [src/interfaces/](https://github.com/bitcoin/bitcoin/blob/master/src/interfaces) instead of being implemented there, so it can be more modular and accessible to unit tests. However the some methods in the newly added `BlockTemplateImpl` and `MinerImpl` classes partially enforces this guideline, as the implementations of the `submitSolution`, `waitNext`, and `waitTipChanged` methods reside within the class itself. #### What the PR Does This PR introduces a simple refactor by moving certain method implementations from `BlockTemplateImpl` into the miner module. It introduces three new functions: 1. Remove rundundant coinbase fee check in `waitNext` 2. **`AddMerkleRootAndCoinbase`**: Computes the block's Merkle root, inserts the coinbase transaction, and sets the Merkle root in the block. This function is called by `submitSolution` before the block is submitted for processing. 3. **`WaitAndCreateNewBlock`**: Returns a new block template either when transaction fees reach a certain threshold or when a new tip is detected. If a timeout is reached, it returns `nullptr`. The `waitNext` method in `BlockTemplateImpl` now simply wraps this function. 4. Move `GetTip` implementation to miner. 5. **`WaitTipChanged`**: Returns the tip when the chain it changes, or `nullopt` if a timeout or interrupt occurs. The `waitTipChanged` method in `MinerImpl` now calls `GetTip` after invoking `ChainTipChanged`, and returns the tip. #### Behavior Change - We now only `Assert` for a valid chainman and notifications pointer once. ACKs for top commit: achow101: ACK 62fc42d Sjors: ACK 62fc42d ryanofsky: Code review ACK 62fc42d. Lots of suggest suggest changes made since last review, altering function names and signatures and also adding new commit to drop negative fee handling. I like the idea of making the wait function return a BlockRef, that is clearer than what I suggested. Left some comments below but they are not important and this looks good as-is Tree-SHA512: 502632f94ced81f576b2c43cf015f1527e2c259e6ca253f670f5a6889171e2246372b4e709575701afa3f01d488d6633557fef54f48fe83bbaf1836ac5326c4f
2 parents f1d78a3 + 62fc42d commit e7a9372

File tree

3 files changed

+174
-132
lines changed

3 files changed

+174
-132
lines changed

‎src/node/interfaces.cpp

Lines changed: 6 additions & 132 deletions
Original file line numberDiff line numberDiff line change
@@ -929,119 +929,21 @@ class BlockTemplateImpl : public BlockTemplate
929929

930930
bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) override
931931
{
932-
CBlock block{m_block_template->block};
933-
934-
if (block.vtx.size() == 0) {
935-
block.vtx.push_back(coinbase);
936-
} else {
937-
block.vtx[0] = coinbase;
938-
}
939-
940-
block.nVersion = version;
941-
block.nTime = timestamp;
942-
block.nNonce = nonce;
943-
944-
block.hashMerkleRoot = BlockMerkleRoot(block);
945-
946-
auto block_ptr = std::make_shared<const CBlock>(block);
947-
return chainman().ProcessNewBlock(block_ptr, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr);
932+
AddMerkleRootAndCoinbase(m_block_template->block, std::move(coinbase), version, timestamp, nonce);
933+
return chainman().ProcessNewBlock(std::make_shared<const CBlock>(m_block_template->block), /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr);
948934
}
949935

950936
std::unique_ptr<BlockTemplate> waitNext(BlockWaitOptions options) override
951937
{
952-
// Delay calculating the current template fees, just in case a new block
953-
// comes in before the next tick.
954-
CAmount current_fees = -1;
955-
956-
// Alternate waiting for a new tip and checking if fees have risen.
957-
// The latter check is expensive so we only run it once per second.
958-
auto now{NodeClock::now()};
959-
const auto deadline = now + options.timeout;
960-
const MillisecondsDouble tick{1000};
961-
const bool allow_min_difficulty{chainman().GetParams().GetConsensus().fPowAllowMinDifficultyBlocks};
962-
963-
do {
964-
bool tip_changed{false};
965-
{
966-
WAIT_LOCK(notifications().m_tip_block_mutex, lock);
967-
// Note that wait_until() checks the predicate before waiting
968-
notifications().m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
969-
AssertLockHeld(notifications().m_tip_block_mutex);
970-
const auto tip_block{notifications().TipBlock()};
971-
// We assume tip_block is set, because this is an instance
972-
// method on BlockTemplate and no template could have been
973-
// generated before a tip exists.
974-
tip_changed = Assume(tip_block) && tip_block != m_block_template->block.hashPrevBlock;
975-
return tip_changed || chainman().m_interrupt;
976-
});
977-
}
978-
979-
if (chainman().m_interrupt) return nullptr;
980-
// At this point the tip changed, a full tick went by or we reached
981-
// the deadline.
982-
983-
// Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks.
984-
LOCK(::cs_main);
985-
986-
// On test networks return a minimum difficulty block after 20 minutes
987-
if (!tip_changed && allow_min_difficulty) {
988-
const NodeClock::time_point tip_time{std::chrono::seconds{chainman().ActiveChain().Tip()->GetBlockTime()}};
989-
if (now > tip_time + 20min) {
990-
tip_changed = true;
991-
}
992-
}
993-
994-
/**
995-
* We determine if fees increased compared to the previous template by generating
996-
* a fresh template. There may be more efficient ways to determine how much
997-
* (approximate) fees for the next block increased, perhaps more so after
998-
* Cluster Mempool.
999-
*
1000-
* We'll also create a new template if the tip changed during this iteration.
1001-
*/
1002-
if (options.fee_threshold < MAX_MONEY || tip_changed) {
1003-
auto tmpl{std::make_unique<BlockTemplateImpl>(m_assemble_options,
1004-
BlockAssembler{
1005-
chainman().ActiveChainstate(),
1006-
context()->mempool.get(),
1007-
m_assemble_options}
1008-
.CreateNewBlock(),
1009-
m_node)};
1010-
1011-
// If the tip changed, return the new template regardless of its fees.
1012-
if (tip_changed) return tmpl;
1013-
1014-
// Calculate the original template total fees if we haven't already
1015-
if (current_fees == -1) {
1016-
current_fees = 0;
1017-
for (CAmount fee : m_block_template->vTxFees) {
1018-
// Skip coinbase
1019-
if (fee < 0) continue;
1020-
current_fees += fee;
1021-
}
1022-
}
1023-
1024-
CAmount new_fees = 0;
1025-
for (CAmount fee : tmpl->m_block_template->vTxFees) {
1026-
// Skip coinbase
1027-
if (fee < 0) continue;
1028-
new_fees += fee;
1029-
Assume(options.fee_threshold != MAX_MONEY);
1030-
if (new_fees >= current_fees + options.fee_threshold) return tmpl;
1031-
}
1032-
}
1033-
1034-
now = NodeClock::now();
1035-
} while (now < deadline);
1036-
938+
auto new_template = WaitAndCreateNewBlock(chainman(), notifications(), m_node.mempool.get(), m_block_template, options, m_assemble_options);
939+
if (new_template) return std::make_unique<BlockTemplateImpl>(m_assemble_options, std::move(new_template), m_node);
1037940
return nullptr;
1038941
}
1039942

1040943
const BlockAssembler::Options m_assemble_options;
1041944

1042945
const std::unique_ptr<CBlockTemplate> m_block_template;
1043946

1044-
NodeContext* context() { return &m_node; }
1045947
ChainstateManager& chainman() { return *Assert(m_node.chainman); }
1046948
KernelNotifications& notifications() { return *Assert(m_node.notifications); }
1047949
NodeContext& m_node;
@@ -1064,40 +966,12 @@ class MinerImpl : public Mining
1064966

1065967
std::optional<BlockRef> getTip() override
1066968
{
1067-
LOCK(::cs_main);
1068-
CBlockIndex* tip{chainman().ActiveChain().Tip()};
1069-
if (!tip) return {};
1070-
return BlockRef{tip->GetBlockHash(), tip->nHeight};
969+
return GetTip(chainman());
1071970
}
1072971

1073972
std::optional<BlockRef> waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
1074973
{
1075-
Assume(timeout >= 0ms); // No internal callers should use a negative timeout
1076-
if (timeout < 0ms) timeout = 0ms;
1077-
if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono
1078-
auto deadline{std::chrono::steady_clock::now() + timeout};
1079-
{
1080-
WAIT_LOCK(notifications().m_tip_block_mutex, lock);
1081-
// For callers convenience, wait longer than the provided timeout
1082-
// during startup for the tip to be non-null. That way this function
1083-
// always returns valid tip information when possible and only
1084-
// returns null when shutting down, not when timing out.
1085-
notifications().m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
1086-
return notifications().TipBlock() || chainman().m_interrupt;
1087-
});
1088-
if (chainman().m_interrupt) return {};
1089-
// At this point TipBlock is set, so continue to wait until it is
1090-
// different then `current_tip` provided by caller.
1091-
notifications().m_tip_block_cv.wait_until(lock, deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
1092-
return Assume(notifications().TipBlock()) != current_tip || chainman().m_interrupt;
1093-
});
1094-
}
1095-
1096-
if (chainman().m_interrupt) return {};
1097-
1098-
// Must release m_tip_block_mutex before getTip() locks cs_main, to
1099-
// avoid deadlocks.
1100-
return getTip();
974+
return WaitTipChanged(chainman(), notifications(), current_tip, timeout);
1101975
}
1102976

1103977
std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override

‎src/node/miner.cpp

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,14 @@
1616
#include <consensus/validation.h>
1717
#include <deploymentstatus.h>
1818
#include <logging.h>
19+
#include <node/context.h>
20+
#include <node/kernel_notifications.h>
1921
#include <policy/feerate.h>
2022
#include <policy/policy.h>
2123
#include <pow.h>
2224
#include <primitives/transaction.h>
2325
#include <util/moneystr.h>
26+
#include <util/signalinterrupt.h>
2427
#include <util/time.h>
2528
#include <validation.h>
2629

@@ -437,4 +440,143 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda
437440
nDescendantsUpdated += UpdatePackagesForAdded(mempool, ancestors, mapModifiedTx);
438441
}
439442
}
443+
444+
void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t version, uint32_t timestamp, uint32_t nonce)
445+
{
446+
if (block.vtx.size() == 0) {
447+
block.vtx.emplace_back(coinbase);
448+
} else {
449+
block.vtx[0] = coinbase;
450+
}
451+
block.nVersion = version;
452+
block.nTime = timestamp;
453+
block.nNonce = nonce;
454+
block.hashMerkleRoot = BlockMerkleRoot(block);
455+
}
456+
457+
std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainman,
458+
KernelNotifications& kernel_notifications,
459+
CTxMemPool* mempool,
460+
const std::unique_ptr<CBlockTemplate>& block_template,
461+
const BlockWaitOptions& options,
462+
const BlockAssembler::Options& assemble_options)
463+
{
464+
// Delay calculating the current template fees, just in case a new block
465+
// comes in before the next tick.
466+
CAmount current_fees = -1;
467+
468+
// Alternate waiting for a new tip and checking if fees have risen.
469+
// The latter check is expensive so we only run it once per second.
470+
auto now{NodeClock::now()};
471+
const auto deadline = now + options.timeout;
472+
const MillisecondsDouble tick{1000};
473+
const bool allow_min_difficulty{chainman.GetParams().GetConsensus().fPowAllowMinDifficultyBlocks};
474+
475+
do {
476+
bool tip_changed{false};
477+
{
478+
WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock);
479+
// Note that wait_until() checks the predicate before waiting
480+
kernel_notifications.m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
481+
AssertLockHeld(kernel_notifications.m_tip_block_mutex);
482+
const auto tip_block{kernel_notifications.TipBlock()};
483+
// We assume tip_block is set, because this is an instance
484+
// method on BlockTemplate and no template could have been
485+
// generated before a tip exists.
486+
tip_changed = Assume(tip_block) && tip_block != block_template->block.hashPrevBlock;
487+
return tip_changed || chainman.m_interrupt;
488+
});
489+
}
490+
491+
if (chainman.m_interrupt) return nullptr;
492+
// At this point the tip changed, a full tick went by or we reached
493+
// the deadline.
494+
495+
// Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks.
496+
LOCK(::cs_main);
497+
498+
// On test networks return a minimum difficulty block after 20 minutes
499+
if (!tip_changed && allow_min_difficulty) {
500+
const NodeClock::time_point tip_time{std::chrono::seconds{chainman.ActiveChain().Tip()->GetBlockTime()}};
501+
if (now > tip_time + 20min) {
502+
tip_changed = true;
503+
}
504+
}
505+
506+
/**
507+
* We determine if fees increased compared to the previous template by generating
508+
* a fresh template. There may be more efficient ways to determine how much
509+
* (approximate) fees for the next block increased, perhaps more so after
510+
* Cluster Mempool.
511+
*
512+
* We'll also create a new template if the tip changed during this iteration.
513+
*/
514+
if (options.fee_threshold < MAX_MONEY || tip_changed) {
515+
auto new_tmpl{BlockAssembler{
516+
chainman.ActiveChainstate(),
517+
mempool,
518+
assemble_options}
519+
.CreateNewBlock()};
520+
521+
// If the tip changed, return the new template regardless of its fees.
522+
if (tip_changed) return new_tmpl;
523+
524+
// Calculate the original template total fees if we haven't already
525+
if (current_fees == -1) {
526+
current_fees = 0;
527+
for (CAmount fee : block_template->vTxFees) {
528+
current_fees += fee;
529+
}
530+
}
531+
532+
CAmount new_fees = 0;
533+
for (CAmount fee : new_tmpl->vTxFees) {
534+
new_fees += fee;
535+
Assume(options.fee_threshold != MAX_MONEY);
536+
if (new_fees >= current_fees + options.fee_threshold) return new_tmpl;
537+
}
538+
}
539+
540+
now = NodeClock::now();
541+
} while (now < deadline);
542+
543+
return nullptr;
544+
}
545+
546+
std::optional<BlockRef> GetTip(ChainstateManager& chainman)
547+
{
548+
LOCK(::cs_main);
549+
CBlockIndex* tip{chainman.ActiveChain().Tip()};
550+
if (!tip) return {};
551+
return BlockRef{tip->GetBlockHash(), tip->nHeight};
552+
}
553+
554+
std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout)
555+
{
556+
Assume(timeout >= 0ms); // No internal callers should use a negative timeout
557+
if (timeout < 0ms) timeout = 0ms;
558+
if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono
559+
auto deadline{std::chrono::steady_clock::now() + timeout};
560+
{
561+
WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock);
562+
// For callers convenience, wait longer than the provided timeout
563+
// during startup for the tip to be non-null. That way this function
564+
// always returns valid tip information when possible and only
565+
// returns null when shutting down, not when timing out.
566+
kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
567+
return kernel_notifications.TipBlock() || chainman.m_interrupt;
568+
});
569+
if (chainman.m_interrupt) return {};
570+
// At this point TipBlock is set, so continue to wait until it is
571+
// different then `current_tip` provided by caller.
572+
kernel_notifications.m_tip_block_cv.wait_until(lock, deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
573+
return Assume(kernel_notifications.TipBlock()) != current_tip || chainman.m_interrupt;
574+
});
575+
}
576+
if (chainman.m_interrupt) return {};
577+
578+
// Must release m_tip_block_mutex before getTip() locks cs_main, to
579+
// avoid deadlocks.
580+
return GetTip(chainman);
581+
}
440582
} // namespace node

‎src/node/miner.h

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#ifndef BITCOIN_NODE_MINER_H
77
#define BITCOIN_NODE_MINER_H
88

9+
#include <interfaces/types.h>
910
#include <node/types.h>
1011
#include <policy/policy.h>
1112
#include <primitives/block.h>
@@ -31,7 +32,11 @@ class ChainstateManager;
3132

3233
namespace Consensus { struct Params; };
3334

35+
using interfaces::BlockRef;
36+
3437
namespace node {
38+
class KernelNotifications;
39+
3540
static const bool DEFAULT_PRINT_MODIFIED_FEE = false;
3641

3742
struct CBlockTemplate
@@ -229,6 +234,27 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman);
229234

230235
/** Apply -blockmintxfee and -blockmaxweight options from ArgsManager to BlockAssembler options. */
231236
void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& options);
237+
238+
/* Compute the block's merkle root, insert or replace the coinbase transaction and the merkle root into the block */
239+
void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t version, uint32_t timestamp, uint32_t nonce);
240+
241+
/**
242+
* Return a new block template when fees rise to a certain threshold or after a
243+
* new tip; return nullopt if timeout is reached.
244+
*/
245+
std::unique_ptr<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainman,
246+
KernelNotifications& kernel_notifications,
247+
CTxMemPool* mempool,
248+
const std::unique_ptr<CBlockTemplate>& block_template,
249+
const BlockWaitOptions& options,
250+
const BlockAssembler::Options& assemble_options);
251+
252+
/* Locks cs_main and returns the block hash and block height of the active chain if it exists; otherwise, returns nullopt.*/
253+
std::optional<BlockRef> GetTip(ChainstateManager& chainman);
254+
255+
/* 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`).
256+
* Returns the current tip, or nullopt if the node is shutting down. */
257+
std::optional<BlockRef> WaitTipChanged(ChainstateManager& chainman, KernelNotifications& kernel_notifications, const uint256& current_tip, MillisecondsDouble& timeout);
232258
} // namespace node
233259

234260
#endif // BITCOIN_NODE_MINER_H

0 commit comments

Comments
 (0)