Add absurdly high fee message to validation state #5913

Currently error message isn't propagating to RPC. Not sure how reject constants should be numbered, for now I arbitrarily chose 0x44, please advise.

There is no need for a reject code, as this can never trigger for peer-to-peer initiated transactions, so it can never result in a reject message.

If you really need a reject code, add a REJECT_LOCAL or something, reserved for conditions that can only trigger locally (and mark it is an implementation detail, not related to BIP 61.

@sipa I believe there should be a code, in case RPC users want to catch the error without having to match the text. Any convention for numbering or could I choose any arbitrary code? (0x60 perhaps)

Well you can't pick anything that could potentially be later used in the P2P protocol, which why I would prefer no actual code. If there really is a need for RPC error codes separately from what the peer to peer protocol does, perhaps there should just be two codes. In general, I think it's not possible to predict all possible reason for rejecting things, and not really viable to have codes for everything.

I'm not sure what you mean by adding REJECT_LOCAL, should I create an error code which will be overloaded for any local error? How about REJECT_LOCAL = 0x00.

You could use reject codes > 0x100 for local-only conditions. These cannot be passed over the P2P network.

@laanwj Is it okay to change reject message codes to unsigned int for this purpose? If not, please advise is we can assign a dedicated 2 byte code (e.g. 0x00 or 0xFF) for all REJECT_LOCAL errors. I prefer the first solution as RPC users can catch errors with explicit error codes.

@shaulkf Changing the internal type is fine with me, otherwise I wouldn't have suggested it. But be careful that you don't accidentally change the type that is sent in the protocol.

utACK

Needs rebase. I suggest leaving the const reject message codes (now in consensus/validation.h) as unsigned char, changing CValidationState as you already do, and putting the local codes in main.h (for now).

Also:

main.cpp: In function ‘void InvalidBlockFound(CBlockIndex*, const CValidationState&)’:
main.cpp:1369:56: warning: narrowing conversion of ‘(& state)->CValidationState::GetRejectCode()’ from ‘unsigned int’ to ‘unsigned char’ inside { } is ill-formed in C++11 [-Wnarrowing]
             CBlockReject reject = {state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), pindex->GetBlockHash()};
                                                        ^

Needs rebase. I'd prefer to have something in really soon. RPC returns no clean error message and it is boring to look up the actual error in the debug log.

Rebased and changed according to @luke-jr's recommendation

Thanks @paveljanik, pushed a fix and rebased, this is a remnant from a change I ended up reverting. I agree regarding Invalid and DoS but this should be in a separate PR as it requires reordering the arguments.

@shaulkf Please fix the warning.

Something like this needed?

diff --git a/src/main.cpp b/src/main.cpp
index f67f1fd..5229f6f 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -182,7 +182,7 @@ namespace {
 namespace {

 struct CBlockReject {
-    unsigned char chRejectCode;
+    unsigned int chRejectCode;
     string strRejectReason;
     uint256 hashBlock;
 };

Not sure, that might change the protocol. Maybe:

CBlockReject reject = {(unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), pindex->GetBlockHash()};

Right.

Need this for #6519

utACK.

Maybe this also needs an assert before sending out a reject message that the chRejectCode is in the right range.

Yes, good point. I'll add it during merge.