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

BIP155: extra-clarify the semantic of sendaddrv2 #1134

Merged
merged 1 commit into from
Jul 2, 2021

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Jun 16, 2021

BIP155: extra-clarify the semantic of sendaddrv2.

This is also how the code works currently.

There seems to be some misinterpretation of the current wording in BIP155. Clarify that (hopefully).

Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept and approach ACK

bip-0155.mediawiki Outdated Show resolved Hide resolved
@vasild vasild force-pushed the bip155_clarify_sendaddrv2 branch from b4f30fd to 337114b Compare June 16, 2021 11:59
@vasild
Copy link
Contributor Author

vasild commented Jun 16, 2021

b4f30fd...337114b: address suggestion

@laanwj
Copy link
Member

laanwj commented Jun 16, 2021

ACK, this was how I imagined it was.

@jnewbery
Copy link
Contributor

NACK. This sentence is confusing and unhelpful to implementers. You could equally (to use your analogy in bitcoin/bitcoin#22245 (comment)) update the BIP to say "Sending or not sending this message does not imply any preference with respect to red apples or green apples". It'd be equally true, and equally unhelpful for anyone implementing the spec.

@jonatack
Copy link
Contributor

ACK 337114b

@@ -131,7 +131,7 @@ See the appendices for the address encodings to be used for the various networks

==Signaling support and compatibility==

Introduce a new message type <code>sendaddrv2</code>. Sending such a message indicates that a node can understand and prefers to receive <code>addrv2</code> messages instead of <code>addr</code> messages. I.e. "Send me addrv2".
Introduce a new message type <code>sendaddrv2</code>. Sending such a message indicates that a node can understand and prefers to receive <code>addrv2</code> messages instead of <code>addr</code> messages. I.e. "Send me addrv2". Sending or not sending this message does not imply any preference with respect to receiving unrequested address messages.
Copy link
Contributor

@harding harding Jun 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird to say "prefers to receive" and then contradict that by saying "does not imply any preference", even if those two statements are referring to different aspects of the same thing. I think the change you are suggesting is much more effectively made by deleting words than by adding them.

Suggested change
Introduce a new message type <code>sendaddrv2</code>. Sending such a message indicates that a node can understand and prefers to receive <code>addrv2</code> messages instead of <code>addr</code> messages. I.e. "Send me addrv2". Sending or not sending this message does not imply any preference with respect to receiving unrequested address messages.
Introduce a new message type <code>sendaddrv2</code>. Sending such a message indicates that a node can understand <code>addrv2</code> messages.

I personally don't think that change makes sense. We don't care about what a node understands or doesn't understand, we only care about what it wants us to send it (or doesn't want us to send it). I think the BIP would be much more useful if it said:

Suggested change
Introduce a new message type <code>sendaddrv2</code>. Sending such a message indicates that a node can understand and prefers to receive <code>addrv2</code> messages instead of <code>addr</code> messages. I.e. "Send me addrv2". Sending or not sending this message does not imply any preference with respect to receiving unrequested address messages.
Introduce a new message type <code>sendaddrv2</code>. Sending such a message indicates that a node wants to receive <code>addrv2</code> messages. Any node receiving <code>addrv2</code> messages should not also receive <code>addr</code> messages.

I don't think that represents a behavior change. I think that provides an opportunity for a useful behavior change on the part of the requesting node, but doesn't represent any behavior change on the part of the receiving node, which is the part where I think we need to be especially careful. The final paragraph of this section still conflicts with the policy described in the OP of bitcoin/bitcoin#21528 , but that final paragraph is to my eyes only a description of how to maintain compatibility with Bitcoin Core's previous un-BIP'd behavior, not a prescription for how all BIP155-conforming nodes should implement addr gossip in the future.

Edit: clarified "behavior change" remarks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this:

Sending such a message signals support for the addrv2 format - a node can create such messages and understand (parse) them.

Given that some addresses can only be represented in addrv2 format and that IPv4 addresses' representation is more compact, a sender, if they choose to send an address message for whatever reason, may find it more suitable to use addrv2 instead of addr if both peers have signaled addrv2 support.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this:

Sending such a message signals support for the addrv2 format - a node can create such messages and understand (parse) them.

Agree -- would omit "Send me addrv2" or replace it with "I support addrv2"

@luke-jr luke-jr merged commit ee0f1c3 into bitcoin:master Jul 2, 2021
@vasild vasild deleted the bip155_clarify_sendaddrv2 branch July 5, 2021 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants