validation: Remove REJECT code from CValidationState #17004

pull jnewbery wants to merge 5 commits into bitcoin:master from jnewbery:2019-09-no-reject-validation-state changing 17 files +137 −140
  1. jnewbery commented at 9:45 pm on September 30, 2019: member

    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 5d08c9c579ba8cc7b684105c6a08263992b08d52.

  2. jnewbery commented at 9:46 pm on September 30, 2019: member
    ~Builds on #15437. Please review that PR first.~
  3. fanquake added the label Validation on Sep 30, 2019
  4. in doc/bips.md:18 in 6f8876e155 outdated 
    14@@ -15,7 +15,6 @@ BIPs that are implemented by Bitcoin Core (up-to-date up to **v0.18.0**):
15 * [`BIP 35`](https://github.com/bitcoin/bips/blob/master/bip-0035.mediawiki): The 'mempool' protocol message (and the protocol version bump to 60002) has been implemented since **v0.7.0** ([PR #1641](https://github.com/bitcoin/bitcoin/pull/1641)). As of **v0.13.0**, this is only available for `NODE_BLOOM` (BIP 111) peers.
16 * [`BIP 37`](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki): The bloom filtering for transaction relaying, partial Merkle trees for blocks, and the protocol version bump to 70001 (enabling low-bandwidth SPV clients) has been implemented since **v0.8.0** ([PR #1795](https://github.com/bitcoin/bitcoin/pull/1795)). Disabled by default since **v0.19.0**, can be enabled by the `-peerbloomfilters` option.
17 * [`BIP 42`](https://github.com/bitcoin/bips/blob/master/bip-0042.mediawiki): The bug that would have caused the subsidy schedule to resume after block 13440000 was fixed in **v0.9.2** ([PR #3842](https://github.com/bitcoin/bitcoin/pull/3842)).
18-* [`BIP 61`](https://github.com/bitcoin/bips/blob/master/bip-0061.mediawiki): The 'reject' protocol message (and the protocol version bump to 70002) was added in **v0.9.0** ([PR #3185](https://github.com/bitcoin/bitcoin/pull/3185)). Starting **v0.17.0**, whether to send reject messages can be configured with the `-enablebip61` option, and support is deprecated as of **v0.18.0**.
    laanwj commented at 10:06 am on October 1, 2019:
    IMO it’d be more clear, for historical context, to add to this line that support was removed, than to remove this line
    jnewbery commented at 1:20 pm on October 1, 2019:
    This comment is for the base PR (https://github.com/bitcoin/bitcoin/pull/15437). I’ve copied it there: #15437 (review)
  5. laanwj added this to the milestone 0.20.0 on Oct 1, 2019
  6. DrahtBot commented at 8:41 pm on October 3, 2019: member

    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.

  7. in src/rpc/rawtransaction.cpp:909 in 6f8876e155 outdated 
    905@@ -906,7 +906,7 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
906     result_0.pushKV("allowed", test_accept_res);
907     if (!test_accept_res) {
908         if (state.IsInvalid()) {
909-            result_0.pushKV("reject-reason", strprintf("%i: %s", state.GetRejectCode(), state.GetRejectReason()));
910+            result_0.pushKV("reject-reason", strprintf("%s", state.GetRejectReason()));
    ryanofsky commented at 4:01 pm on October 8, 2019:

    In commit “[validation] Remove REJECT code from CValidationState” (6f8876e155024181a455fc457ae94a38d0da3626)

    I don’t think it’s necessary, but you could potentially mention this change in release notes, in case someone is relying on the RPC string having a certain format.

    jnewbery commented at 8:24 pm on October 8, 2019:
    Done
  8. ryanofsky approved
  9. ryanofsky commented at 4:04 pm on October 8, 2019: member
    Code review ACK 6f8876e155024181a455fc457ae94a38d0da3626 (last commit only)
  10. jnewbery commented at 8:24 pm on October 8, 2019: member
    Thanks for the review @ryanofsky . I’ve updated the release notes as requested.
  11. laanwj commented at 10:22 am on October 9, 2019: member
    Nice cleanup.
  12. jnewbery force-pushed on Oct 9, 2019
  13. jnewbery commented at 2:12 pm on October 9, 2019: member
    Rebased
  14. ryanofsky approved
  15. ryanofsky commented at 5:10 pm on October 9, 2019: member
    Code review ACK 982ff1a424efefcd7403ad1fb2b1206fd5297035. No changes since last review other than rebase and adding release notes
  16. ryanofsky commented at 5:20 pm on October 9, 2019: member

    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

  17. MarcoFalke added the label P2P on Oct 9, 2019
  18. MarcoFalke commented at 5:31 pm on October 9, 2019: member
    Was about to ask the same question, thanks @ryanofsky
  19. jnewbery force-pushed on Oct 9, 2019
  20. jnewbery commented at 7:22 pm on October 9, 2019: member

    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.

  21. ryanofsky commented at 8:22 pm on October 9, 2019: member

    Code review ACK d938bb4f0208ec629122d0b87f45cad3c8b58089. 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.

  22. jnewbery commented at 8:27 pm on October 9, 2019: member
    Updated the PR description. Thanks for the review!
  23. in doc/release-notes-15437.md:43 in d938bb4f02 outdated 
    38+
39+* `testmempoolaccept` and `sendrawtransaction` no longer return the P2P REJECT
40+  code when a transaction is not accepted to the mempool. They still return the
41+  verbal reject reason.
42+
43+* Log messages that previously reported the REJECT reason when a transaction was
    ariard commented at 4:00 am on October 10, 2019:
    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 ?
    jnewbery commented at 1:57 pm on October 10, 2019:
    Sorry, this should say REJECT code, not REJECT reason. I’ll update the docs.
  24. ariard approved
  25. ariard commented at 4:05 am on October 10, 2019: member
    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.
  26. in src/net_processing.cpp:1241 in 43022a5d2a outdated 
    1239-        if (it != mapBlockSource.end() && State(it->second.first) && state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) {
1240+    // If the block failed validation, we know where it came from and we're still connected
1241+    // to that peer, maybe punish.
1242+    if (state.IsInvalid() &&
1243+        it != mapBlockSource.end() &&
1244+        State(it->second.first)) {
    ryanofsky commented at 12:20 pm on October 10, 2019:
    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
    jnewbery commented at 1:57 pm on October 10, 2019:
    Good idea. I’ll do that. That’ll also make it easier to back-port the bugfix if we want to do that.
  27. [validation] Fix peer punishment for bad blocks 
    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
5d08c9c579ba8cc7b684105c6a08263992b08d52 which did punish nodes that
sent us CACHED_INVALID and BLOCK_MISSING_PREV blocks.
    a1a07cfe99
  28. [logging] Don't log REJECT code when transaction is rejected 
    Remove the BIP61 REJECT code from error messages and logs when a
transaction is rejected.

BIP61 support was removed from Bitcoin Core in
fa25f43ac5692082dba3f90456c501eb08f1b75c. The REJECT codes will be
removed from the codebase entirely in the following commit.
    0053e16714
  29. in src/validation.h:783 in 43022a5d2a outdated 
    782@@ -783,9 +783,6 @@ int32_t ComputeBlockVersion(const CBlockIndex* pindexPrev, const Consensus::Para
783  * for transactions, to signal internal conditions. They cannot and should not
    fjahr commented at 3:31 pm on October 10, 2019:
    Comment should also be removed
    jnewbery commented at 5:32 pm on October 10, 2019:
    Thanks. Fixed!
  30. fjahr approved
  31. fjahr commented at 5:00 pm on October 10, 2019: member

    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.

  32. [validation] Remove REJECT code from CValidationState 
    We no longer send BIP 61 REJECT messages, so there's no need to set
a REJECT code in the CValidationState object.
    e9d5a59e34
  33. [validation] Fix REJECT message comments 04a2f326ec
  34. [docs] Add release notes for removal of REJECT reasons 9075d13153
  35. jnewbery force-pushed on Oct 10, 2019
  36. jnewbery commented at 5:32 pm on October 10, 2019: member
    Fixed all @ryanofsky @ariard @fjahr comments. Thanks for the review!
  37. ryanofsky approved
  38. ryanofsky commented at 5:53 pm on October 10, 2019: member
    Code review ACK 9075d13153ce06cd59a45644831ecc43126e1e82. Only changes since last review are splitting the main commit and updating comments
  39. ariard commented at 8:42 pm on October 10, 2019: member
    ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits
  40. fjahr commented at 5:53 pm on October 21, 2019: member
    ACK 9075d13153ce06cd59a45644831ecc43126e1e82, confirmed diff to last review was fixing nits in docs/comments.
  41. laanwj referenced this in commit b688b859db on Oct 24, 2019
  42. laanwj merged this on Oct 24, 2019
  43. laanwj closed this on Oct 24, 2019
  44. MarcoFalke commented at 5:14 pm on October 24, 2019: member

    It looks like the runtime sanity assert

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

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

  45. MarcoFalke commented at 5:18 pm on October 24, 2019: member
    Oh, I see we used to punish peers when the reject code was 0. Post-merge ACK
  46. jnewbery deleted the branch on Oct 24, 2019
  47. sidhujag referenced this in commit 7ab8446ce1 on Oct 26, 2019
  48. MarcoFalke referenced this in commit a2b282c9d0 on Apr 12, 2020
  49. jasonbcox referenced this in commit 8a980e61c9 on Oct 29, 2020
  50. deadalnix referenced this in commit aaa6b819c0 on Oct 30, 2020
  51. jasonbcox referenced this in commit 70ed7b16a2 on Oct 30, 2020
  52. deadalnix referenced this in commit 3ee9aa1dbc on Oct 30, 2020
  53. sidhujag referenced this in commit ba3af76ce9 on Nov 10, 2020
  54. DrahtBot locked this on Dec 14, 2022


jnewbery laanwj DrahtBot ryanofsky MarcoFalke ariard fjahr

Labels
P2P Validation

Milestone
0.20.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-29 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me