RPC: submitblock: Support for returning specific rejection reasons #3727

luke-jr commented at 12:38 AM on February 22, 2014: member

No description provided.

jgarzik commented at 2:05 AM on February 22, 2014: contributor

ACK the first commit. I need to trace through the code to fully review the second commit, which touches key validation code.

sipa commented at 10:42 PM on February 22, 2014: member

Looks correct to me. The only validation change is replace a "true" by state.Orphan (which returns true), and the changes to state itself should be unobservable except for the IsOrphan() method, which the core code doesn't call.

in src/rpcmining.cpp:None in 3979137418 outdated

 620 | @@ -621,7 +621,23 @@ Value submitblock(const Array& params, bool fHelp)
 621 |      CValidationState state;
 622 |      bool fAccepted = ProcessBlock(state, NULL, &pblock);
 623 |      if (!fAccepted)
 624 | -        return "rejected"; // TODO: report validation state
 625 | +    {
 626 | +        std::string strRejectReason = state.GetRejectReason();
 627 | +        if (state.IsError())
 628 | +            throw JSONRPCError(RPC_MISC_ERROR, strRejectReason);

laanwj commented at 7:13 AM on February 25, 2014:

You could use RPC_TRANSACTION_REJECTED code here, which I introduce in #3730 (maybe rename it to transaction or block rejected...).

luke-jr commented at 7:14 AM on February 25, 2014:

RPC_INPUT_REJECTED?

laanwj commented at 7:16 AM on February 25, 2014:

Input is too general. It should be clear that this concerns transaction/block-level rejection, not just invalid input. Edit: also it would be useful to be able to get the reject code from the error as well, not just the string message.

laanwj commented at 7:23 AM on April 16, 2014:

I eventually added three transaction-validation related error codes in #3730, RPC_TRANSACTION_ERROR would be appropriate here:

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

luke-jr commented at 9:13 AM on April 16, 2014:

But there's no reason to think it has anything to do with transactions...

laanwj commented at 9:45 AM on April 16, 2014:

Obviously. But you could widen the scope.

luke-jr commented at 9:48 AM on April 16, 2014:

So rename it to RPC_ACCEPT_ERROR? Or what?

laanwj commented at 9:52 AM on April 16, 2014:

I'd say RPC_VERIFY_* and change the comments to be "Transaction or block"

luke-jr commented at 2:40 PM on June 26, 2014: member

Rebased, with @laanwj 's change implemented.

gavinandresen commented at 3:57 PM on June 26, 2014: contributor

Untested-but-code-reviewed ACK.

BitcoinPullTester commented at 5:45 PM on June 26, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p3727_8608fce20ac72af78d1f01f8c10d9dacc2c364ed/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

jgarzik commented at 5:13 PM on July 29, 2014: contributor

Shall we move this along and collect ACKs? Consensus seems to be we like this one.

laanwj commented at 8:45 AM on July 30, 2014: member

Untested ACK

sipa commented at 11:08 AM on July 30, 2014: member

This will not work for all types of errors. All ProcessBlock does now is do preliminary checks and store the block in the block database, and remember who we got the block from. Then we invoke ActivateBestChain which connects the new best chain if any, but as it's not about a specific block anymore, it does not return information about errors with the block - they're sent directly as rejections to the peer that gave us the block.

To fix this, either plug into InvalidBlockFound, or we need some more generic way of listening for problems with particular blocks (another signal handler?).

luke-jr commented at 4:33 PM on July 30, 2014: member

@sipa In this case, the "peer that give us the block" is a RPC client. Can we tie in via that somehow?

sipa commented at 9:09 PM on July 30, 2014: member

An alternative is not using ProcessBlock, but calling CheckBlock, AcceptBlock and ConnectTip directly. The logic should be trivial as submitblock can just fail in case the block submitted is not an immediate successor that would cause switching immediately.

laanwj added the label Block Creation on Jul 31, 2014

laanwj commented at 9:17 AM on August 1, 2014: member

@sipa Sounds like a good idea to me.

luke-jr force-pushed on Oct 9, 2014

RPC: submitblock: Support for returning specific rejection reasons e69a5873e7

luke-jr force-pushed on Oct 30, 2014

luke-jr commented at 2:19 AM on October 30, 2014: member

Rebased on top of #5106

luke-jr force-pushed on Nov 3, 2014

luke-jr commented at 11:30 AM on November 3, 2014: member

I believe this is now simple enough to be quickly re-ACK'd?

sipa commented at 11:49 AM on November 3, 2014: member

utACK

laanwj merged this on Nov 3, 2014

laanwj closed this on Nov 3, 2014

laanwj referenced this in commit ff17816abf on Nov 3, 2014

DrahtBot locked this on Sep 8, 2021