Page MenuHomePhabricator

p2p: Remove BIP61 reject messages
ClosedPublic

Authored by PiRK on Oct 19 2020, 11:11.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABCeea74d5db50f: p2p: Remove BIP61 reject messages
Summary

Reject messages (BIP 61) appear in the following settings:

  • Parsing of reject messages (in case -debug=net is set, off by default). This has only been used for a single LogPrint call for several releases now. Such logging is completely meaningless to us and should thus be removed.
  • The sending of reject messages (in case -enablebip61 is set, off by default). This can be used to debug a node that is under our control. Instead of hacking this debugging into the p2p protocol, it could be more easily achieved by parsing the debug log. (Use -printtoconsole to have it as stream, or read from the debug.log file like our python function assert_debug_log in the test framework does)

    Having to maintain all of this logic and code to accommodate debugging, which can be achieved by other means a lot easier, is a burden. It makes review on net processing changes a lot harder, since the reject message logic has to be carried around without introducing any errors or DOS vectors.

Backport of Core PR15437

Test Plan

ninja && ninja check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Owners added a reviewer: Restricted Owners Package.Oct 19 2020, 11:11
PiRK requested review of this revision.Oct 19 2020, 11:11
deadalnix requested changes to this revision.Oct 19 2020, 13:20
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
doc/bips.md
18 ↗(On Diff #24778)

The version needs to be updated to match Bitcoin ABC's

src/test/denialofservice_tests.cpp
316 ↗(On Diff #24778)

Revert

test/functional/feature_cltv.py
37 ↗(On Diff #24778)

Why is it still necessary? It seems like there is something specific to our codebase here that need to be cleaned up.

test/functional/feature_dersig.py
20 ↗(On Diff #24778)

dito

44 ↗(On Diff #24778)

There is a comment missing here that likely comes from a missing dependency. What is the plan for that dependency?

This revision now requires changes to proceed.Oct 19 2020, 13:20
test/functional/feature_cltv.py
37 ↗(On Diff #24778)

Found it: https://github.com/bitcoin/bitcoin/pull/14365/files

It is a PR that I ignored previously because the title mentioned Travis. I will backport it for the removal of dead code.

test/functional/feature_dersig.py
20 ↗(On Diff #24778)
44 ↗(On Diff #24778)

It seems this comment and associated parameter -par=1 was left out intentionally in
D3875 (backport PR14119):

Reviewer note: Some parts of the test differ due to changes done in D521. I left out the forced single-threaded validation since it's no longer relevant.

test/functional/feature_cltv.py
37 ↗(On Diff #24778)

Fixed in a separate Diff: D7987

test/functional/feature_dersig.py
20 ↗(On Diff #24778)

Fixed in a separate Diff: D7987

address 2 review items: add a blank line accidentaly removed, use ABC version numbers in bip.md

This revision is now accepted and ready to land.Oct 20 2020, 14:26
This revision was automatically updated to reflect the committed changes.