feat: add support for listen addresses and filtering#136
Conversation
- Pending `GitCommit` implementation
- Move parse method to `impl` block
- Include Git commit - Include timestamp - Include env version if any
varex83
changed the title
|
question: Do you thing this is required for the MVP? If not, let's leave for now and focus on the current features, in particular those used by the example executables. |
No, as I said this PR is optional for MVP. But it's important for finishing p2p part |
varex83
force-pushed
the
bohdan/finish-p2p
branch
from
5065857 to
67faa24
Compare
| let filtered_addrs = | ||
| utils::filter_advertised_addresses(addrs, external_addrs, filter_private_addrs)?; | ||
|
|
||
| for addr in filtered_addrs { |
There was a problem hiding this comment.
Should it be for addr in addrs { ?
https://github.com/ObolNetwork/charon/blob/v1.7.1/p2p/p2p.go#L110
| external_addrs.dedup(); | ||
| internal_addrs.dedup(); |
There was a problem hiding this comment.
The requirement for dedup is sorted array
| } | ||
|
|
||
| let filtered_addrs = | ||
| utils::filter_advertised_addresses(addrs, external_addrs, filter_private_addrs)?; |
There was a problem hiding this comment.
the order seem wrong ?
The function is
pub(crate) fn filter_advertised_addresses(
mut external_addrs: Vec<Multiaddr>,
mut internal_addrs: Vec<Multiaddr>,
exclude_interval_private: bool,
) -> crate::p2p::Result<Vec<Multiaddr>> {
| pub(crate) fn filter_advertised_addresses( | ||
| mut external_addrs: Vec<Multiaddr>, | ||
| mut internal_addrs: Vec<Multiaddr>, | ||
| exclude_interval_private: bool, |
There was a problem hiding this comment.
exclude_internal_private
emlautarom1
left a comment
emlautarom1
left a comment
There was a problem hiding this comment.
A few small things before merging
- I would look to reduce some duplication when constructing nodes with TPC/QUIC and Relay.
- We could use some tests for the standalone functions in
crates/p2p/src/utils.rs - Other small comments
| pub(crate) struct ExternalAddresses(pub Vec<Multiaddr>); | ||
|
|
||
| pub(crate) struct InternalAddresses(pub Vec<Multiaddr>); |
There was a problem hiding this comment.
I don't see the point on having these wrappers: we don't perform any checks so we could still mix them uo
There was a problem hiding this comment.
Since we have the same types in the function arguments, it's easy to mix them. In the meantime having those wrappers adds additional compiler check for the type.
|
|
||
| /// Returns the default swarm configuration. | ||
| pub(crate) fn default_swarm_config(cfg: libp2p::swarm::Config) -> libp2p::swarm::Config { | ||
| cfg.with_idle_connection_timeout(Duration::from_secs(300)) |
There was a problem hiding this comment.
From where are we getting these 5 minutes?
There was a problem hiding this comment.
It was pretty old issue, the libp2p was dropping connections (though they were active) and by setting the idle_connection_timeout to 5 minutes solved this issue. I don't know whether we can use smaller amount, but I think since we are in a private network of validators it should be okay.
There was a problem hiding this comment.
First time hearing about it. Do you have any links/references to this issue? Is it from libp2p directly or our own code?
There was a problem hiding this comment.
I can't really find the discussion I was referring to, but you can take a look on this
3 participants
Closes #137
Closes #138
This PR implements missing functionality in
charon-p2p/p2p:multiaddr/netfrom go (since Rust doesn't support this). It creates traitManetwhich is implemented forlibp2p::multiaddr::Multiaddr, so it works like native function after importing. Original implementation could be found here also tests