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

p2p: extend inbound eviction protection by network to CJDNS peers #24165

Merged

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Jan 26, 2022

Extend inbound eviction protection for peers connected over CJDNS, as is the case for peers connected via onion, localhost, and I2P since #21261 and #20197. CJDNS peers seem to have better min ping latency than onion and I2P peers but still higher than that of unencrypted IPv4/6 peers and can be disadvantaged under our eviction criteria. They are also very few in number, which is a further reason to protect them, as the goal of this logic is to favorise the diversity of our peer connections. CJDNS support was added in #23077 for the upcoming v23 release.

This commit extends our inbound eviction protection to CJDNS peers to
favorise the diversity of peer connections, as peers connected
through the CJDNS network are otherwise disadvantaged by our eviction
criteria for their higher latency (higher min ping times) relative
to IPv4 and IPv6 peers.

The `networks` array is order-dependent in the case of a tie in
candidate counts between networks; earlier array members receive
priority in the case of a tie.

Therefore, we place CJDNS candidates before I2P, localhost, and onion
ones in terms of opportunity to recover unused remaining protected
slots from the previous iteration, estimating that most nodes allowing
several inbound privacy networks will have more onion, localhost or
I2P peers than CJDNS ones, as CJDNS support is only being added in the
upcoming v23.0 release.
@DrahtBot DrahtBot added the P2P label Jan 26, 2022
@kristapsk
Copy link
Contributor

Concept ACK

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

tACK b7be28c

@laanwj
Copy link
Member

laanwj commented Jan 26, 2022

Concept and code review ACK b7be28c
Code changes look straightforward, thanks for adding tests.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 26, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@jonatack
Copy link
Member Author

Thanks for the ACKs @kristapsk, @w0xlt and @laanwj.

Here is an example at this moment of the min ping latency of inbound IP peers vs CJDNS, Tor and I2P ones.

Screenshot from 2022-01-28 14-11-26

@laanwj laanwj added this to the 23.0 milestone Feb 3, 2022
@bitcoin bitcoin deleted a comment from Fella76 Feb 4, 2022
@bitcoin bitcoin deleted a comment Feb 4, 2022
@ghost
Copy link

ghost commented Feb 14, 2022

Concept ACK

@laanwj laanwj merged commit 8b6cd42 into bitcoin:master Mar 2, 2022
@jonatack jonatack deleted the protect-inbound-cjdns-peers-from-eviction branch March 2, 2022 11:36
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 2, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Mar 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants