BIP 0031: pong message #1081

jgarzik commented at 4:40 PM on April 11, 2012: contributor

Add a pong message that is sent in reply to a ping. It echoes back a nonce field that is now added to the ping message. Send a nonce of zero in ping messages.

Original author: Mike Hearn @ Google

Modified Mike's change to introduce a mild form of protocol documentation in version.h.

jgarzik commented at 4:41 PM on April 11, 2012: contributor

This is intended to supercede pull request #932, for the following minor reasons:

removes the actual protocol version increment. IMO this should be external to the 'pong message' commit.

and under the principle of making code self-documenting...

use a named constant rather than a magic number for version behavior switching. bitcoin code is too full of magic numbers, and we should resist adding more.
place that constant in sipa's newly minted version.h, as a central location for future version-related constants like this.

in src/main.cpp:None in 14492d3684 outdated

2651 | @@ -2652,6 +2652,23 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv)
2652 |  
2653 |      else if (strCommand == "ping")
2654 |      {
2655 | +        if (pfrom->nVersion > BIP0031_VERSION)
2656 | +        {
2657 | +            uint64 nonce = 0;
2658 | +            vRecv >> nonce;

luke-jr commented at 4:48 PM on April 11, 2012:

Isn't the nonce optional, if you don't want a pong?

in src/version.h:None in 14492d3684 outdated

  10 | @@ -11,4 +11,7 @@
  11 |  extern const std::string CLIENT_DATE;
  12 |  extern const int         CLIENT_VERSION;
  13 |  
  14 | +// BIP 0031, pong message, is enabled for all versions AFTER this one
  15 | +const int BIP0031_VERSION = 60000;

luke-jr commented at 4:48 PM on April 11, 2012:

Any reason not to use >= for clarity?

jgarzik commented at 4:58 PM on April 11, 2012:

This matches the original submission, and saw no reason to change it. Using "> 60000" makes us slightly more independent of whatever the next-proto-version is. No strong objections to ">= 70000" though.

jgarzik commented at 9:17 PM on April 11, 2012: contributor

Independent, yes, but where is a better place for version information than version.{h,cpp}?

It is an odd policy that the "version" module excludes certain classes of version information and not others. That seems to violate the Principle of Least Surprise.

sipa commented at 10:01 PM on April 11, 2012: member

Agree, but calling them both "_VERSION" will be very confusing.

jgarzik commented at 11:12 PM on April 11, 2012: contributor

No more confusing than the existing PROTOCOL_VERSION vs. CLIENT_VERSION, each with the _VERSION suffix (which I argue is not confusing at all, but rather a logical naming scheme for each).

If it appeals to one's sense of symmetry, we can move PROTOCOL_VERSION to version.* as well.

laanwj commented at 6:35 AM on April 12, 2012: member

IMO, version.h should contain version information for the current client, that will change over time.

I agree that feature-specific thresholds that will be "written in stone forever" should go somewhere else. Maybe there will eventually be enough for a bip_versions.h :-) (if we tracked down other magic version numbers we could already do so)

Another suggestion would be to rename it to BIP0031_MIN_VERSION to it clear that it is a version threshold (though the comment explains this as well of course...).

jgarzik commented at 2:05 PM on April 12, 2012: contributor

No, we don't need multiple version.* modules in the tree.

sipa commented at 2:20 PM on April 12, 2012: member

To me, mixing the client version (metadata about the project you're compiling) and the network version (a property of the protocol being implemented) in the same module is wrong. It's like having a source file in firefox that encdes both information about the firefox browser version and the differences between HTML4 and HTML5.

The fact that both are called version and have a similar number scheme is an historic artifact, and I would prefer to get them separated rather sooner than later. Maybe remove the moniker '_VERSION' from all protocol-related code, and simply call it "protocol 60000". I agree that we need nice constants for magic protocol switches, but they do not belong in version.h. Really.

jgarzik commented at 2:33 PM on April 12, 2012: contributor

This is being WAY overthought.

It is silly to have multiple version modules, for that will create confusion among outside reviewers.
It is silly to put version constants outside the existing version module, for humans outside the dev team will naturally look for anything version-related in a module called "version."
It is even more silly to rename a protocol version constant to something other than "VERSION". It -is- a version number.

It is not a historical accident that the protocol version is called "PROTOCOL_VERSION". The naming precisely describes its purpose, and changing that name would be detrimental to understanding by outside reviewers.

It is just bonkers to remove the "_VERSION" word from all version-related constants in bitcoin unrelated to client version. That is creating more problems than it is solving.

laanwj commented at 2:59 PM on April 12, 2012: member

You can use the exact same argument that this causes confusion for "outside reviewers". Do we need to increase the BIP0031_VERSION with a new release? How is it different from the CLIENT and PROTOCOL version?... etc.

jgarzik commented at 3:05 PM on April 12, 2012: contributor

Helpfully, there is a comment for outside reviewers in the code I added, solving this imagined problem.

sipa commented at 3:26 PM on April 12, 2012: member

Certainly, a comment saying "This relates to the protocol version being used in P2P connections, and is independent from the client version" is enough to make it acceptable (I see no such comment right now, though...). Still, such constants belong in the networking code, imho.

BIP 0031: pong message

Add a pong message that is sent in reply to a ping. It echoes back a nonce
field that is now added to the ping message. Send a nonce of zero in ping
messages.

Original author: Mike Hearn @ Google

Modified Mike's change to introduce a mild form of protocol documentation in
version.h.

93e447b631

version.h: separate client, net sections. Move more constants to this file.

* move PROTOCOL_VERSION to version.h

* move CLIENT_VERSION* to version.h, make available past cpp stage

* clearly separate client, network version portions of version.h

b87c0fc440

jgarzik commented at 4:28 PM on April 12, 2012: contributor

Updated to add such comments, and further illustrate the usage.

sipa commented at 11:06 PM on April 12, 2012: member

Other magic protocol version constants:

protocol.h: 31402 (nTime in CAddress)
main.cpp: 31402 (avoid requesting addresses from older nodes)
main.cpp: 32000...32400 (don't ask for blocks from these versions)

Replace several network protocol version numbers with named constants

stored in version.h.

Also, a minor CAddress code reformat while we're in there, fixing
some incorrect indentation.

8b09cd3a4d

jgarzik commented at 12:10 AM on April 13, 2012: contributor

added, along with 209

sipa commented at 2:37 PM on April 16, 2012: member

Meh, ACK.

luke-jr commented at 2:50 PM on April 16, 2012: member

Reminder to bump our own PROTOCOL_VERSION to 60001 too...

jgarzik commented at 2:54 PM on April 16, 2012: contributor

@luke-jr yes, that will come in a separate cover

jgarzik referenced this in commit 865a0c1674 on Apr 17, 2012

jgarzik merged this on Apr 17, 2012

jgarzik closed this on Apr 17, 2012

coblee referenced this in commit ad03745a9e on Jul 17, 2012

lateminer referenced this in commit 44ea343b05 on Jan 22, 2019

DrahtBot locked this on Sep 8, 2021