refactor: centralize SubmitBlock result handling
What the developer wrote
Authored by w0xlt
refactor: centralize SubmitBlock result handling
Move the accepted/new-block/reason consistency check into SubmitBlock()
so submitBlock() and submitSolution() use the same success criteria.
This keeps duplicate and inconclusive handling in one place, removes the
new_block output parameter from the helper, and makes the helper return
whether the submitted block was accepted as a new valid block.
What changed, and why it matters
This is a small internal cleanup in Bitcoin Core's block submission code. It moves a duplicate-check and success-calculation that existed in two places into a single shared helper function. The visible behavior for the mining RPCs is intended to stay the same, and the change is described by the author as a refactor.
No immediate action required. Treat as routine code cleanup. Standard review and regression testing for mining RPCs (submitblock, getblocktemplate submitSolution) is sufficient.
Security signals we found
Refactor of block submission success criteria
Removes new_block output parameter from SubmitBlock helper
Centralizes accepted/new-block/reason consistency check in one function
Preserves CHECK_NONFATAL invariant that result equals reason.empty()
No change to network consensus or validation logic visible in diff
Evidence from the diff
The commit refactors SubmitBlock() in src/node/miner.cpp/h and its two callers in src/node/interfaces.cpp. Previously SubmitBlock() returned whether ProcessNewBlock accepted the block and wrote a new_block out-parameter; callers then computed success as accepted && new_block && reason.empty(). The change removes the out-parameter, performs that same computation inside SubmitBlock(), and returns it directly. One caller (submitSolution) previously returned accepted && new_block && reason.empty(); the other (submitBlock) did the same and included a CHECK_NONFATAL(result == reason.empty()) assertion. The assertion is preserved inside SubmitBlock(). The duplicate-as-error behavior is unchanged; only the code location of the consistency check changes.
Changed components
src/node/miner.cppsrc/node/miner.hsrc/node/interfaces.cppSubmitBlock helperMining RPC submitBlock / submitSolution pathsInspect captured patch +12 / −20
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 0699ff8b..feaa9a9d 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -920,9 +920,7 @@ public:
bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase, std::string& reason, std::string& debug) override
{
AddMerkleRootAndCoinbase(m_block_template->block, std::move(coinbase), version, timestamp, nonce);
- bool new_block;
- const bool accepted = SubmitBlock(chainman(), std::make_shared<const CBlock>(m_block_template->block), &new_block, reason, debug);
- return accepted && new_block && reason.empty();
+ return SubmitBlock(chainman(), std::make_shared<const CBlock>(m_block_template->block), reason, debug);
}
std::unique_ptr<BlockTemplate> waitNext(BlockWaitOptions options) override
@@ -1025,15 +1023,7 @@ public:
bool submitBlock(const CBlock& block_in, std::string& reason, std::string& debug) override
{
- auto block = std::make_shared<const CBlock>(block_in);
- bool new_block;
- const bool accepted = SubmitBlock(chainman(), block, &new_block, reason, debug);
- // ProcessNewBlock() can accept and store a block before it is checked
- // for validity. Treat duplicates as errors for mining clients, and only
- // return success when validation completed without setting a reason.
- const bool result{accepted && new_block && reason.empty()};
- CHECK_NONFATAL(result == reason.empty());
- return result;
+ return SubmitBlock(chainman(), std::make_shared<const CBlock>(block_in), reason, debug);
}
const NodeContext* context() override { return &m_node; }
diff --git a/src/node/miner.cpp b/src/node/miner.cpp
index 66ae870f..40e8f524 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -381,7 +381,7 @@ protected:
};
} // namespace
-bool SubmitBlock(ChainstateManager& chainman, const std::shared_ptr<const CBlock>& block, bool* new_block, std::string& reason, std::string& debug)
+bool SubmitBlock(ChainstateManager& chainman, const std::shared_ptr<const CBlock>& block, std::string& reason, std::string& debug)
{
reason.clear();
debug.clear();
@@ -391,16 +391,16 @@ bool SubmitBlock(ChainstateManager& chainman, const std::shared_ptr<const CBlock
// point decodes hex, formats BIP22/JSONRPC results, and calls
// UpdateUncommittedBlockStructures() for legacy witness handling. IPC
// callers submit already-formed blocks and need bool + reason/debug
- // results, while submitSolution() preserves its duplicate-as-success
- // behavior.
+ // results.
auto sc = std::make_shared<SubmitBlockStateCatcher>(block->GetHash());
CHECK_NONFATAL(chainman.m_options.signals)->RegisterSharedValidationInterface(sc);
- bool accepted = chainman.ProcessNewBlock(block, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/new_block);
+ bool new_block;
+ bool accepted = chainman.ProcessNewBlock(block, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/&new_block);
// No queue drain is needed. The BlockChecked notification used above is
// emitted synchronously by ProcessNewBlock, unlike most validation signals.
CHECK_NONFATAL(chainman.m_options.signals)->UnregisterSharedValidationInterface(sc);
- if (new_block && !*new_block && accepted) {
+ if (!new_block && accepted) {
reason = "duplicate";
} else if (!accepted && (!sc->m_found || sc->m_state.IsValid())) {
// ProcessNewBlock can fail without a validation result, for example
@@ -416,7 +416,9 @@ bool SubmitBlock(ChainstateManager& chainman, const std::shared_ptr<const CBlock
reason = sc->m_state.GetRejectReason();
debug = sc->m_state.GetDebugMessage();
}
- return accepted;
+ const bool result{accepted && new_block && reason.empty()};
+ CHECK_NONFATAL(result == reason.empty());
+ return result;
}
void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait)
diff --git a/src/node/miner.h b/src/node/miner.h
index af327307..7b5702ac 100644
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -131,8 +131,8 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman);
void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t version, uint32_t timestamp, uint32_t nonce);
//! Submit a block and capture the validation state via the BlockChecked callback.
-//! Returns whether ProcessNewBlock accepted the block.
-bool SubmitBlock(ChainstateManager& chainman, const std::shared_ptr<const CBlock>& block, bool* new_block, std::string& reason, std::string& debug);
+//! Returns whether the block was accepted as a new valid block.
+bool SubmitBlock(ChainstateManager& chainman, const std::shared_ptr<const CBlock>& block, std::string& reason, std::string& debug);
/* Interrupt a blocking call. */
void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait);
Why this scored 18/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.