-
Notifications
You must be signed in to change notification settings - Fork 450
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
feat: track failed/successful dials per-address #2033
base: main
Are you sure you want to change the base?
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just looking through the code.. some minor improvements and a question but in general it looks reasonable.
There doesn't seem to be any code utilizing the lastSuccess or lastFailed yet, but from the title of the PR that seems intentional.
existingAddr.lastFailure = Number(addr.lastFailure) | ||
} | ||
|
||
if (addr.lastSuccess != null) { | ||
existingAddr.lastSuccess = Number(addr.lastSuccess) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it('merges addresses', async () => { | |
it('merges lastFailure & lastSuccess on peer addresses', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
b32c738
to
3fa4bb2
Compare
242fd96
to
bca8d6e
Compare
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