inbound eviction logic / policy. #431
Conversation
D-Stacks
force-pushed
the
eviction_logic
branch
from
892fc09 to
4279555
Compare
…ssary boxing in `from_peers` fn
rework trait, make it more general, remove unnecessary boxing
|
Can you give the reference spec/code that you were inspired from? |
| assert_eq!(by_lowest_rank(&ranks1, &ranks2), Ordering::Equal); | ||
| } | ||
|
|
||
| //TODO: Add tests for the compare strategy functions |
There was a problem hiding this comment.
Can you please add them?
| pub struct EvictionRanks { | ||
| ip_prefix_bucket: f64, | ||
| time_connected: f64, | ||
| last_ping_duration: f64, | ||
| last_block_transfer: f64, | ||
| last_tx_transfer: f64, | ||
| } |
There was a problem hiding this comment.
Can you add a comment for each field?
| /// | ||
| /// **Includes:** [`Self::last_ping_duration`], [`Self::time_connected`], [`Self::last_block_transfer`], [`Self::last_tx_transfer`], and [`Self::ip_prefix_bucket`]. | ||
| #[inline] | ||
| fn all_ranks(&self) -> [f64; 5] { |
There was a problem hiding this comment.
Consider using something like this to avoid forgetting to update this function when adding new fields to the struct
| where | ||
| Iter: Iterator<Item = (&'a Peer, EvictionRanks)> + 'a, | ||
| { | ||
| fn filter_peers<F>(self, amount: usize, compare_fn: F) -> impl Iterator<Item = (&'a Peer, EvictionRanks)> + 'a |
There was a problem hiding this comment.
Why not always use by_lowest_rank? This will simplify the function and the caller behaviour. The function can than be renamed to skip_peers_with_low_rank or something like this
| .skip(amount) | ||
| } | ||
|
|
||
| fn select_peers_weighted<F>(self, amount: usize, weight_fn: F) -> impl Iterator<Item = (&'a Peer, EvictionRanks)> + 'a |
There was a problem hiding this comment.
Same here: by_highest_none_latency_rank can be hard coded
| @@ -188,6 +189,7 @@ impl HandleRelayInvsFlow { | |||
| Err(rule_error) => return Err(rule_error.into()), | |||
| }; | |||
|
|
|||
| self.router.set_last_block_transfer(last_block_transfer); | |||
There was a problem hiding this comment.
I think we should also add it to blocks that were received during IBD (but not in the proof stage)
| @@ -13,14 +13,16 @@ pub struct PeerProperties { | |||
| pub time_offset: i64, | |||
| } | |||
|
|
|||
| #[derive(Debug)] | |||
| #[derive(Debug, Eq, PartialEq, Hash)] | |||
There was a problem hiding this comment.
I don't think this should be the default behaviour for checking equality, but maybe filtered_eviction_set should use PeerId instead of Peer? And then we can remove the Eq, PartialEq, Hash from here
| pub struct Peer { | ||
| identity: PeerId, | ||
| net_address: SocketAddr, | ||
| is_outbound: bool, | ||
| connection_started: Instant, | ||
| properties: Arc<PeerProperties>, | ||
| last_ping_duration: u64, | ||
| last_block_transfer: Option<Instant>, // TODO: add this to rpc, currently sidelined due to ongoing RPC development |
There was a problem hiding this comment.
The "currently" part is redundant since it can change. Please only leave // TODO: add this to rpc
| @@ -102,6 +102,14 @@ struct RouterMutableState { | |||
|
|
|||
| /// Duration of the last ping to this peer | |||
| last_ping_duration: u64, | |||
|
|
|||
| /// Time of the last block transfer | |||
| /// This corresponds to the last full block transfer, not the last Block Invoice message. | |||
There was a problem hiding this comment.
inv stands for inventory and not invoice. Nevertheless, I think it's better to just say "Block Inv"
| last_block_transfer: Option<Instant>, | ||
|
|
||
| /// Time of the last transaction transfer | ||
| /// This corresponds to the last full transaction(s) transfer(ed), not the last Transaction Invoice message. |
This adds eviction logic and a policy to rusty-kaspa.
The idea is borrowed from btc and straight forward:
rank peers, by
last_ping_duration,time_connected,last_tx_transfer(duration),last_block_transfer(duration),prefix_bucket(dispersal).keep 40% of the best performing peers on any individual metric:
time-connectedandprefix_bucket:On the side, it also logs last tx transfer and last block transfer for each peer, as well as does a small overhaul on the PrefixBucket struct, (mostly inlining and refactoring).