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

Looks good to me, otherwise.

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

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

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?

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" <
[email protected]>
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)

How does it look now?

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

ACK for 0.7

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

Your node by a remote node.

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

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.

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.

It checks whether the connection is alive.

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

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?

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.

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.

ACK, but IMO this needs a BIP

@jgarzik: agree

That seems a little heavy, but alright:

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

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

1. 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...

2. 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.
3. place that constant in sipa's newly minted version.h, as a central location for future version-related constants like this.

1. 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.

merged as #1081.