Merge bitcoin/bitcoin#34672: mining: add reason/debug to submitSolution and unify with submitBlock

75929b11ed doc: add release note for submitSolution IPC changes (woltx)
ed75d70fdb refactor: centralize SubmitBlock result handling (w0xlt)
cbaa1696f3 mining: add reason and debug output to submitSolution (w0xlt)
83f3bc002d mining: clarify SubmitBlock result handling (w0xlt)

Pull request description:

  `BlockTemplate.submitSolution` currently returns only a boolean, so IPC mining clients cannot determine why a submission failed without inspecting Bitcoin Core's debug log.

  Returning `reason` and `debug`, as `Mining.submitBlock` already does, lets callers distinguish a concrete block rejection from a duplicate or inconclusive result. Here, `inconclusive` means the method returns failure, but validation did not determine that the submitted block is invalid.

  This follow-up was suggested during the review of #34644:
  https://github.com/bitcoin/bitcoin/pull/34644#discussion_r2853758006

  This PR:

  - Extracts a shared `SubmitBlock` helper that wraps `ProcessNewBlock` with `SubmitBlockStateCatcher` to capture `BlockValidationState`
  - Adds `reason` and `debug` output parameters to `submitSolution`, matching `submitBlock`
  - Makes both methods delegate to the same helper, eliminating duplicated logic

ACKs for top commit:
  optout21:
    ACK 75929b11ed
  achow101:
    light ACK 75929b11ed
  Sjors:
    ACK 75929b11ed
  enirox001:
    ACK 75929b11ed
  sedited:
    ACK 75929b11ed

Tree-SHA512: 31b1c305c20aaebdfa2d887665d9927830d0f97ba3c3469e2792148ad799d5a400a000cc0ca0b9add071d314e27c9da44d55228c442533a32a7c031678b78a55

src/node/interfaces.cpp

@@ -917,13 +917,11 @@ public:
         return TransactionMerklePath(m_block_template->block, 0);
     }
 
-    bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) override
+    bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase, std::string& reason, std::string& debug) override
     {
         if (!coinbase) return false;
         AddMerkleRootAndCoinbase(m_block_template->block, std::move(coinbase), version, timestamp, nonce);
-        std::string reason;
-        std::string debug;
-        return SubmitBlock(chainman(), std::make_shared<const CBlock>(m_block_template->block), /*new_block=*/nullptr, reason, debug);
+        return SubmitBlock(chainman(), std::make_shared<const CBlock>(m_block_template->block), reason, debug);
     }
 
     std::unique_ptr<BlockTemplate> waitNext(BlockWaitOptions options) override
@@ -1026,13 +1024,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.
-        return accepted && new_block && reason.empty();
+        return SubmitBlock(chainman(), std::make_shared<const CBlock>(block_in), reason, debug);
     }
 
     std::vector<CTransactionRef> getTransactionsByTxID(const std::vector<Txid>& txids) override

src/node/miner.cpp

@@ -385,7 +385,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();
@@ -395,27 +395,34 @@ 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
+        // from an activation or system error. It can also fail after a valid
+        // BlockChecked result. In these cases the validation result is
+        // inconclusive.
+        reason = "inconclusive";
     } else if (!sc->m_found) {
-        // A block can be accepted and stored without being connected, for
-        // example if it does not have more work than the current tip. In that
-        // case no BlockChecked callback is emitted, so the validation result is
-        // inconclusive. Mining::submitBlock treats this as an error for mining
-        // clients, but it does not mean the block is invalid.
+        // The block was accepted but not connected, for example if it does not
+        // have more work than the current tip.
         reason = "inconclusive";
     } else if (!sc->m_state.IsValid()) {
         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)

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);