-
Notifications
You must be signed in to change notification settings - Fork 808
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
[net/libp2p] Use raw Identify
observed addresses to discover external addresses
#7338
Conversation
/cmd prdoc --audience node_dev --bump patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic looks good to me!
const MIN_ADDRESS_CONFIRMATIONS: usize = 2; | ||
/// Number of times observed address is received from different peers before it is confirmed as | ||
/// external. | ||
const MIN_ADDRESS_CONFIRMATIONS: usize = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are now waiting for 3 confirmations in an LRU of 32 addresses. Are we losing precision somehow with this setup? Should we bump MAX_EXTERNAL_ADDRESSES
to a higher value, or are we able to discover addresses just like before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a bug, and when we received the address for the first time we inserted an empty hash map, and only after that counted confirmations. So it was always 3 confirmations, just the information about the first peer ID we got the address from was lost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice job here 🙏
let oldest = (self.address_confirmations.len() >= | ||
self.address_confirmations.limiter().max_length() as usize) | ||
.then(|| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code looks slightly horrible :D (Some upstream change that also returns the removed item on insert would be nice)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a side note, LruMap
will need to return multiple removed items to handle ByMemoryUsage
limiter use case.
* master: Remove warnings by cleaning up the `Cargo.toml` (#7416) [Backport] Version bumps from stable2412-1 + prdocs reorg (#7401) fix pre-dispatch PoV underweight for ParasInherent (#7378) malus-collator: implement malicious collator submitting same collation to all backing groups (#6924) `fatxpool`: use tracing for logging (#6897) Improvements for Weekly bench (#7390) Replace derivative dependency with derive-where (#7324) Add support for feature `pallet_balances/insecure_zero_ed` in benchmarks and testing (#7379) Fix Snowbridge benchmark tests (#7296) Bridges small nits/improvements (#7383) Migrating cumulus-pallet-session-benchmarking to Benchmarking V2 (#6564) [pallet-revive] implement the block author API (#7198) Use checked math in frame-balances named_reserve (#7365) move installation of frame-omni-bencher into a cmd.py itself (#7372) remove old bench & revert the frame-weight-template (#7362) ci: fix workflow permissions (#7366) [net/libp2p] Use raw `Identify` observed addresses to discover external addresses (#7338) Improve `set_validation_data` error message. (#7359) Implement pallet view function queries (#4722)
Instead of using libp2p-provided external address candidates, susceptible to address translation issues, use litep2p-backend approach based on confirming addresses observed by multiple peers as external.
Fixes #7207.