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

feat: track failed/successful dials per-address #2033

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion packages/interface/src/peer-store/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,17 @@ export interface Address {
/**
* Obtained from a signed peer record
*/
isCertified: boolean
isCertified?: true

/**
* A timestamp of the last successful dial of this multiaddr
*/
lastSuccess?: number

/**
* A timestamp of the last unsuccessful dial of this multiaddr
*/
lastFailure?: number
}

/**
Expand Down
58 changes: 43 additions & 15 deletions packages/libp2p/src/connection-manager/dial-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,7 @@ export class DialQueue {
const { peerId, multiaddrs } = getPeerAddress(peerIdOrMultiaddr)

const addrs: Address[] = multiaddrs.map(multiaddr => ({
multiaddr,
isCertified: false
multiaddr
}))

// create abort conditions - need to do this before `calculateMultiaddrs` as we may be about to
Expand Down Expand Up @@ -357,8 +356,7 @@ export class DialQueue {
}

return result.map(multiaddr => ({
multiaddr,
isCertified: false
multiaddr
}))
})
))
Expand Down Expand Up @@ -386,12 +384,6 @@ export class DialQueue {

for (const addr of filteredAddrs) {
const maStr = addr.multiaddr.toString()
const existing = dedupedAddrs.get(maStr)

if (existing != null) {
existing.isCertified = existing.isCertified || addr.isCertified || false
continue
}

dedupedAddrs.set(maStr, addr)
}
Expand Down Expand Up @@ -504,10 +496,25 @@ export class DialQueue {
// update dial status
pendingDial.status = 'active'

const conn = await this.transportManager.dial(addr, {
...options,
signal
})
let conn: Connection

try {
conn = await this.transportManager.dial(addr, {
...options,
signal
})

// mark multiaddr dial as successful
await this._updateAddressStatus(conn.remotePeer, addr, true)
} catch (err: any) {
if (pendingDial.peerId != null) {
// mark multiaddr dial as failure
await this._updateAddressStatus(pendingDial.peerId, addr, false)
}
Comment on lines +510 to +513
Copy link
Member

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?

Copy link
Member Author

@achingbrain achingbrain Sep 12, 2023

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.


// rethrow error
throw err
}

if (controller.signal.aborted) {
// another dial succeeded faster than this one
Expand Down Expand Up @@ -554,7 +561,9 @@ export class DialQueue {
signal.clear()
})

return deferred.promise
const connection = await deferred.promise

return connection
}))

// dial succeeded or failed
Expand All @@ -577,6 +586,25 @@ export class DialQueue {
throw err
}
}

/**
* Record the last dial success/failure status of the passed multiaddr
*/
private async _updateAddressStatus (peerId: PeerId, multiaddr: Multiaddr, success: boolean): Promise<void> {
const addr: Address = {
multiaddr
}

if (success) {
addr.lastSuccess = Date.now()
} else {
addr.lastFailure = Date.now()
}

await this.peerStore.merge(peerId, {
addresses: [addr]
})
}
}

/**
Expand Down
1 change: 0 additions & 1 deletion packages/libp2p/src/identify/identify.ts
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,6 @@ export class DefaultIdentifyService implements Startable, IdentifyService {

if (message.listenAddrs.length > 0) {
peer.addresses = message.listenAddrs.map(buf => ({
isCertified: false,
multiaddr: multiaddr(buf)
}))
}
Expand Down
2 changes: 1 addition & 1 deletion packages/libp2p/test/connection-manager/direct.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ describe('dialing (direct, WebSockets)', () => {
await connectionManager.openConnection(remoteComponents.peerId)

const sortedAddresses = peerMultiaddrs
.map((m) => ({ multiaddr: m, isCertified: false }))
.map((m) => ({ multiaddr: m }))
.sort(defaultAddressSort)

expect(localTMDialStub.getCall(0).args[0].equals(sortedAddresses[0].multiaddr))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,6 @@ describe('content-routing', () => {
await drain(node.contentRouting.findProviders(CID.parse('QmU621oD8AhHw6t25vVyfYKmL9VV3PTgc52FngEhTGACFB')))

await expect(node.peerStore.get(providerPeerId)).to.eventually.have.property('addresses').that.deep.include({
isCertified: false,
multiaddr: result.multiaddrs[0]
})
})
Expand Down Expand Up @@ -376,10 +375,8 @@ describe('content-routing', () => {
await drain(node.contentRouting.findProviders(CID.parse('QmU621oD8AhHw6t25vVyfYKmL9VV3PTgc52FngEhTGACFB')))

await expect(node.peerStore.get(providerPeerId)).to.eventually.have.property('addresses').that.deep.include({
isCertified: false,
multiaddr: result1.multiaddrs[0]
}).and.to.deep.include({
isCertified: false,
multiaddr: result2.multiaddrs[0]
})
})
Expand Down
1 change: 0 additions & 1 deletion packages/libp2p/test/identify/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,6 @@ describe('identify', () => {
expect(peer.metadata.get('ProtocolVersion')).to.equalBytes(uint8ArrayFromString(message.protocolVersion))
expect(peer.protocols).to.deep.equal(message.protocols)
expect(peer.addresses).to.deep.equal([{
isCertified: false,
multiaddr: multiaddr('/ip4/127.0.0.1/tcp/1234')
}])
expect(peer.id.publicKey).to.equalBytes(remoteComponents.peerId.publicKey)
Expand Down
6 changes: 6 additions & 0 deletions packages/peer-store/src/pb/peer.proto
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ message Address {

// Flag to indicate if the address comes from a certified source
optional bool isCertified = 2;

// ms timestamp when we last succesfully dialed this address
optional uint64 lastSuccess = 3;

// ms timestamp when we last failed to dial this address
optional uint64 lastFailure = 4;
}

message Tag {
Expand Down
18 changes: 18 additions & 0 deletions packages/peer-store/src/pb/peer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,8 @@ export namespace Peer {
export interface Address {
multiaddr: Uint8Array
isCertified?: boolean
lastSuccess?: bigint
lastFailure?: bigint
}

export namespace Address {
Expand All @@ -286,6 +288,16 @@ export namespace Address {
w.bool(obj.isCertified)
}

if (obj.lastSuccess != null) {
w.uint32(24)
w.uint64(obj.lastSuccess)
}

if (obj.lastFailure != null) {
w.uint32(32)
w.uint64(obj.lastFailure)
}

if (opts.lengthDelimited !== false) {
w.ldelim()
}
Expand All @@ -306,6 +318,12 @@ export namespace Address {
case 2:
obj.isCertified = reader.bool()
break
case 3:
obj.lastSuccess = reader.uint64()
break
case 4:
obj.lastFailure = reader.uint64()
break
default:
reader.skipType(tag & 7)
break
Expand Down
23 changes: 18 additions & 5 deletions packages/peer-store/src/utils/bytes-to-peer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { peerIdFromPeerId } from '@libp2p/peer-id'
import { multiaddr } from '@multiformats/multiaddr'
import { Peer as PeerPB } from '../pb/peer.js'
import type { PeerId } from '@libp2p/interface/peer-id'
import type { Peer, Tag } from '@libp2p/interface/peer-store'
import type { Address, Peer, Tag } from '@libp2p/interface/peer-store'

export function bytesToPeer (peerId: PeerId, buf: Uint8Array): Peer {
const peer = PeerPB.decode(buf)
Expand Down Expand Up @@ -30,11 +30,24 @@ export function bytesToPeer (peerId: PeerId, buf: Uint8Array): Peer {
return {
...peer,
id: peerId,
addresses: peer.addresses.map(({ multiaddr: ma, isCertified }) => {
return {
multiaddr: multiaddr(ma),
isCertified: isCertified ?? false
addresses: peer.addresses.map(({ multiaddr: ma, isCertified, lastFailure, lastSuccess }) => {
const addr: Address = {
multiaddr: multiaddr(ma)
}

if (isCertified === true) {
addr.isCertified = true
}

if (lastFailure != null) {
addr.lastFailure = Number(lastFailure)
}

if (lastSuccess != null) {
addr.lastSuccess = Number(lastSuccess)
}

return addr
}),
metadata: peer.metadata,
peerRecordEnvelope: peer.peerRecordEnvelope ?? undefined,
Expand Down
51 changes: 38 additions & 13 deletions packages/peer-store/src/utils/dedupe-addresses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,26 +26,51 @@ export async function dedupeFilterAndSortAddresses (peerId: PeerId, filter: Addr
continue
}

const isCertified = addr.isCertified ?? false
const maStr = addr.multiaddr.toString()
const existingAddr = addressMap.get(maStr)
let existingAddr = addressMap.get(maStr)

if (existingAddr != null) {
addr.isCertified = existingAddr.isCertified || isCertified
} else {
addressMap.set(maStr, {
multiaddr: addr.multiaddr,
isCertified
})
if (existingAddr == null) {
existingAddr = {
multiaddr: addr.multiaddr
}

addressMap.set(maStr, existingAddr)
}

if (addr.isCertified === true) {
existingAddr.isCertified = true
}

if (addr.lastFailure != null) {
existingAddr.lastFailure = Number(addr.lastFailure)
}

if (addr.lastSuccess != null) {
existingAddr.lastSuccess = Number(addr.lastSuccess)
Comment on lines +45 to +49
Copy link
Member

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?

Copy link
Member Author

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.

}
}

return [...addressMap.values()]
.sort((a, b) => {
return a.multiaddr.toString().localeCompare(b.multiaddr.toString())
})
.map(({ isCertified, multiaddr }) => ({
isCertified,
multiaddr: multiaddr.bytes
}))
.map(({ isCertified, multiaddr, lastFailure, lastSuccess }) => {
const addr: AddressPB = {
multiaddr: multiaddr.bytes
}

if (isCertified) {
addr.isCertified = true
}

if (lastFailure != null) {
addr.lastFailure = BigInt(lastFailure)
}

if (lastSuccess != null) {
addr.lastSuccess = BigInt(lastSuccess)
}

return addr
})
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export function toDatastorePeer (peerId: PeerId, data: PeerData): PeerPB {

const output: PeerPB = {
addresses: (data.addresses ?? [])
.concat((data.multiaddrs ?? []).map(multiaddr => ({ multiaddr, isCertified: false })))
.concat((data.multiaddrs ?? []).map(multiaddr => ({ multiaddr })))
.filter(address => {
if (!isMultiaddr(address.multiaddr)) {
throw new CodeError('Invalid mulitaddr', codes.ERR_INVALID_PARAMETERS)
Expand All @@ -36,9 +36,11 @@ export function toDatastorePeer (peerId: PeerId, data: PeerData): PeerPB {
.sort((a, b) => {
return a.multiaddr.toString().localeCompare(b.multiaddr.toString())
})
.map(({ multiaddr, isCertified }) => ({
.map(({ multiaddr, isCertified, lastFailure, lastSuccess }) => ({
multiaddr: multiaddr.bytes,
isCertified
isCertified,
lastFailure: lastFailure != null ? BigInt(lastFailure) : undefined,
lastSuccess: lastSuccess != null ? BigInt(lastSuccess) : undefined
})),
protocols: (data.protocols ?? []).sort(),
metadata: new Map(),
Expand Down
2 changes: 0 additions & 2 deletions packages/peer-store/src/utils/to-peer-pb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export async function toPeerPB (peerId: PeerId, data: Partial<PeerData>, strateg

if (data.multiaddrs != null) {
addresses.push(...data.multiaddrs.map(multiaddr => ({
isCertified: false,
multiaddr
})))
}
Expand Down Expand Up @@ -80,7 +79,6 @@ export async function toPeerPB (peerId: PeerId, data: Partial<PeerData>, strateg
if (strategy === 'merge') {
if (data.multiaddrs != null) {
addresses.push(...data.multiaddrs.map(multiaddr => ({
isCertified: false,
multiaddr
})))
}
Expand Down
Loading
Loading