validation: Remove REJECT code from CValidationState #17004

jnewbery · 2019-09-30T21:45:53Z

We no longer send BIP 61 REJECT messages, so there's no need to set
a REJECT code in the CValidationState object.

Note that there is a minor bug fix in p2p behaviour here. Because the
call to MaybePunishNode() in PeerLogicValidation::BlockChecked() only
previously happened if the REJECT code was > 0 and < REJECT_INTERNAL,
then there are cases were MaybePunishNode() can get called where it
wasn't previously:

when AcceptBlockHeader() fails with CACHED_INVALID.
when AcceptBlockHeader() fails with BLOCK_MISSING_PREV.

Note that BlockChecked() cannot fail with an 'internal' reject code. The
only internal reject code was REJECT_HIGHFEE, which was only set in
ATMP.

This reverts a minor bug introduced in 5d08c9c.

jnewbery · 2019-09-30T21:46:07Z

~~Builds on #15437. Please review that PR first.~~

doc/bips.md

DrahtBot · 2019-10-03T20:41:28Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17080 (consensus: Explain why fCheckDuplicateInputs can not be skipped by MarcoFalke)
#16688 (log: Add validation interface logging by jkczyz)
#16401 (Package relay by sdaftuar)
#15921 (validation: Tidy up ValidationState interface by jnewbery)
#13525 (policy: Report reason inputs are nonstandard from AreInputsStandard by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ryanofsky

Code review ACK 6f8876e (last commit only)

src/rpc/rawtransaction.cpp

jnewbery · 2019-10-08T20:24:53Z

Thanks for the review @ryanofsky . I've updated the release notes as requested.

laanwj · 2019-10-09T10:22:06Z

Nice cleanup.

jnewbery · 2019-10-09T14:12:49Z

Rebased

ryanofsky

Code review ACK 982ff1a. No changes since last review other than rebase and adding release notes

ryanofsky · 2019-10-09T17:20:30Z

there are cases were MaybePunishNode() can get called where it
wasn't previously

I didn't see this in my previous reviews, and I'm not really sure what the implications are. It might help for the comment to say what the original reason for not calling MaybePunish was, and why it's no longer applicable or important now

maflcko · 2019-10-09T17:31:44Z

Was about to ask the same question, thanks @ryanofsky

jnewbery · 2019-10-09T19:22:16Z

It looks like the bug was introduced here: https://github.com/bitcoin/bitcoin/pull/10135/files. That PR fixed a bug where we'd send out REJECT messages with a reject code of 0 (not valid in BIP 61), but it also inadvertently stopped us from punishing nodes that sent a block that failed here https://github.com/jnewbery/bitcoin/blob/5d08c9c579ba8cc7b684105c6a08263992b08d52/src/validation.cpp#L3077 and here https://github.com/jnewbery/bitcoin/blob/5d08c9c579ba8cc7b684105c6a08263992b08d52/src/validation.cpp#L3088 where the reject code wasn't set in the state.Invalid() call.

This PR restores that old behaviour of maybe punishing peers that send us blocks that fail CACHED_INVALID or BLOCK_MISSING_PREV. There's already code to handle those failure reasons in MaybePunishNode().

I've updated the commit log with more detail, and added a comment in the net_processing BlockChecked() callback.

ryanofsky · 2019-10-09T20:22:52Z

Code review ACK d938bb4. Only comment and formatting changes since my last review. Thanks for digging up the history!

IIUC, could maybe update the PR description to clarify that the "minor change in p2p behaviour" is a bugfix.

jnewbery · 2019-10-09T20:27:36Z

Updated the PR description. Thanks for the review!

ariard

ACK d938bb4. Reviewed and tested code, grep'ed to see any reference to reject-code, included in tests, also checked that there weren't any other cases like CACHED_INVALID and BLOCK_MISSING_PREV returning a reject-code of 0.

ariard · 2019-10-10T04:00:21Z

doc/release-notes-15437.md

+  code when a transaction is not accepted to the mempool. They still return the
+  verbal reject reason.
+
+* Log messages that previously reported the REJECT reason when a transaction was


Not sure about this statement, at least the reject-reason should be logged for the broadcastTransaction call in SubmitMemoryPoolAndRelay, which ones were you thinking off ?

Sorry, this should say REJECT code, not REJECT reason. I'll update the docs.

ryanofsky · 2019-10-10T12:20:07Z

src/net_processing.cpp

-    if (state.IsInvalid()) {
-        // Don't send reject message with code 0 or an internal reject code.
-        if (it != mapBlockSource.end() && State(it->second.first) && state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) {
+    // If the block failed validation, we know where it came from and we're still connected
+    // to that peer, maybe punish.
+    if (state.IsInvalid() &&
+        it != mapBlockSource.end() &&
+        State(it->second.first)) {


Just a suggestion, but imo it wouldn't be a bad idea to break this commit up to separate the behavior changes from the refactoring changes. It probably says something bad about my own code review practices, but I completely missed this change the first time I first reviewed and ACKed this PR. It seems like this commit could nicely be broken into 3 commits: 1) P2P behavior change 2) RPC and logging changes 3) Refactor to remove reject constants arguments

Good idea. I'll do that. That'll also make it easier to back-port the bugfix if we want to do that.

Because the call to MaybePunishNode() in PeerLogicValidation::BlockChecked() only previously happened if the REJECT code was > 0 and < REJECT_INTERNAL, then there are cases were MaybePunishNode() can get called where it wasn't previously: - when AcceptBlockHeader() fails with CACHED_INVALID. - when AcceptBlockHeader() fails with BLOCK_MISSING_PREV. Note that BlockChecked() cannot fail with an 'internal' reject code. The only internal reject code was REJECT_HIGHFEE, which was only set in ATMP. This change restores the behaviour pre-commit 5d08c9c which did punish nodes that sent us CACHED_INVALID and BLOCK_MISSING_PREV blocks.

Remove the BIP61 REJECT code from error messages and logs when a transaction is rejected. BIP61 support was removed from Bitcoin Core in fa25f43. The REJECT codes will be removed from the codebase entirely in the following commit.

fjahr

ACK d938bb4

Code review and ran and reviewed tests.

Just one comment was probably overlooked and +1 for moving the behavior change into a separate commit.

fjahr · 2019-10-10T15:31:37Z

src/validation.h

@@ -783,9 +783,6 @@ int32_t ComputeBlockVersion(const CBlockIndex* pindexPrev, const Consensus::Para
 * for transactions, to signal internal conditions. They cannot and should not


Comment should also be removed

Thanks. Fixed!

We no longer send BIP 61 REJECT messages, so there's no need to set a REJECT code in the CValidationState object.

jnewbery · 2019-10-10T17:32:30Z

Fixed all @ryanofsky @ariard @fjahr comments. Thanks for the review!

ryanofsky

Code review ACK 9075d13. Only changes since last review are splitting the main commit and updating comments

ariard · 2019-10-10T20:42:30Z

ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits

fjahr · 2019-10-21T17:53:33Z

ACK 9075d13, confirmed diff to last review was fixing nits in docs/comments.

9075d13 [docs] Add release notes for removal of REJECT reasons (John Newbery) 04a2f32 [validation] Fix REJECT message comments (John Newbery) e9d5a59 [validation] Remove REJECT code from CValidationState (John Newbery) 0053e16 [logging] Don't log REJECT code when transaction is rejected (John Newbery) a1a07cf [validation] Fix peer punishment for bad blocks (John Newbery) Pull request description: We no longer send BIP 61 REJECT messages, so there's no need to set a REJECT code in the CValidationState object. Note that there is a minor bug fix in p2p behaviour here. Because the call to `MaybePunishNode()` in `PeerLogicValidation::BlockChecked()` only previously happened if the REJECT code was > 0 and < `REJECT_INTERNAL`, then there are cases were `MaybePunishNode()` can get called where it wasn't previously: - when `AcceptBlockHeader()` fails with `CACHED_INVALID`. - when `AcceptBlockHeader()` fails with `BLOCK_MISSING_PREV`. Note that `BlockChecked()` cannot fail with an 'internal' reject code. The only internal reject code was `REJECT_HIGHFEE`, which was only set in ATMP. This reverts a minor bug introduced in 5d08c9c. ACKs for top commit: ariard: ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits fjahr: ACK 9075d13, confirmed diff to last review was fixing nits in docs/comments. ryanofsky: Code review ACK 9075d13. Only changes since last review are splitting the main commit and updating comments Tree-SHA512: 58e8a1a4d4e6f156da5d29fb6ad6a62fc9c594bbfc6432b3252e962d0e9e10149bf3035185dc5320c46c09f3e49662bc2973ec759679c0f3412232087cb8a3a7

maflcko · 2019-10-24T17:14:27Z

It looks like the runtime sanity assert

assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes

was transformed into a less fatal if check in 5d08c9c and now removed completely. What is the reason for removing it?

maflcko · 2019-10-24T17:18:22Z

Oh, I see we used to punish peers when the reject code was 0. Post-merge ACK

…tate

b1b0cfe test: Remove REJECT message code (Hennadii Stepanov) Pull request description: We no longer use REJECT p2p message: - bitcoin#15437 - bitcoin#17004 ACKs for top commit: theStack: ACK bitcoin@b1b0cfe (nice dead code find) Tree-SHA512: 0a662b282e921c3991aeb15f54d077837f1ef20bc2e3b0b35117bb97a21d1bd1c3e21458e5c18ba0ca02030d559e3e8e74dbd3d3e2b46dbe7bede550948c3b55

Summary: > Because the call to MaybePunishNode() in > PeerLogicValidation::BlockChecked() only previously happened if the > REJECT code was > 0 and < REJECT_INTERNAL, then there are cases were > MaybePunishNode() can get called where it wasn't previously: > > - when AcceptBlockHeader() fails with CACHED_INVALID. > - when AcceptBlockHeader() fails with BLOCK_MISSING_PREV. > > Note that BlockChecked() cannot fail with an 'internal' reject code. The > only internal reject code was REJECT_HIGHFEE, which was only set in > ATMP. > > This change restores the behaviour pre-commit > bitcoin/bitcoin@5d08c9c > which did punish nodes that > sent us CACHED_INVALID and BLOCK_MISSING_PREV blocks. This is a backport of Core [[bitcoin/bitcoin#17004 | PR17004]] [1/5] Commit bitcoin/bitcoin@a1a07cf Test Plan: `ninja all check-all` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8181

Summary: > We no longer send BIP 61 REJECT messages, so there's no need to set > a REJECT code in the CValidationState object. This is a backport of Core [[bitcoin/bitcoin#17004 | PR17004]] [3/5] Commit bitcoin/bitcoin@e9d5a59 Depends on D8182 Test Plan: `ninja all check-all` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8183

Summary: Comments change only. This is a backport of Core [[bitcoin/bitcoin#17004 | PR17004]] [4/5] Commit bitcoin/bitcoin@04a2f32 Depends on D8183 Test Plan: `ninja` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8184

Summary: Add some release notes related to the user-facing changes introduced by [[bitcoin/bitcoin#17004 | PR17004]]. The original 5th commit of the PR also added release notes related to changes in other PR, which were backported for our previous release (0.22.5). We only mention the changes to logging. This concludes the backport of Core [[bitcoin/bitcoin#17004 | PR17004]] [5/5] Inspired by commit bitcoin/bitcoin@9075d13 Depends on D8184 Test Plan: Proof-reading Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8185

…tate

fanquake added the Validation label Sep 30, 2019

laanwj reviewed Oct 1, 2019

View reviewed changes

doc/bips.md Outdated Show resolved Hide resolved

laanwj added this to the 0.20.0 milestone Oct 1, 2019

This was referenced Oct 1, 2019

p2p: Remove BIP61 reject messages #15437

Merged

validation: Tidy up ValidationState interface #15921

Merged

ryanofsky approved these changes Oct 8, 2019

View reviewed changes

src/rpc/rawtransaction.cpp Show resolved Hide resolved

jnewbery force-pushed the 2019-09-no-reject-validation-state branch from 3f4c6b8 to 982ff1a Compare October 9, 2019 14:12

ryanofsky approved these changes Oct 9, 2019

View reviewed changes

maflcko added the P2P label Oct 9, 2019

jnewbery force-pushed the 2019-09-no-reject-validation-state branch from 982ff1a to d938bb4 Compare October 9, 2019 19:20

ariard approved these changes Oct 10, 2019

View reviewed changes

ryanofsky reviewed Oct 10, 2019

View reviewed changes

jnewbery added 2 commits October 10, 2019 11:17

fjahr approved these changes Oct 10, 2019

View reviewed changes

jnewbery added 3 commits October 10, 2019 13:31

[validation] Remove REJECT code from CValidationState

e9d5a59

We no longer send BIP 61 REJECT messages, so there's no need to set a REJECT code in the CValidationState object.

[validation] Fix REJECT message comments

04a2f32

[docs] Add release notes for removal of REJECT reasons

9075d13

jnewbery force-pushed the 2019-09-no-reject-validation-state branch from d938bb4 to 9075d13 Compare October 10, 2019 17:32

ryanofsky approved these changes Oct 10, 2019

View reviewed changes

laanwj merged commit 9075d13 into bitcoin:master Oct 24, 2019

jnewbery deleted the 2019-09-no-reject-validation-state branch October 24, 2019 17:19

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 26, 2019

Merge bitcoin#17004: validation: Remove REJECT code from CValidationS…

7ab8446

…tate

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

hebasto mentioned this pull request Apr 12, 2020

test: Remove REJECT message code #18609

Merged

ariard mentioned this pull request May 22, 2020

[WIP] rebase: Call ProcessNewBlock() asynchronously #18963

Closed

2 tasks

sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020

Merge bitcoin#17004: validation: Remove REJECT code from CValidationS…

ba3af76

…tate

bitcoin locked and limited conversation to collaborators Dec 14, 2022

		@@ -783,9 +783,6 @@ int32_t ComputeBlockVersion(const CBlockIndex* pindexPrev, const Consensus::Para
		* for transactions, to signal internal conditions. They cannot and should not

validation: Remove REJECT code from CValidationState #17004

validation: Remove REJECT code from CValidationState #17004

Uh oh!

Conversation

jnewbery commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented Oct 8, 2019

Uh oh!

laanwj commented Oct 9, 2019

Uh oh!

jnewbery commented Oct 9, 2019

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Oct 9, 2019

Uh oh!

maflcko commented Oct 9, 2019

Uh oh!

jnewbery commented Oct 9, 2019

Uh oh!

ryanofsky commented Oct 9, 2019

Uh oh!

jnewbery commented Oct 9, 2019

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ariard Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

fjahr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjahr Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 10, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ariard commented Oct 10, 2019

Uh oh!

fjahr commented Oct 21, 2019

Uh oh!

maflcko commented Oct 24, 2019

Uh oh!

maflcko commented Oct 24, 2019

Uh oh!

Uh oh!

jnewbery commented Sep 30, 2019 •

edited

Loading

jnewbery commented Sep 30, 2019 •

edited

Loading

DrahtBot commented Oct 3, 2019 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky left a comment •

edited

Loading

fjahr left a comment •

edited

Loading