litep2p/discovery: Publish authority records with external addresses only #5176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
records Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
list Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
lexnv
added
A1-insubstantial
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
dmitry-markin
approved these changes
Jul 29, 2024
bkchr
approved these changes
Jul 29, 2024
| @@ -278,7 +284,7 @@ impl Discovery { | |||
| find_node_query_id: None, | |||
| pending_events: VecDeque::new(), | |||
| duration_to_next_find_query: Duration::from_secs(1), | |||
| address_confirmations: LruMap::new(ByLength::new(8)), | |||
| address_confirmations: LruMap::new(ByLength::new(MAX_EXTERNAL_ADDRESSES)), | |||
There was a problem hiding this comment.
How do we ensure that nodes do not report junk addresses to us?
There was a problem hiding this comment.
Generally I don't get why we not bootstrap it with the addresses of our interfaces? If these addresses are global, the likelihood that the node is reachable via them should be quite big?
There was a problem hiding this comment.
Yep that could also work, we can check our interfaces and iff we find a global, we'll propagate that. I think this wasn't considered before because it was assumed that most nodes are under nat / firewall.
Another approach for this would be to use a distributed address lookup service (instead of curl centralized.service.ip).
How do we ensure that nodes do not report junk addresses to us?
I don't think anything is protecting either libp2p or litep2p here. Litep2p ran into this error because it was waiting for 5 confirmations, instead of eagerly accepting all reported addresses 🤔
Co-authored-by: Bastian Köcher <git@kchr.de>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
… into lexnv/unknown-authority-records
|
The CI pipeline was cancelled due to failure one of the required jobs. |
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
dmitry-markin
approved these changes
Jul 31, 2024
AndreiEres
approved these changes
Jul 31, 2024
TarekkMA
pushed a commit
to moonbeam-foundation/polkadot-sdk
that referenced
this pull request
Aug 2, 2024
…only (paritytech#5176) This PR reduces the occurrences for identified observed addresses. Litep2p discovers its external addresses by inspecting the `IdentifyInfo::ObservedAddress` field reported by other peers. After we get 5 confirmations of the same external observed address (the address the peer dialed to reach us), the address is reported through the network layer. The PR effectively changes this from 5 to 2. This has a subtle implication on freshly started nodes for the authority-discovery discussed below. The PR also makes the authority discovery a bit more robust by not publishing records if the node doesn't have addresses yet to report. This aims to fix a scenario where: - the litep2p node has started, it has some pending observed addresses but less than 5 - the authorit-discovery publishes a record, but at this time the node doesn't have any addresses discovered and the record is published without addresses -> this means other nodes will not be able to reach us Next Steps - [ ] versi testing Closes: paritytech#5147 cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Bastian Köcher <git@kchr.de>
ordian
added a commit
that referenced
this pull request
Aug 6, 2024
* master: (51 commits) Remove unused feature gated code from the minimal template (#5237) make polkadot-parachain startup errors pretty (#5214) Coretime auto-renew (#4424) network/strategy: Backoff and ban overloaded peers to avoid submitting the same request multiple times (#5029) Fix frame crate usage doc (#5222) beefy: Tolerate pruned state on runtime API call (#5197) rpc: Enable ChainSpec for polkadot-parachain (#5205) Add an adapter for configuring AssetExchanger (#5130) Replace env_logger with sp_tracing (#5065) Adjust sync templates flow to use new release branch (#5182) litep2p/discovery: Publish authority records with external addresses only (#5176) Run UI tests in CI for some other crates (#5167) Remove `pallet::getter` usage from the pallet-balances (#4967) pallet-timestamp: `UnixTime::now` implementation logs error only if called at genesis (#5055) [CI] Cache try-runtime check (#5179) [Backport] version bumps and the prdocs reordering from stable2407 (#5178) [subsystem-benchmark] Update availability-distribution-regression-bench baseline after recent subsystem changes (#5180) Remove pallet::getter usage from proxy (#4963) Remove pallet::getter macro usage from pallet-election-provider-multi-phase (#4487) Review-bot@2.6.0 (#5177) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR reduces the occurrences for identified observed addresses.
Litep2p discovers its external addresses by inspecting the
IdentifyInfo::ObservedAddressfield reported by other peers.After we get 5 confirmations of the same external observed address (the address the peer dialed to reach us), the address is reported through the network layer.
The PR effectively changes this from 5 to 2.
This has a subtle implication on freshly started nodes for the authority-discovery discussed below.
The PR also makes the authority discovery a bit more robust by not publishing records if the node doesn't have addresses yet to report.
This aims to fix a scenario where:
Next Steps
Closes: #5147
cc @paritytech/networking