Skip to content

BIP155 follow-ups #20119

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 12, 2020
Merged

BIP155 follow-ups #20119

merged 2 commits into from
Oct 12, 2020

Conversation

sipa
Copy link
Member

@sipa sipa commented Oct 11, 2020

This:

  • Documents BIP155 support in doc/bips.md
  • Restricts addrv2 relay to IPv4, IPv6, and Tor addresses. Relaying addresses in ranges that no network software has support for seems like a gratuitous spam vector.

@fanquake fanquake added the P2P label Oct 11, 2020
hebasto
hebasto reviewed Oct 11, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Concept ACK.

hebasto
hebasto approved these changes Oct 11, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 7ca0e87

jonatack
jonatack reviewed Oct 11, 2020
View reviewed changes
@jonatack
Copy link
Member

jonatack commented Oct 11, 2020 •
edited
Loading

Concept ACK.

I wondered a little why the additional "should we relay this address?" logic is in RelayAddress() instead in the caller along with the existing logic. (Edit: domain separation and/or more elegant code, I suppose. It's certainly not a blocker.)

src/net_processing.cpp::L2660

            bool fReachable = IsReachable(addr);
            if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 && addr.IsRoutable())
            {
                // Relay to a limited number of other nodes
                RelayAddress(addr, fReachable, m_connman);
            }

@sipa sipa force-pushed the 202010_bip155_followup branch from 7ca0e87 to 56f9dba Compare October 11, 2020 18:29
@jonatack
Copy link
Member

ACK 56f9dba

hebasto
hebasto approved these changes Oct 12, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 56f9dba, verified both links.

@naumenkogs
Copy link
Member

ACK 56f9dba

@fanquake fanquake merged commit af22322 into bitcoin:master Oct 12, 2020
vasild
vasild reviewed Oct 12, 2020
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

(post merge) ACK 56f9dba

This patch achieves the same as the one at the bottom of #19954 (comment) and in addition the patch in this PR will relay I2P addresses if they become reachable in the future without needing an adjustment.

Given that the newly added function IsRelayable() deals with a higher level logic of whether to relay or not, maybe it would be better if it lives somewhere in net_processing.cc, next to RelayAddress() instead of as a method of the lower level CNetAddr class. But it works anyway and can be adjusted further if needed.

Thanks!

@jonatack
Copy link
Member

Further BIP155 follow-ups and minor bugfix at #20120.

vasild added a commit to vasild/bitcoin that referenced this pull request Jun 10, 2021
@vasild
net: relay I2P addresses even if not reachable (by us)
ba45f02 
Nodes that can reach the I2P network (have set `-i2psam=`) will relay
I2P addresses even without this patch. However, nodes that can't reach
the I2P network will not. This was done as a precaution in
bitcoin#20119 before anybody could
connect to I2P because then, for sure, it would have been useless.

Now, however, we have I2P support and a bunch of I2P nodes, so get all
nodes on the network to relay I2P addresses to help with propagation,
similarly to what we do with Tor addresses.
@vasild vasild mentioned this pull request Jun 10, 2021
Merged
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021
@vasild @luke-jr
net: relay I2P addresses even if not reachable (by us)
0ffe412 
Nodes that can reach the I2P network (have set `-i2psam=`) will relay
I2P addresses even without this patch. However, nodes that can't reach
the I2P network will not. This was done as a precaution in
bitcoin#20119 before anybody could
connect to I2P because then, for sure, it would have been useless.

Now, however, we have I2P support and a bunch of I2P nodes, so get all
nodes on the network to relay I2P addresses to help with propagation,
similarly to what we do with Tor addresses.

Github-Pull: bitcoin#22211
Rebased-From: ba45f02
laanwj added a commit to bitcoin-core/gui that referenced this pull request Jul 15, 2021
@laanwj
Merge bitcoin/bitcoin#22211: net: relay I2P addresses even if not rea…
a88fa1a 
…chable (by us)

7593b06 test: ensure I2P addresses are relayed (Vasil Dimov)
e746813 test: make CAddress in functional tests comparable (Vasil Dimov)
33e211d test: implement ser/unser of I2P addresses in functional tests (Vasil Dimov)
8674281 test: use NODE_* constants instead of magic numbers (Vasil Dimov)
ba45f02 net: relay I2P addresses even if not reachable (by us) (Vasil Dimov)

Pull request description:

  Nodes that can reach the I2P network (have set `-i2psam=`) will relay
  I2P addresses even without this patch. However, nodes that can't reach
  the I2P network will not. This was done as a precaution in
  bitcoin/bitcoin#20119 before anybody could
  connect to I2P because then, for sure, it would have been useless.

  Now, however, we have I2P support and a bunch of I2P nodes, so get all
  nodes on the network to relay I2P addresses to help with propagation,
  similarly to what we do with Tor addresses.

ACKs for top commit:
  jonatack:
    ACK 7593b06
  naumenkogs:
    ACK 7593b06.
  laanwj:
    Code review ACK 7593b06
  kristapsk:
    ACK 7593b06. Code looks correct, tested that functional test suite passes and also that `test/functional/p2p_addrv2_replay.py` fails if I undo changes in `IsRelayable()`.

Tree-SHA512: c9feec4a9546cc06bc2fec6d74f999a3c0abd3d15b7c421c21fcf2d610eb94611489e33d61bdcd5a4f42041a6d84aa892f7ae293b0d4f755309a8560b113b735
hebasto pushed a commit to hebasto/gui-qml that referenced this pull request Jul 19, 2021
@vasild @hebasto
net: relay I2P addresses even if not reachable (by us)
cb443c7 
Nodes that can reach the I2P network (have set `-i2psam=`) will relay
I2P addresses even without this patch. However, nodes that can't reach
the I2P network will not. This was done as a precaution in
bitcoin/bitcoin#20119 before anybody could
connect to I2P because then, for sure, it would have been useless.

Now, however, we have I2P support and a bunch of I2P nodes, so get all
nodes on the network to relay I2P addresses to help with propagation,
similarly to what we do with Tor addresses.
jarolrod pushed a commit to jarolrod/gui-qml that referenced this pull request Jul 19, 2021
@vasild @jarolrod
net: relay I2P addresses even if not reachable (by us)
20bf2d3 
Nodes that can reach the I2P network (have set `-i2psam=`) will relay
I2P addresses even without this patch. However, nodes that can't reach
the I2P network will not. This was done as a precaution in
bitcoin/bitcoin#20119 before anybody could
connect to I2P because then, for sure, it would have been useless.

Now, however, we have I2P support and a bunch of I2P nodes, so get all
nodes on the network to relay I2P addresses to help with propagation,
similarly to what we do with Tor addresses.
josibake pushed a commit to josibake/bitcoin that referenced this pull request Jul 21, 2021
@vasild @josibake
net: relay I2P addresses even if not reachable (by us)
74e076b 
Nodes that can reach the I2P network (have set `-i2psam=`) will relay
I2P addresses even without this patch. However, nodes that can't reach
the I2P network will not. This was done as a precaution in
bitcoin#20119 before anybody could
connect to I2P because then, for sure, it would have been useless.

Now, however, we have I2P support and a bunch of I2P nodes, so get all
nodes on the network to relay I2P addresses to help with propagation,
similarly to what we do with Tor addresses.
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 23, 2021
@laanwj
Merge bitcoin#22211: net: relay I2P addresses even if not reachable (…
ae9a90b 
…by us)

7593b06 test: ensure I2P addresses are relayed (Vasil Dimov)
e746813 test: make CAddress in functional tests comparable (Vasil Dimov)
33e211d test: implement ser/unser of I2P addresses in functional tests (Vasil Dimov)
8674281 test: use NODE_* constants instead of magic numbers (Vasil Dimov)
ba45f02 net: relay I2P addresses even if not reachable (by us) (Vasil Dimov)

Pull request description:

  Nodes that can reach the I2P network (have set `-i2psam=`) will relay
  I2P addresses even without this patch. However, nodes that can't reach
  the I2P network will not. This was done as a precaution in
  bitcoin#20119 before anybody could
  connect to I2P because then, for sure, it would have been useless.

  Now, however, we have I2P support and a bunch of I2P nodes, so get all
  nodes on the network to relay I2P addresses to help with propagation,
  similarly to what we do with Tor addresses.

ACKs for top commit:
  jonatack:
    ACK 7593b06
  naumenkogs:
    ACK 7593b06.
  laanwj:
    Code review ACK 7593b06
  kristapsk:
    ACK 7593b06. Code looks correct, tested that functional test suite passes and also that `test/functional/p2p_addrv2_replay.py` fails if I undo changes in `IsRelayable()`.

Tree-SHA512: c9feec4a9546cc06bc2fec6d74f999a3c0abd3d15b7c421c21fcf2d610eb94611489e33d61bdcd5a4f42041a6d84aa892f7ae293b0d4f755309a8560b113b735
janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Oct 29, 2021
@vasild @janus
net: relay I2P addresses even if not reachable (by us)
184583b 
Nodes that can reach the I2P network (have set `-i2psam=`) will relay
I2P addresses even without this patch. However, nodes that can't reach
the I2P network will not. This was done as a precaution in
bitcoin/bitcoin#20119 before anybody could
connect to I2P because then, for sure, it would have been useless.

Now, however, we have I2P support and a bunch of I2P nodes, so get all
nodes on the network to relay I2P addresses to help with propagation,
similarly to what we do with Tor addresses.
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 10, 2021
@sipa @PiRK
BIP155 follow-ups
5a00698 
Summary:
> Mention BIP155 in doc/bips.md

> Only relay IPv4, IPv6, Tor addresses

This is a backport of [[bitcoin/bitcoin#20119 | core#20119]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10438
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto approved these changes

+2 more reviewers

@vasild vasild vasild left review comments

@jonatack jonatack jonatack left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sipa @jonatack @naumenkogs @vasild @hebasto @fanquake