p2p: Remove BIP61 reject messages

MarcoFalke commented at 8:03 pm on February 18, 2019: member

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.

MarcoFalke added the label P2P on Feb 18, 2019

MarcoFalke added this to the milestone 0.19.0 on Feb 18, 2019

MarcoFalke commented at 8:03 pm on February 18, 2019: member

Not for 0.18.0, obviously

DrahtBot commented at 8:13 pm on February 18, 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:

#17004 (validation: Remove REJECT code from CValidationState by jnewbery)
#16839 (Replace Connman and BanMan globals with Node local by ryanofsky)
#16762 (Rust-based Backup over-REST block downloader by TheBlueMatt)
#16442 (Serve BIP 157 compact filters by jimpo)
#16202 (Refactor network message deserialization by jonasschnelli)
#15921 (validation: Tidy up ValidationState interface by jnewbery)
#15206 (Immediately disconnect on invalid net message checksum by jonasschnelli)

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.

MarcoFalke force-pushed on Feb 18, 2019

practicalswift commented at 9:39 am on February 19, 2019: contributor

Concept ACK

-134 lines: nice!

in test/functional/feature_dersig.py:43 in fa1fb01cbb outdated

40@@ -45,7 +41,7 @@ def unDERify(tx):
41 class BIP66Test(BitcoinTestFramework):
42     def set_test_params(self):
43         self.num_nodes = 1
44-        self.extra_args = [['-whitelist=127.0.0.1', '-par=1', '-enablebip61']]  # Use only one script thread to get the exact reject reason for testing
45+        self.extra_args = [['-whitelist=127.0.0.1', '-par=1']]  # Use only one script thread to get the exact reject reason for testing

jnewbery commented at 4:55 pm on February 19, 2019:

nit: update comment to:

0# Use only one script thread to get the exact log message for testing

jnewbery commented at 7:26 pm on September 30, 2019:

not addressed

jnewbery commented at 4:58 pm on February 19, 2019: member

Mild concept ACK.

However, I think this PR should be preceded by discussion on the dev mailing list. It’s possible that organizations are using bitcoind as their border node, and broadcast transactions from internal applications via bitcoind. Perhaps they rely on REJECT messages? @laanwj has also commented that he found the messages indispensable when writing client software: #13134#issue-185175846.

gmaxwell commented at 8:21 am on February 20, 2019: contributor

Mild concept ACK– the loc reduction makes a case for it. If someone were relying on this, I’d really encourage them to not do so, unrelated to if this PR were going in or not. :) The ability to use it during testing/debugging, makes for a case against it.

MarcoFalke commented at 4:31 pm on February 22, 2019: member

Indeed, if someone relies on this in production, I’d rather have them to switch to safer ways of achieving their goal. So imo another reason to remove the “feature”

jnewbery commented at 5:04 pm on February 22, 2019: member

I’d rather have them to switch to safer ways of achieving their goal. So imo another reason to remove the “feature”

I think there’s a difference between encouraging users to change behaviour and pulling a feature out from under them. We really don’t have a good picture of how users use our code, so I’d always err on the side of caution when deprecating features.

I’m going to modify my mild concept ACK with a merge-NACK-until-this-has-been-more-widely-discussed. We’re probably fine, but it seems to me that any P2P change that was added to Bitcoin Core with a BIP should be removed after public discussion.

Here’s how I expect it’ll go: someone will post a mail to the bitcoin-core mailing list saying “I propose to remove REJECT messages in Bitcoin Core v0.19”, no-one will respond, and then we’ll remove them. I’m happy to send that post if you’d prefer not to.

MarcoFalke commented at 5:11 pm on February 22, 2019: member

I am happy to write release notes and a mailing list post, I just didn’t come around to do this yet.

The previous release 0.18.0 already disabled BIP61 by default, so this shouldn’t come off as a massive surprise. Also, I wouldn’t be surprised if the entity that uses the feature isn’t subscribed to the mailing list. Could Bitcoin Optech send out a mail as well?

jnewbery commented at 5:16 pm on February 22, 2019: member

Thanks Marco. Sounds good. I’m very happy to review the post/release notes if you want.

The previous release 0.18.0 already disabled BIP61 by default, so this shouldn’t come off as a massive surprise

Agree. :+1: for staged deprecation of features!

Could Bitcoin Optech send out a mail as well?

We’d certainly include it in our next newsletter if there was a post on the mailing list.

MarcoFalke renamed this:
~~p2p: Remove remote debugging code~~
p2p: Remove BIP61 reject messages
on Feb 25, 2019

MarcoFalke commented at 10:17 pm on February 25, 2019: member

Added a commit with the draft for the mailing list post as requested by @jnewbery

MarcoFalke force-pushed on Feb 25, 2019

in doc/release-notes-15437.md:29 in 91d1930fbf outdated

24+feature, and I would like to remove if from Bitcoin Core to make the codebase
25+slimmer, easier to understand and maintain. Let us know if your application
26+relies on this feature and you can not use any of the recommended alternatives:
27+
28+* Testing or debugging of implementations of the Bitcoin P2P network protocol
29+  should be done by inspecting the reject messages that are produced by a

jnewbery commented at 10:27 pm on February 25, 2019:

Should this say “inspecting the logs …”?

Might be useful to have an example log here.

MarcoFalke commented at 10:34 pm on February 25, 2019:

I guess an example log depends on the message type that was rejected “version”, “tx” or “block” and the reason for rejection. I’d rather not add a specific example, since the logs are not guaranteed to be stable (only used for testing and debugging)

jnewbery commented at 10:42 pm on February 25, 2019:

I think it’d still be helpful, even with a caveat that log messages are not guaranteed to be stable. Obviously up to you!

jnewbery commented at 10:28 pm on February 25, 2019: member

Email draft looks good Marco.

MarcoFalke force-pushed on Feb 25, 2019

DrahtBot added the label Needs rebase on Mar 5, 2019

MarcoFalke force-pushed on Mar 5, 2019

DrahtBot removed the label Needs rebase on Mar 5, 2019

MarcoFalke removed this from the milestone 0.19.0 on Mar 14, 2019

MarcoFalke added this to the milestone 0.20.0 on Mar 14, 2019

MarcoFalke commented at 7:30 pm on March 14, 2019: member

Moved to 0.20.0 milestone for now

DrahtBot added the label Needs rebase on Apr 1, 2019

MarcoFalke force-pushed on Jul 12, 2019

DrahtBot removed the label Needs rebase on Jul 12, 2019

DrahtBot added the label Needs rebase on Jul 19, 2019

MarcoFalke force-pushed on Sep 23, 2019

DrahtBot removed the label Needs rebase on Sep 23, 2019

in src/net_processing.cpp:3241 in fab8741b31 outdated

3188@@ -3238,18 +3189,11 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
3189     return true;
3190 }
3191 
3192-bool PeerLogicValidation::SendRejectsAndCheckIfBanned(CNode* pnode, bool enable_bip61) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

jnewbery commented at 7:23 pm on September 30, 2019:

What’s the reason for removing EXCLUSIVE_LOCKS_REQUIRED(cs_main)?

MarcoFalke commented at 8:07 pm on September 30, 2019:

Lock annotations are ignored and useless when not in the function declaration

in src/net_processing.cpp:3601 in fab8741b31 outdated

3538@@ -3598,11 +3539,12 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3539             }
3540         }
3541 
3542-        TRY_LOCK(cs_main, lockMain); // Acquire cs_main for IsInitialBlockDownload() and CNodeState()

jnewbery commented at 7:24 pm on September 30, 2019:

Why remove this comment?

MarcoFalke commented at 8:05 pm on September 30, 2019:

IsInitialBlockDownload does not require cs_main

jnewbery commented at 7:38 pm on September 30, 2019: member

A few minor comments inline. Otherwise ACK fab8741b31dd2feb1edb1fdcc5263fd2da5b7b18

(for merging after the 0.19 branch)

jnewbery commented at 9:44 pm on September 30, 2019: member

Thanks for the clarifications. ACK once the test comment is corrected.

fanquake added the label Waiting for author on Oct 1, 2019

in doc/bips.md:18 in fab8741b31 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**.

jnewbery commented at 1:19 pm on October 1, 2019:

From @laanwj :

IMO it’d be more clear, for historical context, to add to this line that support was removed, than to remove this line

(https://github.com/bitcoin/bitcoin/pull/17004#discussion_r329976844)

MarcoFalke removed the label Waiting for author on Oct 2, 2019

p2p: Remove BIP61 reject messages fa25f43ac5

MarcoFalke force-pushed on Oct 2, 2019

MarcoFalke commented at 2:40 pm on October 2, 2019: member

Addressed doc nits by @jnewbery and @laanwj

jnewbery commented at 4:57 am on October 3, 2019: member

utACK fa25f43ac5692082dba3f90456c501eb08f1b75c

laanwj commented at 5:04 am on October 3, 2019: member

I’m still not 100% convinced that I like getting rid of BIP61 conceptually, but apparently everyone wants it, code review ACK fa25f43ac5692082dba3f90456c501eb08f1b75c.

in doc/man/bitcoin-qt.1:182 in fa25f43ac5

178@@ -179,10 +179,6 @@ Allow DNS lookups for \fB\-addnode\fR, \fB\-seednode\fR and \fB\-connect\fR (def
179 Query for peer addresses via DNS lookup, if low on addresses (default: 1
180 unless \fB\-connect\fR used)
181 .HP
182-\fB\-enablebip61\fR

ryanofsky commented at 3:47 pm on October 8, 2019:

I think these man files get generated from -help, so there’s no need to make the same updates repeatedly

MarcoFalke commented at 5:05 pm on October 8, 2019:

I think generally it is fine to update them either in the same commit, or wait for the script to bump them later.

Here, I made the changes directly, so that git grep enablebip61 doesn’t return these.

laanwj commented at 9:53 am on October 9, 2019:

yes it’s okay to do this (but also to not do this, for ex. because it can cause easily avoidable merge conflicts, as these are would-be hotspots), similar for updating the bitcoin_en translation

ryanofsky approved

ryanofsky commented at 3:54 pm on October 8, 2019: member

Code review ACK fa25f43ac5692082dba3f90456c501eb08f1b75c

I don’t know much about the BIP process, but should https://github.com/bitcoin/bips/blob/master/bip-0061.mediawiki have some kind of “Status: Obsolete” tag or a separate warning that it shouldn’t be relied on?

laanwj referenced this in commit c08bf2b574 on Oct 9, 2019

laanwj merged this on Oct 9, 2019

laanwj closed this on Oct 9, 2019

MarcoFalke deleted the branch on Oct 9, 2019

sidhujag referenced this in commit ba8ba3bd54 on Oct 9, 2019

jnewbery commented at 9:27 pm on October 9, 2019: member

We should announce that this has been merged on the bitcoin-dev mailing list, to close the https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-March/016701.html thread.

MarcoFalke commented at 6:18 pm on October 16, 2019: member

Done by @jnewbery here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-October/017400.html

Thanks

MarcoFalke referenced this in commit a2b282c9d0 on Apr 12, 2020

deadalnix referenced this in commit eea74d5db5 on Oct 20, 2020

random-zebra referenced this in commit d340e2804b on Jul 25, 2021

MarcoFalke locked this on Feb 15, 2022

p2p: Remove BIP61 reject messages #15437

Conflicts