New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add absurdly high fee message to validation state #5913

Merged
merged 1 commit into from Aug 5, 2015

Conversation

Projects
None yet
6 participants
@shaulkf
Contributor

shaulkf commented Mar 17, 2015

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 17, 2015

Member

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.

Member

sipa commented Mar 17, 2015

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.

@shaulkf

This comment has been minimized.

Show comment
Hide comment
@shaulkf

shaulkf Mar 17, 2015

Contributor

@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)

Contributor

shaulkf commented Mar 17, 2015

@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)

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 17, 2015

Member

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.

Member

sipa commented Mar 17, 2015

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.

@shaulkf

This comment has been minimized.

Show comment
Hide comment
@shaulkf

shaulkf Mar 17, 2015

Contributor

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.

Contributor

shaulkf commented Mar 17, 2015

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.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 18, 2015

Member

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

Member

laanwj commented Mar 18, 2015

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

@laanwj laanwj added the RPC/REST/ZMQ label Mar 18, 2015

@shaulkf

This comment has been minimized.

Show comment
Hide comment
@shaulkf

shaulkf Mar 18, 2015

Contributor

@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.

Contributor

shaulkf commented Mar 18, 2015

@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.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 24, 2015

Member

@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.

Member

laanwj commented Mar 24, 2015

@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.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 8, 2015

Member

utACK

Member

laanwj commented Apr 8, 2015

utACK

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 24, 2015

Member

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).

Member

luke-jr commented Jun 24, 2015

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).

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 24, 2015

Member

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()};
                                                        ^
Member

luke-jr commented Jun 24, 2015

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()};
                                                        ^
@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 26, 2015

Contributor

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.

Contributor

paveljanik commented Jun 26, 2015

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.

@shaulkf

This comment has been minimized.

Show comment
Hide comment
@shaulkf

shaulkf Jun 26, 2015

Contributor

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

Contributor

shaulkf commented Jun 26, 2015

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

@shaulkf

This comment has been minimized.

Show comment
Hide comment
@shaulkf

shaulkf Jun 30, 2015

Contributor

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.

Contributor

shaulkf commented Jun 30, 2015

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.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 2, 2015

Member

@shaulkf Please fix the warning.

Member

luke-jr commented Jul 2, 2015

@shaulkf Please fix the warning.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jul 2, 2015

Contributor

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;
 };
Contributor

paveljanik commented Jul 2, 2015

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;
 };
@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 2, 2015

Member

Not sure, that might change the protocol. Maybe:

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

luke-jr commented Jul 2, 2015

Not sure, that might change the protocol. Maybe:

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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jul 2, 2015

Contributor

Right.

Contributor

paveljanik commented Jul 2, 2015

Right.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 5, 2015

Member

Need this for #6519

Member

laanwj commented Aug 5, 2015

Need this for #6519

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 5, 2015

Member

utACK.

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

Member

sipa commented Aug 5, 2015

utACK.

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 5, 2015

Member

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

Member

laanwj commented Aug 5, 2015

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

@laanwj laanwj merged commit a651403 into bitcoin:master Aug 5, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 5, 2015

Merge pull request #5913
5922b67 Add assertion and cast before sending reject code (Wladimir J. van der Laan)
a651403 Add absurdly high fee message to validation state (for RPC propagation) (Shaul Kfir)
@@ -497,4 +497,7 @@ extern CBlockTreeDB *pblocktree;
*/
int GetSpendHeight(const CCoinsViewCache& inputs);
/** local "reject" message codes for RPC which can not be triggered by p2p trasactions */

This comment has been minimized.

@Diapolo

Diapolo Aug 6, 2015

Nit: Spelling error trasactions.
See theuni#48

@theuni Will there be a Trivial pull before 0.11?

@Diapolo

Diapolo Aug 6, 2015

Nit: Spelling error trasactions.
See theuni#48

@theuni Will there be a Trivial pull before 0.11?

laanwj added a commit to laanwj/bitcoin that referenced this pull request Dec 7, 2015

net: Fix sent reject messages for blocks and transactions
Ever since we #5913 have been sending invalid reject messages
for transactions and blocks.

laanwj added a commit that referenced this pull request Dec 10, 2015

net: Fix sent reject messages for blocks and transactions
Ever since we #5913 have been sending invalid reject messages
for transactions and blocks.

test: Add basic test for `reject` code

Extend P2P test framework to make it possible to expect reject
codes for transactions and blocks.

Github-Pull: #7179
Rebased-From: 9fc6ed6 2041190
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment