Add a pong message that is sent in reply to a ping. #932

mikehearn commented at 10:14 PM on March 11, 2012: contributor

It echoes back a nonce field that is now added to the ping 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. 991193a07c

sipa commented at 11:27 PM on March 11, 2012: member

I suppose this also requires an increase of the network protocol number.

Looks good to me, otherwise.

mikehearn commented at 11:49 PM on March 11, 2012: contributor

Yes, I was assuming that would happen automatically once 0.7 starts development.

sipa commented at 12:04 AM on March 12, 2012: member

Since BIP14 was introduced, client versions and network versions are independent. In fact, the network version hasn't changed since.

TheBlueMatt commented at 10:25 PM on March 12, 2012: member

Ive always kinda wondered why we have a ping command that doesnt do anything...does this need to be BIPified? It probably needs a version number bump, so I guess?

mikehearn commented at 8:19 AM on March 13, 2012: contributor

Yeah the patch is incomplete. I'll add another commit soon. We need to send a nonce with outbound pings too. On Mar 12, 2012 11:25 PM, "Matt Corallo" < reply@reply.github.com> wrote:

Ive always kinda wondered why we have a ping command that doesnt do anything...does this need to be BIPified? It probably needs a version number bump, so I guess?

Reply to this email directly or view it on GitHub: #932 (comment)

Bump protocol version. Send a nonce of zero in ping messages. 2fef9dac62

mikehearn commented at 5:46 PM on March 13, 2012: contributor

How does it look now?

TheBlueMatt commented at 6:03 PM on March 13, 2012: member

Given a post to the mailing list with no significant dissent, ack.

in src/serialize.h:None in 2fef9dac62

  52 | @@ -53,7 +53,7 @@
  53 |  class CAutoFile;
  54 |  static const unsigned int MAX_SIZE = 0x02000000;
  55 |  
  56 | -static const int PROTOCOL_VERSION = 60000;
  57 | +static const int PROTOCOL_VERSION = 70000;

TheBlueMatt commented at 7:44 PM on March 13, 2012:

Do we really want to increment a full major version...not that it matters, but it seems excessive...

rebroad commented at 4:45 PM on March 20, 2012:

Is there any documentation on PROTOCOL_VERSION I can refer to please? How is this decided and how does it cope with branches?

gavinandresen commented at 9:50 PM on March 13, 2012: contributor

ACK for 0.7

rebroad commented at 4:46 PM on March 20, 2012: contributor

Not sure if this is the right place to ask, but what exactly is being pinged and by what?

Diapolo commented at 5:20 PM on March 20, 2012: none

Your node by a remote node.

rebroad commented at 5:22 PM on March 20, 2012: contributor

How are nodes uniquely identified in order to know how to route the ping?

Diapolo commented at 5:27 PM on March 20, 2012: none

I'm sure it's not like an ICMP ping, but a ping via the normal "message flow" between nodes, so if nodes can talk to eachother this ping would work. I think of it as a heartbeat check-alive signal.

rebroad commented at 5:57 PM on March 20, 2012: contributor

Ok, so if it's not a ping, what is it? Is it a check-still-responding then? What would be the impact/harm of this change not happening? I think the answer to this last question should always be included by default in the description of any pull request, IMHO.

sipa commented at 5:57 PM on March 20, 2012: member

It checks whether the connection is alive.

Diapolo commented at 6:03 PM on March 20, 2012: none

It's a feature, not a bug-fix, so it extends the nodes possibilities to interact with eachother.

rebroad commented at 6:07 PM on March 20, 2012: contributor

I'm fine with clients having extra functionality added which isn't part of the core bitcoin protocol, but this change confuses me as there's an increase in the protocol number associated with it. I'd understood bitcoin wasn't supposed to be changed in ways that weren't part of the core functionality, i.e. transferring and storing of money/bitcoins, otherwise what is to stop the protocol from becoming taken over by other non-core functionality such as things like torchat, etc (which I'd not be against, but I do think it should be transmitted over a separate protocol so that non-compatible clients don't have to deal with the noise). If it goes away from the core basis of bitcoin, I think it should be bitcoin protocol independant.

Based on the "it's a feature, not a bug-fix", is my assumption that it's not core bitcoin functionality correct?

sipa commented at 6:16 PM on March 20, 2012: member

Bitcoin chose to implement its own network layer: the p2p protocol that's part of its functionality. I believe this was worth it, but it also means this layer must work well and be maintained, even if it's not its goal but only a medium to achieve it. As Mike noted, it would be useful to have a ping reply functionality at this layer, and I see no reason to object to it.

rebroad commented at 6:37 PM on March 20, 2012: contributor

Well, I agree that if there's a ping, there ought to be a pong, otherwise, what does the ping alone achieve? I do agree that a connection needs a way to determine if it's still active/operatonal or taking up limited (file descriptors, memory, bandwidth, etc) space with no benefit. If this does that, then I support this change.

jgarzik commented at 8:27 PM on April 10, 2012: contributor

ACK, but IMO this needs a BIP

sipa commented at 8:28 PM on April 10, 2012: member

@jgarzik: agree

mikehearn commented at 1:25 PM on April 11, 2012: contributor

That seems a little heavy, but alright:

https://en.bitcoin.it/wiki/BIP_0031

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

I just created a fork that includes a few minor changes, in pull req #1081:

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.

mikehearn commented at 5:05 PM on April 11, 2012: contributor

Why should the version increment be in a different change to the actual new functionality? Doesn't that just increase the risk of confusion with people running betas/dev builds? It's a number, incrementing it is cheap :-)

2/3 - sounds good to me.

gmaxwell closed this on May 8, 2012

sipa commented at 8:38 PM on May 8, 2012: member

merged as #1081.

dexX7 referenced this in commit 6df462774f on May 14, 2019

lateminer referenced this in commit 924ec4f6dd on Oct 30, 2019

DrahtBot locked this on Sep 8, 2021