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

fix: respect the IPNS TTL field #482

Merged
merged 2 commits into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion packages/ipns/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@
"@libp2p/peer-id": "^4.0.7",
"@multiformats/dns": "^1.0.1",
"interface-datastore": "^8.2.11",
"ipns": "^9.0.0",
"ipns": "^9.1.0",
"multiformats": "^13.1.0",
"progress-events": "^1.0.0",
"uint8arrays": "^5.0.2"
Expand Down
45 changes: 38 additions & 7 deletions packages/ipns/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,8 @@
const DEFAULT_LIFETIME_MS = 24 * HOUR
const DEFAULT_REPUBLISH_INTERVAL_MS = 23 * HOUR

const DEFAULT_TTL_NS = BigInt(HOUR) * 1_000_000n

export type PublishProgressEvents =
ProgressEvent<'ipns:publish:start'> |
ProgressEvent<'ipns:publish:success', IPNSRecord> |
Expand Down Expand Up @@ -425,8 +427,8 @@

if (await this.localStore.has(routingKey, options)) {
// if we have published under this key before, increment the sequence number
const buf = await this.localStore.get(routingKey, options)
const existingRecord = unmarshal(buf)
const { record } = await this.localStore.get(routingKey, options)
const existingRecord = unmarshal(record)

Check warning on line 431 in packages/ipns/src/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/ipns/src/index.ts#L430-L431

Added lines #L430 - L431 were not covered by tests
sequenceNumber = existingRecord.sequence + 1n
}

Expand Down Expand Up @@ -539,17 +541,46 @@
log('record is present in the cache')

if (options.nocache !== true) {
// check the local cache first
const record = await this.localStore.get(routingKey, options)

try {
// check the local cache first
const { record, created } = await this.localStore.get(routingKey, options)

this.log('record retrieved from cache')

// validate the record
await ipnsValidator(routingKey, record)

this.log('record successfully retrieved from cache')
this.log('record was valid')

// check the TTL
const ipnsRecord = unmarshal(record)

return unmarshal(record)
// IPNS TTL is in nanoseconds, convert to milliseconds, default to one
// hour
const ttlMs = Number((ipnsRecord.ttl ?? DEFAULT_TTL_NS) / 1_000_000n)
const ttlExpires = created.getTime() + ttlMs

if (ttlExpires > Date.now()) {
// the TTL has not yet expired, return the cached record
this.log('record TTL was valid')
return ipnsRecord
}

if (options.offline === true) {
// the TTL has expired but we are skipping the routing search
this.log('record TTL has been reached but we are resolving offline-only, returning record')
return ipnsRecord

Check warning on line 572 in packages/ipns/src/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/ipns/src/index.ts#L570-L572

Added lines #L570 - L572 were not covered by tests
}

this.log('record TTL has been reached, searching routing for updates')

// add the local record to our list of resolved record, and also
// search the routing for updates - the most up to date record will be
// returned
records.push(record)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we are calling ipnsValidator twice for these records. it would be nice to consolidate the record processing logic

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, there's no point in re-validating the cached record, but it takes several orders of magnitude longer to look up a record than it does to validate it so any performance gain would be unnoticeable.

} catch (err) {
this.log('cached record was invalid', err)
await this.localStore.delete(routingKey, options)
}
} else {
log('ignoring local cache due to nocache=true option')
Expand Down
39 changes: 35 additions & 4 deletions packages/ipns/src/routing/local-store.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { Record } from '@libp2p/kad-dht'
import { type Datastore, Key } from 'interface-datastore'
import { CustomProgressEvent, type ProgressEvent } from 'progress-events'
import { equals as uint8ArrayEquals } from 'uint8arrays/equals'
import { toString as uint8ArrayToString } from 'uint8arrays/to-string'
import type { GetOptions, IPNSRouting, PutOptions } from '../routing'
import type { GetOptions, PutOptions } from '../routing'
import type { AbortOptions } from '@libp2p/interface'

function dhtRoutingKey (key: Uint8Array): Key {
Expand All @@ -14,8 +15,16 @@
ProgressEvent<'ipns:routing:datastore:get'> |
ProgressEvent<'ipns:routing:datastore:error', Error>

export interface LocalStore extends IPNSRouting {
export interface GetResult {
record: Uint8Array
created: Date
}

export interface LocalStore {
put(routingKey: Uint8Array, marshaledRecord: Uint8Array, options?: PutOptions): Promise<void>
get(routingKey: Uint8Array, options?: GetOptions): Promise<GetResult>
has(routingKey: Uint8Array, options?: AbortOptions): Promise<boolean>
delete(routingKey: Uint8Array, options?: AbortOptions): Promise<void>
}

/**
Expand All @@ -29,6 +38,21 @@
try {
const key = dhtRoutingKey(routingKey)

// don't overwrite existing, identical records as this will affect the
// TTL
try {
const existingBuf = await datastore.get(key)
const existingRecord = Record.deserialize(existingBuf)

if (uint8ArrayEquals(existingRecord.value, marshalledRecord)) {
return
}
} catch (err: any) {
if (err.code !== 'ERR_NOT_FOUND') {
throw err
}

Check warning on line 53 in packages/ipns/src/routing/local-store.ts

View check run for this annotation

Codecov / codecov/patch

packages/ipns/src/routing/local-store.ts#L52-L53

Added lines #L52 - L53 were not covered by tests
}

// Marshal to libp2p record as the DHT does
const record = new Record(routingKey, marshalledRecord, new Date())

Expand All @@ -39,7 +63,7 @@
throw err
}
},
async get (routingKey: Uint8Array, options: GetOptions = {}): Promise<Uint8Array> {
async get (routingKey: Uint8Array, options: GetOptions = {}): Promise<GetResult> {
try {
const key = dhtRoutingKey(routingKey)

Expand All @@ -49,7 +73,10 @@
// Unmarshal libp2p record as the DHT does
const record = Record.deserialize(buf)

return record.value
return {
record: record.value,
created: record.timeReceived
}
} catch (err: any) {
options.onProgress?.(new CustomProgressEvent<Error>('ipns:routing:datastore:error', err))
throw err
Expand All @@ -58,6 +85,10 @@
async has (routingKey: Uint8Array, options: AbortOptions = {}): Promise<boolean> {
const key = dhtRoutingKey(routingKey)
return datastore.has(key, options)
},
async delete (routingKey, options): Promise<void> {
const key = dhtRoutingKey(routingKey)
return datastore.delete(key, options)
}
}
}
6 changes: 4 additions & 2 deletions packages/ipns/src/routing/pubsub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class PubSubRouting implements IPNSRouting {
await ipnsValidator(routingKey, message.data)

if (await this.localStore.has(routingKey)) {
const currentRecord = await this.localStore.get(routingKey)
const { record: currentRecord } = await this.localStore.get(routingKey)

if (uint8ArrayEquals(currentRecord, message.data)) {
log('not storing record as we already have it')
Expand Down Expand Up @@ -128,7 +128,9 @@ class PubSubRouting implements IPNSRouting {
}

// chain through to local store
return await this.localStore.get(routingKey, options)
const { record } = await this.localStore.get(routingKey, options)

return record
} catch (err: any) {
options.onProgress?.(new CustomProgressEvent<Error>('ipns:pubsub:error', err))
throw err
Expand Down
4 changes: 2 additions & 2 deletions packages/ipns/test/publish.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe('publish', () => {
const ipnsEntry = await name.publish(key, cid)

expect(ipnsEntry).to.have.property('sequence', 1n)
expect(ipnsEntry).to.have.property('ttl', 8640000000000n) // 24 hours
expect(ipnsEntry).to.have.property('ttl', 3600000000000n) // 1 hour
achingbrain marked this conversation as resolved.
Show resolved Hide resolved
})

it('should publish an IPNS record with a custom ttl params', async function () {
Expand All @@ -55,7 +55,7 @@ describe('publish', () => {
})

expect(ipnsEntry).to.have.property('sequence', 1n)
expect(ipnsEntry).to.have.property('ttl', BigInt(lifetime) * 100000n)
expect(ipnsEntry).to.have.property('ttl', 3600000000000n)
achingbrain marked this conversation as resolved.
Show resolved Hide resolved

expect(heliaRouting.put.called).to.be.true()
expect(customRouting.put.called).to.be.true()
Expand Down
102 changes: 100 additions & 2 deletions packages/ipns/test/resolve.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { createEd25519PeerId } from '@libp2p/peer-id-factory'
import { expect } from 'aegir/chai'
import { MemoryDatastore } from 'datastore-core'
import { type Datastore, Key } from 'interface-datastore'
import { create, marshal, peerIdToRoutingKey, unmarshal } from 'ipns'
import { create, createWithExpiration, marshal, peerIdToRoutingKey, unmarshal } from 'ipns'
import drain from 'it-drain'
import { CID } from 'multiformats/cid'
import Sinon from 'sinon'
Expand Down Expand Up @@ -78,6 +78,7 @@ describe('resolve', () => {
})

it('should skip the local cache when resolving a record', async () => {
const cachePutSpy = Sinon.spy(datastore, 'put')
const cacheGetSpy = Sinon.spy(datastore, 'get')

const key = await createEd25519PeerId()
Expand All @@ -92,7 +93,9 @@ describe('resolve', () => {

expect(heliaRouting.get.called).to.be.true()
expect(customRouting.get.called).to.be.true()
expect(cacheGetSpy.called).to.be.false()

// we call `.get` during `.put`
cachePutSpy.calledBefore(cacheGetSpy)
})

it('should retrieve from local cache when resolving a record', async () => {
Expand Down Expand Up @@ -198,4 +201,99 @@ describe('resolve', () => {

expect(result).to.have.deep.property('record', record)
})

it('should not search the routing for updated IPNS records when a locally cached copy is within the TTL', async () => {
const key = await createEd25519PeerId()
const customRoutingKey = peerIdToRoutingKey(key)
const dhtKey = new Key('/dht/record/' + uint8ArrayToString(customRoutingKey, 'base32'), false)

// create a record with a valid lifetime and a non-expired TTL
const ipnsRecord = await create(key, cid, 1, Math.pow(2, 10), {
ttlNs: 10_000_000
})
const dhtRecord = new Record(customRoutingKey, marshal(ipnsRecord), new Date(Date.now()))

await datastore.put(dhtKey, dhtRecord.serialize())

const result = await name.resolve(key)
expect(result).to.have.deep.property('record', unmarshal(marshal(ipnsRecord)))

// should not have searched the routing
expect(customRouting.get.called).to.be.false()
})

it('should search the routing for updated IPNS records when a locally cached copy has passed the TTL', async () => {
const key = await createEd25519PeerId()

const customRoutingKey = peerIdToRoutingKey(key)
const dhtKey = new Key('/dht/record/' + uint8ArrayToString(customRoutingKey, 'base32'), false)

// create a record with a valid lifetime but an expired ttl
const ipnsRecord = await create(key, cid, 1, Math.pow(2, 10), {
ttlNs: 10
})
const dhtRecord = new Record(customRoutingKey, marshal(ipnsRecord), new Date(Date.now() - 1000))

await datastore.put(dhtKey, dhtRecord.serialize())

const result = await name.resolve(key)
expect(result).to.have.deep.property('record', unmarshal(marshal(ipnsRecord)))

// should have searched the routing
expect(customRouting.get.called).to.be.true()
})

it('should search the routing for updated IPNS records when a locally cached copy has passed the TTL and choose the record with a higher sequence number', async () => {
const key = await createEd25519PeerId()

const customRoutingKey = peerIdToRoutingKey(key)
const dhtKey = new Key('/dht/record/' + uint8ArrayToString(customRoutingKey, 'base32'), false)

// create a record with a valid lifetime but an expired ttl
const ipnsRecord = await create(key, cid, 10, Math.pow(2, 10), {
ttlNs: 10
})
const dhtRecord = new Record(customRoutingKey, marshal(ipnsRecord), new Date(Date.now() - 1000))

await datastore.put(dhtKey, dhtRecord.serialize())

// the routing returns a valid record with an higher sequence number
const ipnsRecordFromRouting = await create(key, cid, 11, Math.pow(2, 10), {
ttlNs: 10_000_000
})
customRouting.get.withArgs(customRoutingKey).resolves(marshal(ipnsRecordFromRouting))

const result = await name.resolve(key)
expect(result).to.have.deep.property('record', unmarshal(marshal(ipnsRecordFromRouting)))

// should have searched the routing
expect(customRouting.get.called).to.be.true()
})

it('should search the routing when a locally cached copy has an expired lifetime', async () => {
const key = await createEd25519PeerId()

const customRoutingKey = peerIdToRoutingKey(key)
const dhtKey = new Key('/dht/record/' + uint8ArrayToString(customRoutingKey, 'base32'), false)

// create a record with an expired lifetime but valid TTL
const ipnsRecord = await createWithExpiration(key, cid, 10, new Date(Date.now() - Math.pow(2, 10)).toString(), {
ttlNs: 10_000_000
})
const dhtRecord = new Record(customRoutingKey, marshal(ipnsRecord), new Date(Date.now()))

await datastore.put(dhtKey, dhtRecord.serialize())

// the routing returns a valid record with an higher sequence number
const ipnsRecordFromRouting = await create(key, cid, 11, Math.pow(2, 10), {
ttlNs: 10_000_000
})
customRouting.get.withArgs(customRoutingKey).resolves(marshal(ipnsRecordFromRouting))

const result = await name.resolve(key)
expect(result).to.have.deep.property('record', unmarshal(marshal(ipnsRecordFromRouting)))

// should have searched the routing
expect(customRouting.get.called).to.be.true()
})
})
Loading