Skip to content
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

p2p: Remove BIP61 reject messages #15437

Merged
merged 1 commit into from Oct 9, 2019

Conversation

@MarcoFalke
Copy link
Member

MarcoFalke commented Feb 18, 2019

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 MarcoFalke added the P2P label Feb 18, 2019
@MarcoFalke MarcoFalke added this to the 0.19.0 milestone Feb 18, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Feb 18, 2019

Not for 0.18.0, obviously

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Feb 18, 2019

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 MarcoFalke force-pushed the MarcoFalke:Mf1809-noBip61 branch 2 times, most recently from fa68bce to fa1fb01 Feb 18, 2019
@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Feb 19, 2019

Concept ACK

-134 lines: nice!

Copy link
Member

jnewbery left a comment

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

test/functional/feature_dersig.py Outdated Show resolved Hide resolved
@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Feb 20, 2019

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

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Feb 22, 2019

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

This comment has been minimized.

Copy link
Member

jnewbery commented Feb 22, 2019

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

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Feb 22, 2019

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

This comment has been minimized.

Copy link
Member

jnewbery commented Feb 22, 2019

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. 👍 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 MarcoFalke changed the title p2p: Remove remote debugging code p2p: Remove BIP61 reject messages Feb 25, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Feb 25, 2019

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-noBip61 branch 3 times, most recently from 1bbf4ca to 91d1930 Feb 25, 2019
Copy link
Member

jnewbery left a comment

Email draft looks good Marco.

relies on this feature and you can not use any of the recommended alternatives:

* Testing or debugging of implementations of the Bitcoin P2P network protocol
should be done by inspecting the reject messages that are produced by a

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 25, 2019

Member

Should this say "inspecting the logs ..."?

Might be useful to have an example log here.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 25, 2019

Author Member

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)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 25, 2019

Member

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-noBip61 branch 2 times, most recently from f22e760 to 3458603 Feb 25, 2019
@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-noBip61 branch from 3458603 to 7ab1c61 Mar 5, 2019
@DrahtBot DrahtBot removed the Needs rebase label Mar 5, 2019
@MarcoFalke MarcoFalke modified the milestones: 0.19.0, 0.20.0 Mar 14, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 14, 2019

Moved to 0.20.0 milestone for now

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-noBip61 branch from 07f00ec to fa00728 Jul 12, 2019
@DrahtBot DrahtBot added Needs rebase and removed Needs rebase labels Jul 12, 2019
@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-noBip61 branch from fa00728 to fab8741 Sep 23, 2019
@DrahtBot DrahtBot removed the Needs rebase label Sep 23, 2019
Copy link
Member

jnewbery left a comment

A few minor comments inline. Otherwise ACK fab8741

(for merging after the 0.19 branch)

src/net_processing.cpp Show resolved Hide resolved
src/net_processing.cpp Show resolved Hide resolved
test/functional/feature_dersig.py Outdated Show resolved Hide resolved
@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Sep 30, 2019

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

doc/bips.md Show resolved Hide resolved
@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-noBip61 branch from fab8741 to fa25f43 Oct 2, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Oct 2, 2019

Addressed doc nits by @jnewbery and @laanwj

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Oct 3, 2019

utACK fa25f43

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 3, 2019

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

Copy link
Contributor

ryanofsky left a comment

Code review ACK fa25f43

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?

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 8, 2019

Contributor

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 8, 2019

Author Member

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.

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 9, 2019

Member

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

laanwj added a commit that referenced this pull request Oct 9, 2019
fa25f43 p2p: Remove BIP61 reject messages (MarcoFalke)

Pull request description:

  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.

ACKs for top commit:
  jnewbery:
    utACK fa25f43
  laanwj:
    I'm still not 100% convinced that I like getting rid of BIP61 conceptually, but apparently everyone wants it, code review ACK fa25f43.
  ryanofsky:
    Code review ACK fa25f43

Tree-SHA512: daf55254202925e56be3d6cfb3c1c804e7a82cecb1dd1e5bd7b472bae989fd68ac4f21ec53fc46751353056fd645f7f877bebcb0b40920257991423a3d99e0be
@laanwj laanwj merged commit fa25f43 into bitcoin:master Oct 9, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1809-noBip61 branch Oct 9, 2019
sidhujag added a commit to syscoin/syscoin that referenced this pull request Oct 9, 2019
fa25f43 p2p: Remove BIP61 reject messages (MarcoFalke)

Pull request description:

  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.

ACKs for top commit:
  jnewbery:
    utACK fa25f43
  laanwj:
    I'm still not 100% convinced that I like getting rid of BIP61 conceptually, but apparently everyone wants it, code review ACK fa25f43.
  ryanofsky:
    Code review ACK fa25f43

Tree-SHA512: daf55254202925e56be3d6cfb3c1c804e7a82cecb1dd1e5bd7b472bae989fd68ac4f21ec53fc46751353056fd645f7f877bebcb0b40920257991423a3d99e0be
@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Oct 9, 2019

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

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.