Dynamic predicting of external IP and UDP port #5602
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5602 +/- ##
=========================================
+ Coverage 62.24% 62.34% +0.1%
=========================================
Files 347 350 +3
Lines 29299 29391 +92
Branches 3312 3317 +5
=========================================
+ Hits 18237 18324 +87
- Misses 9873 9875 +2
- Partials 1189 1192 +3 |
| // Returns number of currently kept statements in favor of _externalEndpoint | ||
| size_t addEndpointStatement( | ||
| bi::udp::endpoint const& _sourceEndpoint, bi::udp::endpoint const& _externalEndpoint); | ||
|
|
There was a problem hiding this comment.
(nit) typo (statemens)
libp2p/EndpointTracker.cpp
Outdated
| namespace p2p | ||
| { | ||
|
|
||
| // Register the statement about endpoint from one othe peers. |
There was a problem hiding this comment.
You should use /// comments for documentation.
There was a problem hiding this comment.
@chfast : I see a mix of both in our code base...should we always use ///? Presumably git clang-format won't set the right comment formatting for us?
There was a problem hiding this comment.
The /// or /** are doxygen comments so we could generate documentation out of it. Which we will probably never do so this is not critical. This should not matter for clang-format at all.
libp2p/EndpointTracker.cpp
Outdated
| { | ||
| size_t maxCount = 0; | ||
| bi::udp::endpoint bestEndpoint; | ||
| for (auto const& endpointAndCount : m_endpointStatementCountMap) |
| private: | ||
| using EndpointAndTimePoint = | ||
| std::pair<bi::udp::endpoint, std::chrono::steady_clock::time_point>; | ||
| using SourceToStatementMap = std::map<bi::udp::endpoint, EndpointAndTimePoint>; |
There was a problem hiding this comment.
Boost doesn't have a hashing function defined for udp::endpoint (but has operator<), so I used map to avoid defining it myself
There was a problem hiding this comment.
Can you check in the latest boost version? If not there we should report a bug.
There was a problem hiding this comment.
Not sure if it's a bug, maybe not every type should define hashing function?
There was a problem hiding this comment.
Looks like good candidate for being a key in a hash map.
There was a problem hiding this comment.
Reported boostorg/asio#245
There was a problem hiding this comment.
Resurrecting this from the dead, just to say that I'm surprised that boostorg/asio#245 got no responses so far. We have been carrying custom hashing support for these types in rippled so it's been "out of sight, out of mind" for that codebase, but this really is something that Boost should provide.
I'm happy to have Ripple sponsor a bounty to get support coded up, or I can have someone from my team do it if nobody else is interested.
| // Statements about our external endpoint, maps statement source peer => endpoint, timestamp | ||
| SourceToStatementMap m_statementsMap; | ||
| // map external endpoint => how many sources reported it | ||
| std::map<bi::udp::endpoint, size_t> m_endpointStatementCountMap; |
| private: | ||
| using EndpointAndTimePoint = | ||
| std::pair<bi::udp::endpoint, std::chrono::steady_clock::time_point>; | ||
| using SourceToStatementMap = std::map<bi::udp::endpoint, EndpointAndTimePoint>; |
There was a problem hiding this comment.
Can you check in the latest boost version? If not there we should report a bug.
This improves how we track what our peers see as our external IP address / UDP port.
Previously we updated external port every time we receive new port number in Pong packet, now we count which port is reported by most of the peers and update it when new value is reported from more than 10 peers.
Also external IP address is similarly updated from Pong packet, but only unless it was given as a command line argument (or detected by UPNP).
Here's go-ethereum code doing quite the same:
https://github.com/ethereum/go-ethereum/blob/HEAD@%7B2019-05-15T10:20:04Z%7D/p2p/netutil/iptrack.go
https://github.com/ethereum/go-ethereum/blob/97d3615612a49488d02807944696d001b88f9e0d/p2p/enode/localnode.go#L180-L202