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.

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

P2p: peer discouragement #1451

Merged
merged 3 commits into from
Feb 2, 2024
Merged

P2p: peer discouragement #1451

merged 3 commits into from
Feb 2, 2024

Conversation

ImplOfAnImpl
Copy link
Contributor

@ImplOfAnImpl ImplOfAnImpl commented Jan 15, 2024 •
edited
Loading

  • Discouraged peers are prioritized during eviction and outbound connections are never established to discouraged addresses.
  • Peers are disconnected automatically when discouraged.
  • Inbound connections from discouraged addresses are allowed if the connection number limit hasn't been reached yet.
  • Discouraged and banned addresses are not gossiped via AnnounceAddrRequest and are not included into AddrListResponse.
  • Discouraged addresses are stored in the peer db the same way as banned ones. This is different from what bitcoin core does, where discouraged addresses are kept in a non-persistent rolling bloom filter (which means that the discouragement time is determined by the node's uptime).
  • We no longer ban automatically.
  • When banning a peer manually, the user has to specify the duration explicitly. The duration is parsed via humantime crate, so it can be something like 1y (1 year) or 3M (3 months).
  • Printing banned or discouraged addresses in the wallet now also prints the expiration time.
  • Small improvements in printing of Time - instead of having a separate function as_standard_printable_time it now has custom implementations of Debug and Display. The implementations delegate printing to chrono::DateTime<Utc>, just like as_standard_printable_time, but also handle the case when Time is not representable via chrono::DateTime.
  • I also added rpc & wallet commands to print reserved peer addresses.

ImplOfAnImpl
ImplOfAnImpl commented Jan 15, 2024
View reviewed changes
p2p/src/ban_config.rs Outdated Show resolved Hide resolved
node-lib/src/options.rs Outdated Show resolved Hide resolved
p2p/src/peer_manager/config.rs
PreservedInboundCountNewTransactions, PreservedInboundCountPing,
},
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: except for the TODO about naming, there is nothing new in this file, I've just moved PeerManagerConfig to a separate file.

p2p/src/peer_manager/peerdb/mod.rs Outdated Show resolved Hide resolved
p2p/src/peer_manager/tests/connections.rs
@@ -79,6 +82,136 @@ use crate::{
PeerManagerEvent,
};

async fn validate_invalid_connection<A, S>()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: I just moved this test here from ban.rs

p2p/src/peer_manager/tests/connections.rs
.await;
}

async fn inbound_connection_invalid_magic<A, T>()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: again, this was moved from ban.rs.

@ImplOfAnImpl ImplOfAnImpl marked this pull request as ready for review January 15, 2024 11:56
TheQuantumPhysicist
TheQuantumPhysicist reviewed Jan 16, 2024
View reviewed changes
p2p/src/peer_manager/config.rs
make_config_setting!(EnableFeelerConnections, bool, true);
make_config_setting!(ForceDnsQueryIfNoGlobalAddressesKnown, bool, false);

// TODO: this name is too generic, because not all peer manager settings are contained here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have a discussion about this at some point when you feel it's appropriate. Using words like "internal" can quickly become a mess.

TheQuantumPhysicist
TheQuantumPhysicist reviewed Jan 16, 2024
View reviewed changes
Copy link
Contributor

@TheQuantumPhysicist TheQuantumPhysicist left a comment

Choose a reason for hiding this comment

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

First round looks good. I'll review again when you're done with the details we discussed.

@TheQuantumPhysicist TheQuantumPhysicist marked this pull request as draft January 16, 2024 14:02
@TheQuantumPhysicist TheQuantumPhysicist mentioned this pull request Jan 16, 2024
Closed
@ImplOfAnImpl ImplOfAnImpl force-pushed the p2p_peer_discouragement branch from ecd6f1d to 3e0f528 Compare January 30, 2024 13:44
@ImplOfAnImpl ImplOfAnImpl marked this pull request as ready for review January 30, 2024 14:15
@ImplOfAnImpl
p2p: peer discouragement was added; peer eviction tests were refactor…
3979f62 
…ed; ban tests were rewritten; PeermanagerConfig was moved to a separate file; some cleanup
@ImplOfAnImpl
Discouraged peers are now disconnected automatically;
a3e500d 
Peers are no longer automatically banned;
Ban duration is now always specified explicitly by the user;
Time formatting improvements;
Wallet & rpc commands for printing reserved and discouraged addresses were added;
Printing banned and discouraged addresses now also prints the expiration time;
@ImplOfAnImpl ImplOfAnImpl force-pushed the p2p_peer_discouragement branch from 3e0f528 to a3e500d Compare February 2, 2024 11:50
azarovh
azarovh approved these changes Feb 2, 2024
View reviewed changes
Copy link
Member

@azarovh azarovh left a comment

Choose a reason for hiding this comment

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

Looks good.
I just have one question. Because of the bloom filter in bitcoin you cannot retrieve all the elements. I'm not sure what is exact reason in case of discouragement but we are doing this intentionally, right?

@ImplOfAnImpl
Copy link
Contributor Author

Looks good. I just have one question. Because of the bloom filter in bitcoin you cannot retrieve all the elements. I'm not sure what is exact reason in case of discouragement but we are doing this intentionally, right?

[discussed] yes, we do this intentionally

@ImplOfAnImpl ImplOfAnImpl merged commit e00561b into master Feb 2, 2024
27 checks passed
@ImplOfAnImpl ImplOfAnImpl deleted the p2p_peer_discouragement branch February 2, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azarovh azarovh azarovh approved these changes

@TheQuantumPhysicist TheQuantumPhysicist TheQuantumPhysicist approved these changes

@erubboli erubboli Awaiting requested review from erubboli erubboli is a code owner

@iljakuklic iljakuklic Awaiting requested review from iljakuklic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ImplOfAnImpl @TheQuantumPhysicist @azarovh