Bugfix: submitblock: Use a temporary CValidationState to determine accurately the outcome of ProcessBlock #5106

pull luke-jr wants to merge 4 commits into bitcoin:master from luke-jr:bugfix_submitblock_reject changing 6 files +71 −18

luke-jr commented at 4:19 AM on October 20, 2014: member

Fixes #5083 (based on #5105)
sipa commented at 6:13 AM on October 20, 2014: member

utACK. Thanks for listening to my complaints :)
Add CValidationInterface::BlockChecked notification 24e8896430
Bugfix: submitblock: Use a temporary CValidationState to determine accurately the outcome of ProcessBlock, now that it no longer does the full block validity check f877aaaf16
Rename RPC_TRANSACTION_* errors to RPC_VERIFY_* and use RPC_VERIFY_ERROR for submitblock d29a2917ff
luke-jr force-pushed on Oct 21, 2014
luke-jr force-pushed on Oct 21, 2014
laanwj added the label RPC on Oct 22, 2014
in src/rpcprotocol.h:None in d29a2917ff outdated
```
  54 | -    RPC_TRANSACTION_ALREADY_IN_CHAIN= -27, // Transaction already in chain
  55 | +    RPC_VERIFY_ERROR                = -25, // General error during transaction or block submission
  56 | +    RPC_VERIFY_REJECTED             = -26, // Transaction or block was rejected by network rules
  57 | +    RPC_VERIFY_ALREADY_IN_CHAIN     = -27, // Transaction already in chain
  58 | +
  59 | +    // Aliases for backward compatibility
```
TheBlueMatt commented at 8:07 PM on October 27, 2014:

Ehh, if you're gonna rename something, why not just rename it instead of aliasing?

luke-jr commented at 11:31 PM on October 27, 2014:

Backward compatibility?

TheBlueMatt commented at 12:20 AM on October 28, 2014:

Its only a code change, not exposed to users, no?

luke-jr commented at 12:24 AM on October 28, 2014:

This enum needs to be exposed to users somewhere/somehow (possibly by reimplementation) for it to be of use... it's an external interface, after all.

TheBlueMatt commented at 12:42 AM on October 28, 2014:

Huh??? Only the numbers are exosed, not the names.

luke-jr commented at 12:56 AM on October 28, 2014:

Technically, perhaps. Does it help if you consider this a form of documentation?

TheBlueMatt commented at 12:57 AM on October 28, 2014:

'dafuq? Comments count there too...this argument makes no sense, I'll just leave this pull alone.

laanwj commented at 6:50 AM on October 28, 2014:

@TheBlueMatt developers refer to this file to make constants in, for example, Python or other language bindings for the different error codes. Having codes change name suddenly may be confusing, so I'm in favour of documenting it in some way (I don't care if this is done with aliases, or just a comment, but I'm fine with the current way).
in src/rpcmining.cpp:None in d29a2917ff outdated
```
 582 | -    if (!fAccepted)
 583 | +    UnregisterValidationInterface(&sc);
 584 | +    if (fAccepted)
 585 | +    {
 586 | +        if (!sc.found)
 587 | +            return "inconclusive";
```
TheBlueMatt commented at 8:22 PM on October 27, 2014:

inconclusive is...well...inconclusive...maybe "not best chain, validity not checked" or so.

laanwj commented at 8:34 PM on October 27, 2014:

As the result is likely machine-parsed, let's not return long messages here - It would have been better to use status codes instead of strings, but as we're stuck with strings let's return short identifiers and not whole English messages.

luke-jr commented at 11:33 PM on October 27, 2014:

@laanwj Normally rejection reasons would be human-parsed, although "inconclusive" may be a special case that machines may want to parse/handle.

TheBlueMatt commented at 12:21 AM on October 28, 2014:

@laanwj I dont think thats so long as to make it at all hard to machine parse? though may we want to skip the ","...

luke-jr commented at 12:25 AM on October 28, 2014:

inconclusive-<reason> would fit with regular rejection reasons, but then we have to track the actual reason it's inconclusive, which isn't something we can determine from here...

laanwj commented at 7:57 AM on October 28, 2014:

@TheBlueMatt It's not about hardness to parse, but the likelyness people will reword the message because it seems informal and directed at humans. Could also add a comment that the exact message is part of the interface and it should not be changed, of course.
in src/rpcmining.cpp:None in d29a2917ff outdated
```
 591 | +    {
 592 | +        std::string strRejectReason = state.GetRejectReason();
 593 | +        throw JSONRPCError(RPC_VERIFY_ERROR, strRejectReason);
 594 | +    }
 595 | +    if (state.IsInvalid())
 596 |          return "rejected"; // TODO: report validation state
```
TheBlueMatt commented at 8:24 PM on October 27, 2014:

We now have the validation state...want to fix this one now?

luke-jr commented at 11:34 PM on October 27, 2014:

@TheBlueMatt #3727 will do that - this PR is just fixing a new bug introduced since 0.9.
in src/rpcmining.cpp:None in d29a2917ff outdated
```
 575 | @@ -558,8 +576,22 @@ Value submitblock(const Array& params, bool fHelp)
 576 |      }
 577 |  
 578 |      CValidationState state;
 579 | +    submitblock_StateCatcher sc(pblock.GetHash());
 580 | +    RegisterValidationInterface(&sc);
 581 |      bool fAccepted = ProcessBlock(state, NULL, &pblock);
```
TheBlueMatt commented at 8:31 PM on October 27, 2014:

Can we get some contract from ProcessBlock (in the form of a comment) that indicates that it will fully process every dependent block before returning (ie not push work to a background thread?)

laanwj commented at 8:33 PM on October 27, 2014:

Right, that was my comment as well. If the notification arrives asynchronously this wouldn't work.

luke-jr commented at 11:35 PM on October 27, 2014:

@sipa ^

sipa commented at 3:50 AM on October 28, 2014:

The current contract for ProcessBlock is that it only returns after the best known valid block is made active - taking the newly processed block into account. Adding that as a comment (which implies that it currently can't do any asynchronous processing) sounds fine to me.

luke-jr commented at 7:42 AM on October 28, 2014:

Ok, added a commit renaming ProcessBlock (since its behaviour changed after 0.9) and documenting it.
sipa commented at 7:45 AM on October 28, 2014: member

ProcessBlock is not only used for incoming blocks...
Rename ProcessBlock to ProcessNewBlock to indicate change of behaviour, and document it 1bea2bbddc
luke-jr force-pushed on Oct 28, 2014
luke-jr commented at 8:15 AM on October 28, 2014: member

Ok, fixed the new name to be more accurate.
sipa commented at 10:07 AM on November 3, 2014: member

ACK the new name
laanwj commented at 10:44 AM on November 3, 2014: member

I don't really like the idea of registering and unregistering signals for every submitblock call. I don't see any functional issues with this - signals connecting/disconnection is thread-safe - but it's not how signals are supposed to be used. Signal calls are cheap, but connection/disconnection may have a significant overhead. After discussion on IRC: the reason it makes sense this way is to prepare for future asynchronous handling, where the RPC would wait on a condition value raised by the signal.

utACK
laanwj merged this on Nov 3, 2014
laanwj closed this on Nov 3, 2014
laanwj referenced this in commit 84d26d3a36 on Nov 3, 2014
luke-jr deleted the branch on Jan 1, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#5083 master submitblock fails to detect some validation failures #5105 Rename CWalletInterface to CValidationInterface