feat: track failed/successful dials per-address #2033
Conversation
Instead of tracking which peer we failed to dial, track dial success or failure on a per-address basis. This will let us detect when IPv6 is unsupported or certain transports (e.g. UDP ones) are blocked. Refs: 2010
| existingAddr.lastFailure = Number(addr.lastFailure) | ||
| } | ||
|
|
||
| if (addr.lastSuccess != null) { | ||
| existingAddr.lastSuccess = Number(addr.lastSuccess) |
There was a problem hiding this comment.
Is there a good reason to use Number here instead of BigInt as is used elsewhere in this PR?
There was a problem hiding this comment.
The value is a timestamp, the max value of which can exceed 32 bits so we need to use a 64 bit number in the protobuf to store it, which means we need to represent it as a BigInt, but this is overkill for actual time values.
We can refactor this once ipfs/protons#112 is implemented to have protons serialize/deserialize to Number instead of BigInt.
| @@ -244,4 +241,53 @@ describe('merge', () => { | |||
| expect(updated).to.have.property('tags').that.deep.equals(original.tags) | |||
| expect(updated).to.have.property('protocols').that.deep.equals(original.protocols) | |||
| }) | |||
|
|
|||
| it('merges addresses', async () => { | |||
There was a problem hiding this comment.
| it('merges addresses', async () => { | |
| it('merges lastFailure & lastSuccess on peer addresses', async () => { |
There was a problem hiding this comment.
This test also ensures multiaddr and isCertified values are merged and not lost so they are missing from the test name if you want it to be exhaustive, or it can remain as it is.
| if (pendingDial.peerId != null) { | ||
| // mark multiaddr dial as failure | ||
| await this._updateAddressStatus(pendingDial.peerId, addr, false) | ||
| } |
There was a problem hiding this comment.
This check implies that we may also attempt to dial peers where peerId == null. Can we log when that is the case, and that we're not marking peer as having a failed dial?
There was a problem hiding this comment.
we may also attempt to dial peers where peerId == null
Yes, this is the case when dialing a multiaddr without a peer id, e.g. /dnsaddr/bootstrap.libp2p.io
and that we're not marking peer as having a failed dial?
We can't mark the peer as having a failed dial as peer data is keyed on the peer id - if we don't know the peer id we can't mark anything as having failed.
There was a problem hiding this comment.
Following from previous conversations I think this is the right direction, is this ready for review @achingbrain ?
|
LGTM but we should perhaps merge #2150 into this before merging this as it effectively adds no enhancement without the sorting functionality. |
…lures-per-address
achingbrain
force-pushed
the
main
branch
12 times, most recently
from
b32c738
to
3fa4bb2
Compare
achingbrain
force-pushed
the
main
branch
25 times, most recently
from
242fd96
to
bca8d6e
Compare
achingbrain
force-pushed
the
main
branch
2 times, most recently
from
6453a80
to
c2bc7fe
Compare
Instead of tracking which peer we failed to dial, track dial success or failure on a per-address basis.
This will let us detect when IPv6 is unsupported or certain transports (e.g. UDP ones) are blocked.
Refs: #2010