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

discovery: append quic multiaddrs along with tcp, #6

Merged
merged 3 commits into from
Aug 8, 2023

Conversation

jxs
Copy link
Collaborator

@jxs jxs commented Aug 4, 2023

on handle_pending_outbound_connection.

on handle_pending_outbound_connection
jxs added 2 commits August 7, 2023 19:00
on GenerateEvent, and therefore remove unnecessary Enr fetching on Network.
Copy link
Collaborator Author

@jxs jxs left a comment

Choose a reason for hiding this comment

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

following our conversation off band and wrt isolating the multiaddresses gathering on the PeerManager I was able t return only Enr's from Discovery and therefore avoid some iterations and the enr_of_peer calls (as we have the ENR from Discovery) but something tells me that we need those right?

beacon_node/lighthouse_network/src/discovery/mod.rs Outdated Show resolved Hide resolved
Copy link
Owner

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

Nice! Looks like you've got the hang of everything.

You can probably see the dialing logic is scattered around. In a future PR, maybe we can group some of this into the peer manager.

@@ -317,11 +317,11 @@ impl<TSpec: EthSpec> PeerManager<TSpec> {
/// multiaddr here, however this could relate to duplicate PeerId's etc. If the lookup
/// proves resource constraining, we should switch to multiaddr dialling here.
#[allow(clippy::mutable_key_type)]
pub fn peers_discovered(&mut self, results: HashMap<PeerId, Option<Instant>>) -> Vec<PeerId> {
pub fn peers_discovered(&mut self, results: HashMap<Enr, Option<Instant>>) -> Vec<Enr> {
Copy link
Owner

Choose a reason for hiding this comment

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

Nice. I'll also update the doc comments to match

}

// Remove the ENR from the cache to prevent continual re-dialing on disconnects
// TODO: add this step to the iteration above.
Copy link
Owner

Choose a reason for hiding this comment

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

I'll add this guy also

Copy link
Owner

Choose a reason for hiding this comment

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

Is a bit trickier because the iterator is on a reference of cached peers.

The second iteration isn't that bad. Feel free to update if you find a nice way to group them and avoid the ENR clone

let DiscoveredPeers { peers } = event;
let to_dial_peers = self.peer_manager_mut().peers_discovered(peers);
for peer_id in to_dial_peers {
debug!(self.log, "Dialing discovered peer"; "peer_id" => %peer_id);
Copy link
Owner

Choose a reason for hiding this comment

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

Some of the logs we emit are quite important. Trace logs not so much, but debug logs are. This one in particular is very useful to track errors throughout the stack as we note when we dial and when connections fail etc.

I'll add this log into the peer manager poll when we submit the actual dial.

@AgeManning
Copy link
Owner

I have forgotten why we didn't store the ENRs originally.

My initial feeling was that ENRs can be quite large and they are stored in discovery, so perhaps we wanted to avoid cloning them and only get them if we were actually going to use them.

However, your changes here look much nicer. ENRs are not massive anyway, at most 1280 bytes IIRC.

@AgeManning AgeManning merged commit f98f143 into AgeManning:quic Aug 8, 2023
27 of 28 checks passed
AgeManning pushed a commit that referenced this pull request Oct 16, 2023
* introduce availability pending block

* add intoavailableblock trait

* small fixes

* add 'gossip blob cache' and start to clean up processing and transition types

* shard memory blob cache

* Initial commit

* Fix after rebase

* Add gossip verification conditions

* cache cleanup

* general chaos

* extended chaos

* cargo fmt

* more progress

* more progress

* tons of changes, just tryna compile

* everything, everywhere, all at once

* Reprocess an ExecutedBlock on unavailable blobs

* Add sus gossip verification for blobs

* Merge stuff

* Remove reprocessing cache stuff

* lint

* Add a wrapper to allow construction of only valid `AvailableBlock`s

* rename blob arc list to blob list

* merge cleanuo

* Revert "merge cleanuo"

This reverts commit 5e98326.

* Revert "Revert "merge cleanuo""

This reverts commit 3a40094.

* fix rpc methods

* move beacon block and blob to eth2/types

* rename gossip blob cache to data availability checker

* lots of changes

* fix some compilation issues

* fix compilation issues

* fix compilation issues

* fix compilation issues

* fix compilation issues

* fix compilation issues

* cargo fmt

* use a common data structure for block import types

* fix availability check on proposal import

* refactor the blob cache and split the block wrapper into two types

* add type conversion for signed block and block wrapper

* fix beacon chain tests and do some renaming, add some comments

* Partial processing (#4)

* move beacon block and blob to eth2/types

* rename gossip blob cache to data availability checker

* lots of changes

* fix some compilation issues

* fix compilation issues

* fix compilation issues

* fix compilation issues

* fix compilation issues

* fix compilation issues

* cargo fmt

* use a common data structure for block import types

* fix availability check on proposal import

* refactor the blob cache and split the block wrapper into two types

* add type conversion for signed block and block wrapper

* fix beacon chain tests and do some renaming, add some comments

* cargo update (#6)

---------

Co-authored-by: realbigsean <sean@sigmaprime.io>
Co-authored-by: realbigsean <seananderson33@gmail.com>
AgeManning pushed a commit that referenced this pull request Jan 15, 2024
* Update DB migration docs

* Document VC broadcast modes

* Update downgrade example (#6)

* update downgrade example

* Add period

* Add v4.1.0

---------

Co-authored-by: chonghe <44791194+chong-he@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants