Skip to content

net: relay I2P addresses even if not reachable (by us) #22211

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
laanwj merged 5 commits into from
Jul 15, 2021
Merged

net: relay I2P addresses even if not reachable (by us) #22211

laanwj merged 5 commits into from
Jul 15, 2021

Conversation

@vasild
Copy link
Contributor

@vasild vasild commented Jun 10, 2021 •
edited by laanwj
Loading

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
#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
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.
@fanquake fanquake added the P2P label Jun 10, 2021
@vasild
Copy link
Contributor Author

vasild commented Jun 10, 2021

cc @sipa

@vasild
Copy link
Contributor Author

vasild commented Jun 10, 2021

Would be good to get this in 22.0.

kristapsk
kristapsk approved these changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@kristapsk kristapsk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK ba45f02

jonatack
jonatack reviewed Jun 10, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK ba45f02 IsRelayable() is only called by RelayAddress() in the codebase. My I2P+onion+clearnet node has a slowly increasing number of I2P connections, both inbound and outbound, currently 9-10, but the number will increase once v22 is released and it seems good to relay them.

More test coverage may be good.

@vasild
test: use NODE_* constants instead of magic numbers
8674281 
We just assigned `NODE_NETWORK | NODE_WITNESS` to `nServices` a few
lines above. Use that for verifying correctness instead of `9`.
@vasild
Copy link
Contributor Author

vasild commented Jun 11, 2021

ba45f02708..823b85e158: append tests to ensure the behavior

More test coverage may be good.

Here it is! :)

jonatack
jonatack reviewed Jun 11, 2021
View reviewed changes
jonatack
jonatack reviewed Jun 11, 2021
View reviewed changes
jonatack
jonatack reviewed Jun 11, 2021
View reviewed changes
Copy link
Member

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

ACK 823b85e158161279f053e38935e7033d7d0a23a1 verified the test fails without ba45f02 with sending addrv2 (118 bytes) peer=1 (118 bytes instead of 159)

test failure without ba45f02

2021-06-11T16:15:19.507000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_znhw4kg4
2021-06-11T16:15:20.028000Z TestFramework (INFO): Create connection that sends addrv2 messages
2021-06-11T16:15:20.131000Z TestFramework (INFO): Send too-large addrv2 message
2021-06-11T16:15:20.188000Z TestFramework (INFO): Check that addrv2 message content is relayed and added to addrman
2021-06-11T16:15:22.319000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 128, in main
    self.run_test()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/p2p_addrv2_relay.py", line 78, in run_test
    addr_receiver.wait_for_addrv2()
  File "/usr/lib/python3.9/contextlib.py", line 124, in __exit__
    next(self.gen)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 400, in assert_debug_log
    self._raise_assertion_error('Expected messages "{}" does not partially match log:\n\n{}\n\n'.format(str(expected_msgs), print_log))
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 166, in _raise_assertion_error
    raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] Expected messages "['Added 9 addresses from 127.0.0.1: 0 tried', 'received: addrv2 (159 bytes) peer=0', 'sending addrv2 (159 bytes) peer=1']" does not partially match log:

 - 2021-06-11T16:15:20.296366Z [msghand] [net_processing.cpp:2404] [ProcessMessage] received: addrv2 (159 bytes) peer=0
 - 2021-06-11T16:15:20.296802Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.0 mapped to AS0 belongs to new bucket 628
 - 2021-06-11T16:15:20.296892Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.1 mapped to AS0 belongs to new bucket 628
 - 2021-06-11T16:15:20.296976Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.2 mapped to AS0 belongs to new bucket 628
 - 2021-06-11T16:15:20.297026Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.3 mapped to AS0 belongs to new bucket 628
 - 2021-06-11T16:15:20.297075Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.4 mapped to AS0 belongs to new bucket 628
 - 2021-06-11T16:15:20.297122Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.6 mapped to AS0 belongs to new bucket 628
 - 2021-06-11T16:15:20.297165Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.7 mapped to AS0 belongs to new bucket 628
 - 2021-06-11T16:15:20.297217Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.8 mapped to AS0 belongs to new bucket 628
 - 2021-06-11T16:15:20.297266Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.9 mapped to AS0 belongs to new bucket 628
 - 2021-06-11T16:15:20.297293Z [msghand] [addrman.h:532] [Add] Added 9 addresses from 127.0.0.1: 0 tried, 9 new
 - 2021-06-11T16:15:20.297604Z [msghand] [net_processing.cpp:2404] [ProcessMessage] received: ping (8 bytes) peer=0
 - 2021-06-11T16:15:20.297661Z [msghand] [net.cpp:2959] [PushMessage] sending pong (8 bytes) peer=0
 - 2021-06-11T16:15:20.347034Z [http] [httpserver.cpp:237] [http_request_cb] Received a POST request for / from 127.0.0.1:35120
 - 2021-06-11T16:15:20.347555Z [httpworker.0] [rpc/request.cpp:174] [parse] ThreadRPCServer method=setmocktime user=__cookie__
 - 2021-06-11T16:15:20.352683Z (mocktime: 2021-06-11T16:45:20Z) [opencon] [net.cpp:401] [ConnectNode] trying connection 123.123.123.8:8341 lastseen=2.5hrs
 - 2021-06-11T16:15:20.398215Z (mocktime: 2021-06-11T16:45:20Z) [msghand] [net.cpp:2959] [PushMessage] sending ping (8 bytes) peer=0
 - 2021-06-11T16:15:20.398608Z (mocktime: 2021-06-11T16:45:20Z) [msghand] [net.cpp:2959] [PushMessage] sending ping (8 bytes) peer=1
 - 2021-06-11T16:15:20.398852Z (mocktime: 2021-06-11T16:45:20Z) [msghand] [net.cpp:2959] [PushMessage] sending addrv2 (118 bytes) peer=1
 - 2021-06-11T16:15:20.399445Z (mocktime: 2021-06-11T16:45:20Z) [msghand] [net_processing.cpp:2404] [ProcessMessage] received: pong (8 bytes) peer=0
 - 2021-06-11T16:15:20.399591Z (mocktime: 2021-06-11T16:45:20Z) [msghand] [net_processing.cpp:2404] [ProcessMessage] received: pong (8 bytes) peer=1

vasild added 3 commits June 11, 2021 19:16
@vasild
test: implement ser/unser of I2P addresses in functional tests
33e211d
@vasild
test: make CAddress in functional tests comparable
e746813 
This way we can compare CAddress objects using `==` or even
arrays of CAddress using `array1 == array2`.
@vasild
test: ensure I2P addresses are relayed
7593b06 
This test would fail if `CNetAddr::IsRelayable()` returns `false` for
I2P addresses, given that this test node does not have I2P connectivity.
@vasild vasild force-pushed the i2p_IsRelayable branch from 823b85e to 7593b06 Compare June 11, 2021 17:20
@vasild
Copy link
Contributor Author

vasild commented Jun 11, 2021

823b85e158...7593b06bd1: address review suggestions and pet the linter

@jonatack
Copy link
Member

jonatack commented Jun 12, 2021

ACK 7593b06

kristapsk
kristapsk approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

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().

@naumenkogs
Copy link
Contributor

naumenkogs commented Jun 18, 2021

ACK 7593b06.
Thank you for taking care of i2p.

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
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021
@vasild @luke-jr
test: implement ser/unser of I2P addresses in functional tests
ba3ea56 
Github-Pull: bitcoin#22211
Rebased-From: 33e211d
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021
@vasild @luke-jr
test: make CAddress in functional tests comparable
0d29255 
This way we can compare CAddress objects using `==` or even
arrays of CAddress using `array1 == array2`.

Github-Pull: bitcoin#22211
Rebased-From: e746813
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 29, 2021
@vasild @luke-jr
test: ensure I2P addresses are relayed
b639380 
This test would fail if `CNetAddr::IsRelayable()` returns `false` for
I2P addresses, given that this test node does not have I2P connectivity.

Github-Pull: bitcoin#22211
Rebased-From: 7593b06
@jonatack jonatack mentioned this pull request Jul 8, 2021
Merged
@laanwj laanwj added this to the 22.0 milestone Jul 8, 2021
@laanwj
Copy link
Member

laanwj commented Jul 15, 2021

Code review ACK 7593b06

@laanwj laanwj merged commit a88fa1a into bitcoin:master Jul 15, 2021
@vasild vasild deleted the i2p_IsRelayable branch July 16, 2021 06:44
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 1, 2022
@sipa @PiRK
Rate limit the processing of incoming addr messages
a1509f7 
Summary:
> While limitations on the influence of attackers on addrman already
> exist (affected buckets are restricted to a subset based on incoming
> IP / network group), there is no reason to permit them to let them
> feed us addresses at more than a multiple of the normal network
> rate.
>
> This commit introduces a "token bucket" rate limiter for the
> processing of addresses in incoming ADDR and ADDRV2 messages.
> Every connection gets an associated token bucket. Processing an
> address in an ADDR or ADDRV2 message from non-whitelisted peers
> consumes a token from the bucket. If the bucket is empty, the
> address is ignored (it is not forwarded or processed). The token
> counter increases at a rate of 0.1 tokens per second, and will
> accrue up to a maximum of 1000 tokens (the maximum we accept in a
> single ADDR or ADDRV2). When a GETADDR is sent to a peer, it
> immediately gets 1000 additional tokens, as we actively desire many
> addresses from such peers (this may temporarily cause the token
> count to exceed 1000).
>
> The rate limit of 0.1 addr/s was chosen based on observation of
> honest nodes on the network. Activity in general from most nodes
> is either 0, or up to a maximum around 0.025 addr/s for recent
> Bitcoin Core nodes. A few (self-identified, through subver) crawler
> nodes occasionally exceed 0.1 addr/s.

> Randomize the order of addr processing

> Functional tests for addr rate limiting

> Add logging and addr rate limiting statistics
>
> Includes logging improvements by Vasil Dimov and John Newbery.

> Improve addr relay tests using statistics

Note: this backport bypasses an intermediate change in `p2p_addrv2_relay.py` from [[bitcoin/bitcoin#22211 | core#22211]]

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

Test Plan:
`ninja all check-all`
IBD tests

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10941
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge a88fa1a into merged_master (Bitcoin PR bitcoin/bitcoin#22211)
6361a13
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@jonatack jonatack jonatack left review comments

@kristapsk kristapsk kristapsk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P

Projects

None yet

Milestone

22.0

Development

Successfully merging this pull request may close these issues.

6 participants

@vasild @jonatack @naumenkogs @laanwj @kristapsk @fanquake