Merge #13439: rpc: Avoid "duplicate" return value for invalid submitblock

jonasschnelli · jonasschnelli · commit 3f398d7a17f1 · 2018-06-19T09:25:17.000+02:00
f748944 Only set fNewBlock to true in AcceptBlock when we write to disk (Matt Corallo) fa6e497 rpc: Avoid "duplicate" return value for invalid submitblock (MarcoFalke) Pull request description: This is #13395 with one more commit tacked on. MarcoFalke got tired of dealing with the stupidity of fixing a return code with too many rounds of review (not that I blame him). Honestly we should probably have no return whatsoever, but for now, this fixes it (as well as nLastBlockTime for eviction purposes). Original description: When `submitblock` of an invalid block, the return value should not be `"duplicate"`. This is only seen when the header was previously found (denoted by the incorrectly named boolean `fBlockPresent`). Fix this bug by removing `fBlockPresent`. Tree-SHA512: 0ce3092655d5d904b4c8c5ff7479f73ce387144a738f20472b8af132564005c6db5594ae366e589508f6258506ee7a28b1c7995a83a8328b334f99316006bf2d
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
@@ -725,7 +725,6 @@ static UniValue submitblock(const JSONRPCRequest& request)
     }
 
     uint256 hash = block.GetHash();
-    bool fBlockPresent = false;
     {
         LOCK(cs_main);
         const CBlockIndex* pindex = LookupBlockIndex(hash);
@@ -736,8 +735,6 @@ static UniValue submitblock(const JSONRPCRequest& request)
             if (pindex->nStatus & BLOCK_FAILED_MASK) {
                 return "duplicate-invalid";
             }
-            // Otherwise, we might only have the header - process the block before returning
-            fBlockPresent = true;
         }
     }
 
@@ -749,13 +746,15 @@ static UniValue submitblock(const JSONRPCRequest& request)
         }
     }
 
+    bool new_block;
     submitblock_StateCatcher sc(block.GetHash());
     RegisterValidationInterface(&sc);
-    bool fAccepted = ProcessNewBlock(Params(), blockptr, true, nullptr);
+    bool accepted = ProcessNewBlock(Params(), blockptr, /* fForceProcessing */ true, /* fNewBlock */ &new_block);
     UnregisterValidationInterface(&sc);
-    if (fBlockPresent) {
-        if (fAccepted && !sc.found) {
-            return "duplicate-inconclusive";
+    if (!new_block) {
+        if (!accepted) {
+            // TODO Maybe pass down fNewBlock to AcceptBlockHeader, so it is properly set to true in this case?
+            return "invalid";
         }
         return "duplicate";
     }
diff --git a/src/validation.cpp b/src/validation.cpp
@@ -3508,7 +3508,6 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CVali
         // request; don't process these.
         if (pindex->nChainWork < nMinimumChainWork) return true;
     }
-    if (fNewBlock) *fNewBlock = true;
 
     if (!CheckBlock(block, state, chainparams.GetConsensus()) ||
         !ContextualCheckBlock(block, state, chainparams.GetConsensus(), pindex->pprev)) {
@@ -3525,6 +3524,7 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CVali
         GetMainSignals().NewPoWValidBlock(pindex, pblock);
 
     // Write block to history file
+    if (fNewBlock) *fNewBlock = true;
     try {
         CDiskBlockPos blockPos = SaveBlockToDisk(block, pindex->nHeight, chainparams, dbp);
         if (blockPos.IsNull()) {

Original file line number	Diff line number	Diff line change
`@@ -725,7 +725,6 @@ static UniValue submitblock(const JSONRPCRequest& request)`
`725`	`725`	`}`
`726`	`726`
`727`	`727`	`uint256 hash = block.GetHash();`
`728`		`- bool fBlockPresent = false;`
`729`	`728`	`{`
`730`	`729`	`LOCK(cs_main);`
`731`	`730`	`const CBlockIndex* pindex = LookupBlockIndex(hash);`
`@@ -736,8 +735,6 @@ static UniValue submitblock(const JSONRPCRequest& request)`
`736`	`735`	`if (pindex->nStatus & BLOCK_FAILED_MASK) {`
`737`	`736`	`return "duplicate-invalid";`
`738`	`737`	`}`
`739`		`- // Otherwise, we might only have the header - process the block before returning`
`740`		`- fBlockPresent = true;`
`741`	`738`	`}`
`742`	`739`	`}`
`743`	`740`
`@@ -749,13 +746,15 @@ static UniValue submitblock(const JSONRPCRequest& request)`
`749`	`746`	`}`
`750`	`747`	`}`
`751`	`748`
	`749`	`+ bool new_block;`
`752`	`750`	`submitblock_StateCatcher sc(block.GetHash());`
`753`	`751`	`RegisterValidationInterface(&sc);`
`754`		`- bool fAccepted = ProcessNewBlock(Params(), blockptr, true, nullptr);`
	`752`	`+ bool accepted = ProcessNewBlock(Params(), blockptr, /* fForceProcessing / true, / fNewBlock */ &new_block);`
`755`	`753`	`UnregisterValidationInterface(&sc);`
`756`		`- if (fBlockPresent) {`
`757`		`- if (fAccepted && !sc.found) {`
`758`		`- return "duplicate-inconclusive";`
	`754`	`+ if (!new_block) {`
	`755`	`+ if (!accepted) {`
	`756`	`+ // TODO Maybe pass down fNewBlock to AcceptBlockHeader, so it is properly set to true in this case?`
	`757`	`+ return "invalid";`
`759`	`758`	`}`
`760`	`759`	`return "duplicate";`
`761`	`760`	`}`