Merge pull request #5106

1bea2bb Rename ProcessBlock to ProcessNewBlock to indicate change of behaviour, and document it (Luke Dashjr)
d29a291 Rename RPC_TRANSACTION_* errors to RPC_VERIFY_* and use RPC_VERIFY_ERROR for submitblock (Luke Dashjr)
f877aaa Bugfix: submitblock: Use a temporary CValidationState to determine accurately the outcome of ProcessBlock, now that it no longer does the full block validity check (Luke Dashjr)
24e8896 Add CValidationInterface::BlockChecked notification (Luke Dashjr)

6 files changed, 71 insertions, 18 deletions

diff --git a/src/main.cpp b/src/main.cpp
index 008a059103..37891f3b0d 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -152,6 +152,8 @@ struct CMainSignals {
     boost::signals2::signal<void (const uint256 &)> Inventory;
     // Tells listeners to broadcast their data.
     boost::signals2::signal<void ()> Broadcast;
+    // Notifies listeners of a block validation result
+    boost::signals2::signal<void (const CBlock&, const CValidationState&)> BlockChecked;
 } g_signals;
 
 } // anon namespace
@@ -163,9 +165,11 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) {
     g_signals.SetBestChain.connect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1));
     g_signals.Inventory.connect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1));
     g_signals.Broadcast.connect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn));
+    g_signals.BlockChecked.connect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
 }
 
 void UnregisterValidationInterface(CValidationInterface* pwalletIn) {
+    g_signals.BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
     g_signals.Broadcast.disconnect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn));
     g_signals.Inventory.disconnect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1));
     g_signals.SetBestChain.disconnect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1));
@@ -175,6 +179,7 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn) {
 }
 
 void UnregisterAllValidationInterfaces() {
+    g_signals.BlockChecked.disconnect_all_slots();
     g_signals.Broadcast.disconnect_all_slots();
     g_signals.Inventory.disconnect_all_slots();
     g_signals.SetBestChain.disconnect_all_slots();
@@ -1864,7 +1869,9 @@ bool static ConnectTip(CValidationState &state, CBlockIndex *pindexNew, CBlock *
     {
         CCoinsViewCache view(pcoinsTip);
         CInv inv(MSG_BLOCK, pindexNew->GetBlockHash());
-        if (!ConnectBlock(*pblock, state, pindexNew, view)) {
+        bool rv = ConnectBlock(*pblock, state, pindexNew, view);
+        g_signals.BlockChecked(*pblock, state);
+        if (!rv) {
             if (state.IsInvalid())
                 InvalidBlockFound(pindexNew, state);
             return error("ConnectTip() : ConnectBlock %s failed", pindexNew->GetBlockHash().ToString());
@@ -2504,7 +2511,7 @@ void CBlockIndex::BuildSkip()
         pskip = pprev->GetAncestor(GetSkipHeight(nHeight));
 }
 
-bool ProcessBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBlockPos *dbp)
+bool ProcessNewBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBlockPos *dbp)
 {
     // Preliminary checks
     bool checked = CheckBlock(*pblock, state);
@@ -2513,7 +2520,7 @@ bool ProcessBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBl
         LOCK(cs_main);
         MarkBlockAsReceived(pblock->GetHash());
         if (!checked) {
-            return error("ProcessBlock() : CheckBlock FAILED");
+            return error("%s : CheckBlock FAILED", __func__);
         }
 
         // Store to disk
@@ -2523,11 +2530,11 @@ bool ProcessBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBl
             mapBlockSource[pindex->GetBlockHash()] = pfrom->GetId();
         }
         if (!ret)
-            return error("ProcessBlock() : AcceptBlock FAILED");
+            return error("%s : AcceptBlock FAILED", __func__);
     }
 
     if (!ActivateBestChain(state, pblock))
-        return error("ProcessBlock() : ActivateBestChain failed");
+        return error("%s : ActivateBestChain failed", __func__);
 
     return true;
 }
@@ -3136,7 +3143,7 @@ bool LoadExternalBlockFile(FILE* fileIn, CDiskBlockPos *dbp)
                 // process in case the block isn't known yet
                 if (mapBlockIndex.count(hash) == 0) {
                     CValidationState state;
-                    if (ProcessBlock(state, NULL, &block, dbp))
+                    if (ProcessNewBlock(state, NULL, &block, dbp))
                         nLoaded++;
                     if (state.IsError())
                         break;
@@ -3156,7 +3163,7 @@ bool LoadExternalBlockFile(FILE* fileIn, CDiskBlockPos *dbp)
                             LogPrintf("%s: Processing out of order child %s of %s\n", __func__, block.GetHash().ToString(),
                                     head.ToString());
                             CValidationState dummy;
-                            if (ProcessBlock(dummy, NULL, &block, &it->second))
+                            if (ProcessNewBlock(dummy, NULL, &block, &it->second))
                             {
                                 nLoaded++;
                                 queue.push_back(block.GetHash());
@@ -3934,7 +3941,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
         pfrom->AddInventoryKnown(inv);
 
         CValidationState state;
-        ProcessBlock(state, pfrom, &block);
+        ProcessNewBlock(state, pfrom, &block);
         int nDoS;
         if (state.IsInvalid(nDoS)) {
             pfrom->PushMessage("reject", strCommand, state.GetRejectCode(),
diff --git a/src/main.h b/src/main.h
index 1941ca7059..74ed1b8ed2 100644
--- a/src/main.h
+++ b/src/main.h
@@ -151,8 +151,16 @@ void RegisterNodeSignals(CNodeSignals& nodeSignals);
 /** Unregister a network node */
 void UnregisterNodeSignals(CNodeSignals& nodeSignals);
 
-/** Process an incoming block */
-bool ProcessBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBlockPos *dbp = NULL);
+/** Process an incoming block. This only returns after the best known valid
+    block is made active. Note that it does not, however, guarantee that the
+    specific block passed to it has been checked for validity!
+    @param[out]  state   This may be set to an Error state if any error occurred processing it, including during validation/connection/etc of otherwise unrelated blocks during reorganisation; or it may be set to an Invalid state iff pblock is itself invalid (but this is not guaranteed even when the block is checked). If you want to *possibly* get feedback on whether pblock is valid, you must also install a CValidationInterface - this will have its BlockChecked method called whenever *any* block completes validation.
+    @param[in]   pfrom   The node which we are receiving the block from; it is added to mapBlockSource and may be penalised if the block is invalid.
+    @param[in]   pblock  The block we want to process.
+    @param[out]  dbp     If pblock is stored to disk (or already there), this will be set to its location.
+    @return True if state.IsValid()
+*/
+bool ProcessNewBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBlockPos *dbp = NULL);
 /** Check whether enough disk space is available for an incoming block */
 bool CheckDiskSpace(uint64_t nAdditionalBytes = 0);
 /** Open a block file (blk?????.dat) */
@@ -651,6 +659,7 @@ protected:
     virtual void UpdatedTransaction(const uint256 &hash) {};
     virtual void Inventory(const uint256 &hash) {};
     virtual void ResendWalletTransactions() {};
+    virtual void BlockChecked(const CBlock&, const CValidationState&) {};
     friend void ::RegisterValidationInterface(CValidationInterface*);
     friend void ::UnregisterValidationInterface(CValidationInterface*);
     friend void ::UnregisterAllValidationInterfaces();
diff --git a/src/miner.cpp b/src/miner.cpp
index 0235de3ab3..b5bfa9c7be 100644
--- a/src/miner.cpp
+++ b/src/miner.cpp
@@ -428,8 +428,8 @@ bool ProcessBlockFound(CBlock* pblock, CWallet& wallet, CReserveKey& reservekey)
 
     // Process this block the same as if we had received it from another node
     CValidationState state;
-    if (!ProcessBlock(state, NULL, pblock))
-        return error("BitcoinMiner : ProcessBlock, block not accepted");
+    if (!ProcessNewBlock(state, NULL, pblock))
+        return error("BitcoinMiner : ProcessNewBlock, block not accepted");
 
     return true;
 }
diff --git a/src/rpcmining.cpp b/src/rpcmining.cpp
index 8af3c46348..0be4e06164 100644
--- a/src/rpcmining.cpp
+++ b/src/rpcmining.cpp
@@ -527,6 +527,24 @@ Value getblocktemplate(const Array& params, bool fHelp)
     return result;
 }
 
+class submitblock_StateCatcher : public CValidationInterface
+{
+public:
+    uint256 hash;
+    bool found;
+    CValidationState state;
+
+    submitblock_StateCatcher(const uint256 &hashIn) : hash(hashIn), found(false), state() {};
+
+protected:
+    virtual void BlockChecked(const CBlock& block, const CValidationState& stateIn) {
+        if (block.GetHash() != hash)
+            return;
+        found = true;
+        state = stateIn;
+    };
+};
+
 Value submitblock(const Array& params, bool fHelp)
 {
     if (fHelp || params.size() < 1 || params.size() > 2)
@@ -559,8 +577,22 @@ Value submitblock(const Array& params, bool fHelp)
     }
 
     CValidationState state;
-    bool fAccepted = ProcessBlock(state, NULL, &pblock);
-    if (!fAccepted)
+    submitblock_StateCatcher sc(pblock.GetHash());
+    RegisterValidationInterface(&sc);
+    bool fAccepted = ProcessNewBlock(state, NULL, &pblock);
+    UnregisterValidationInterface(&sc);
+    if (fAccepted)
+    {
+        if (!sc.found)
+            return "inconclusive";
+        state = sc.state;
+    }
+    if (state.IsError())
+    {
+        std::string strRejectReason = state.GetRejectReason();
+        throw JSONRPCError(RPC_VERIFY_ERROR, strRejectReason);
+    }
+    if (state.IsInvalid())
         return "rejected"; // TODO: report validation state
 
     return Value::null;
diff --git a/src/rpcprotocol.h b/src/rpcprotocol.h
index a9adb58803..9117248506 100644
--- a/src/rpcprotocol.h
+++ b/src/rpcprotocol.h
@@ -49,9 +49,14 @@ enum RPCErrorCode
     RPC_INVALID_PARAMETER           = -8,  // Invalid, missing or duplicate parameter
     RPC_DATABASE_ERROR              = -20, // Database error
     RPC_DESERIALIZATION_ERROR       = -22, // Error parsing or validating structure in raw format
-    RPC_TRANSACTION_ERROR           = -25, // General error during transaction submission
-    RPC_TRANSACTION_REJECTED        = -26, // Transaction was rejected by network rules
-    RPC_TRANSACTION_ALREADY_IN_CHAIN= -27, // Transaction already in chain
+    RPC_VERIFY_ERROR                = -25, // General error during transaction or block submission
+    RPC_VERIFY_REJECTED             = -26, // Transaction or block was rejected by network rules
+    RPC_VERIFY_ALREADY_IN_CHAIN     = -27, // Transaction already in chain
+
+    // Aliases for backward compatibility
+    RPC_TRANSACTION_ERROR           = RPC_VERIFY_ERROR,
+    RPC_TRANSACTION_REJECTED        = RPC_VERIFY_REJECTED,
+    RPC_TRANSACTION_ALREADY_IN_CHAIN= RPC_VERIFY_ALREADY_IN_CHAIN,
 
     // P2P client errors
     RPC_CLIENT_NOT_CONNECTED        = -9,  // Bitcoin is not connected
diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
index 93b7fe189a..032ae983ce 100644
--- a/src/test/miner_tests.cpp
+++ b/src/test/miner_tests.cpp
@@ -79,7 +79,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
         pblock->hashMerkleRoot = pblock->BuildMerkleTree();
         pblock->nNonce = blockinfo[i].nonce;
         CValidationState state;
-        BOOST_CHECK(ProcessBlock(state, NULL, pblock));
+        BOOST_CHECK(ProcessNewBlock(state, NULL, pblock));
         BOOST_CHECK(state.IsValid());
         pblock->hashPrevBlock = pblock->GetHash();
     }