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.

Sign up for GitHub

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

Jump to bottom

feat: add cache control to IPNS #473

Merged
merged 3 commits into from
Apr 2, 2024
Merged

feat: add cache control to IPNS #473

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
afa9785
feat: add cache control to IPNS
achingbrain Mar 16, 2024
259fd98
chore: add missing dep
achingbrain Mar 16, 2024
0539d9e
chore: apply suggestions from code review
achingbrain Mar 27, 2024
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
1 change: 1 addition & 0 deletions packages/ipns/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@
"@types/dns-packet": "^5.6.5",
"aegir": "^42.2.5",
"datastore-core": "^9.2.9",
"it-drain": "^3.0.5",
"sinon": "^17.0.1",
"sinon-ts": "^2.0.0"
},
Expand Down
53 changes: 38 additions & 15 deletions packages/ipns/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,9 +294,18 @@

export interface ResolveOptions extends AbortOptions, ProgressOptions<ResolveProgressEvents | IPNSRoutingEvents> {
/**
* Do not query the network for the IPNS record (default: false)
* Do not query the network for the IPNS record
*
* @default false
*/
offline?: boolean

/**
* Do not use cached DNS entries
achingbrain marked this conversation as resolved.
Show resolved Hide resolved
*
* @default false
*/
nocache?: boolean
}

export interface ResolveDNSLinkOptions extends AbortOptions, ProgressOptions<ResolveDNSLinkProgressEvents> {
Expand Down Expand Up @@ -523,22 +532,40 @@
}

async #findIpnsRecord (routingKey: Uint8Array, options: ResolveOptions = {}): Promise<IPNSRecord> {
let routers = [
this.localStore,
...this.routers
]
const records: Uint8Array[] = []
const cached = await this.localStore.has(routingKey, options)

if (cached) {
log('record is present in the cache')

if (options.nocache !== true) {
Copy link
Member

Choose a reason for hiding this comment

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

In a sense, we're breaking from the IPNS spec by not respecting the TTL value:

TTL: A hint for how long (in nanoseconds) the record should be cached before going back to, for instance the DHT, in order to check if it has been updated. The function and trade-offs of this value are analogous to the TTL of DNS record.

This is somewhat related to this PR. If we were to respect the TTL, we could apply TTL logic when nocache is undefined. However this would also require us to store a timestamp when we resolve the record the first time. Sounds like something for a separate PR.

Copy link
Member

@lidel lidel Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Better to address this now, in this PR. I am afraid if we don't leverage TTL in caching decision here, we will have users complaining that "(JS-)IPNS is slow" because the result will be cached until signature is valid, which is usually 2 days. So for 2 days you get no updates.

Adding TTL-awareness is not hard, instead of caching record, cache object with timestamp field { record, cacheEOL } where cacheEOL is min((now()+record-ttl) ,record-validity-expiration), and then when reading a cached entry back, ignore it if now()>cacheEOL.

Prior art in boxo/namesys/ipns_resolver#calculateBestTTL

Copy link
Member Author

Choose a reason for hiding this comment

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

nocache is undefined by default, so the local datastore is queried first. If the record is present, and ipnsValidator does not throw (e.g. it's structurally valid and we are within the TTL), the record is returned.

the result will be cached until signature is valid, which is usually 2 days. So for 2 days you get no updates.

I think you mean "while TTL is valid", please correct me if I'm wrong.

I'm a little confused here, as I read it, this is by design:

TTL: how long the record should be cached before going back to, for instance the DHT, in order to check if it has been updated. (emphasis mine)

So you don't check for updates while the TTL is valid. If you want to bypass the cache and check for updates, pass nocache: true.

If boxo is doing something else, then it's not following the spec, or the spec needs to be updated to say that we should check for updates every time.

Also, the suggested default in the spec is one hour, not two days?

Copy link
Member

@lidel lidel Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IPNS Records have two values:

  • TTL (kubo default ~1h)
  • Signature Validity (kubo default ~48h)

What I meant is that as far I can see, #findIpnsRecord has no concept of cache expiration based on TTL (if it happens on different abstraction later, lmk where). When TTL is not implemented for caching, Signature Validity takes its place and you get "IPNS is slow (to update)" behavior:

  • User resolves IPNS record, code saves it to this.localStore
  • All subsequent lookups return the cached value from this.localStore
  • This works until the signature is no longer valid (and signatures are usually valid for 48 hours).
  • Node does not check for updates for 48h, instead of 1h (for example).

But I don't think we should block on this, over-caching is better than no caching, and TTL support can be added in follow-up PR.

Copy link
Member

Choose a reason for hiding this comment

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

if we don't store something like cacheEOL or storedAt timestamp, then we have no way to, as Daniel first mentioned, "[respect] the TTL value", or am I missing something?

this is the current flow:

requesting record for the first time

  1. check if this.localStore has record // NO
  2. query network for the record.
  3. validate that record is correct
  4. store record in cache (has ttl)
  5. return record to user

later requests for record (with default nocache=undefined)

  1. check if this.localStore has record // yes
  2. return cached record

we essentially need a check between 1 & 2 of later requests for record that will check if the record is expired.. unless this.localStore is already handling expiry, but it doesn't seem like it is:

╰─ ✔ ❯ rg 'localStore' packages/ipns
packages/ipns/src/routing/local-store.ts
26:export function localStore (datastore: Datastore): LocalStore {

packages/ipns/src/index.ts
240:import { localStore, type LocalStore } from './routing/local-store.js'
406:  private readonly localStore: LocalStore
416:    this.localStore = localStore(components.datastore)
426:      if (await this.localStore.has(routingKey, options)) {
428:        const buf = await this.localStore.get(routingKey, options)
437:      await this.localStore.put(routingKey, marshaledRecord, options)
536:    const cached = await this.localStore.has(routingKey, options)
543:        const record = await this.localStore.get(routingKey, options)
604:    await this.localStore.put(routingKey, record, options)

packages/ipns/src/routing/pubsub.ts
10:import { localStore, type LocalStore } from './local-store.js'
34:  private readonly localStore: LocalStore
40:    this.localStore = localStore(components.datastore)
74:    if (await this.localStore.has(routingKey)) {
75:      const currentRecord = await this.localStore.get(routingKey)
91:    await this.localStore.put(routingKey, message.data)
131:      return await this.localStore.get(routingKey, options)

we have no calls to remove, and localStore doesn't have any methods other than put/get/has, and nothing called on the datastore (given in localStore creation) other than put/get/has

Copy link
Member

Choose a reason for hiding this comment

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

if we don't store something like cacheEOL or storedAt timestamp, then we have no way to, as Daniel first mentioned, "[respect] the TTL value", or am I missing something?

That's also my understanding. @SgtPooki's summed it up nicely.

Either, no need to block this PR

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

try {
await ipnsValidator(routingKey, record)

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

return unmarshal(record)
} catch (err) {
this.log('cached record was invalid', err)
Copy link
Member

Choose a reason for hiding this comment

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

should we remove it from the cache? or are we waiting on it being overwritten with later processing?

Copy link
Member

Choose a reason for hiding this comment

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

we already did the work and confirmed it is invalid, removing it sgtm

}
} else {
log('ignoring local cache due to nocache=true option')
}
}

if (options.offline === true) {
routers = [
this.localStore
]
throw new CodeError('Record was not present in the cache or has expired', 'ERR_NOT_FOUND')

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

View check run for this annotation

Codecov / codecov/patch

packages/ipns/src/index.ts#L560 

Added line #L560 was not covered by tests
}

const records: Uint8Array[] = []
log('did not have record locally')

let foundInvalid = 0

await Promise.all(
routers.map(async (router) => {
this.routers.map(async (router) => {
let record: Uint8Array

try {
Expand All @@ -547,11 +574,7 @@
validate: false
})
} catch (err: any) {
if (router === this.localStore && err.code === 'ERR_NOT_FOUND') {
log('did not have record locally')
} else {
log.error('error finding IPNS record', err)
}
log.error('error finding IPNS record', err)

return
}
Expand Down
8 changes: 4 additions & 4 deletions packages/ipns/test/resolve-dnslink.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ describe('resolveDNSLink', () => {

await name.publish(key, cid)

const result = await name.resolveDNSLink('foobar.baz', { nocache: true })
const result = await name.resolveDNSLink('foobar.baz')

if (result == null) {
throw new Error('Did not resolve entry')
Expand Down Expand Up @@ -186,7 +186,7 @@ describe('resolveDNSLink', () => {

await name.publish(key, cid)

const result = await name.resolveDNSLink('foobar.baz', { nocache: true })
const result = await name.resolveDNSLink('foobar.baz')

if (result == null) {
throw new Error('Did not resolve entry')
Expand All @@ -213,7 +213,7 @@ describe('resolveDNSLink', () => {

await name.publish(key, cid)

const result = await name.resolveDNSLink('foobar.baz', { nocache: true })
const result = await name.resolveDNSLink('foobar.baz')

if (result == null) {
throw new Error('Did not resolve entry')
Expand All @@ -235,7 +235,7 @@ describe('resolveDNSLink', () => {

await name.publish(key, cid)

const result = await name.resolveDNSLink('foobar.baz', { nocache: true })
const result = await name.resolveDNSLink('foobar.baz')

if (result == null) {
throw new Error('Did not resolve entry')
Expand Down
54 changes: 36 additions & 18 deletions packages/ipns/test/resolve.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ 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 drain from 'it-drain'
import { CID } from 'multiformats/cid'
import Sinon from 'sinon'
import { type StubbedInstance, stubInterface } from 'sinon-ts'
Expand Down Expand Up @@ -46,14 +47,14 @@ describe('resolve', () => {

it('should resolve a record', async () => {
const key = await createEd25519PeerId()
await name.publish(key, cid)
const record = await name.publish(key, cid)

const resolvedValue = await name.resolve(key)
// empty the datastore to ensure we resolve using the routing
await drain(datastore.deleteMany(datastore.queryKeys({})))

if (resolvedValue == null) {
throw new Error('Did not resolve entry')
}
heliaRouting.get.resolves(marshal(record))

const resolvedValue = await name.resolve(key)
expect(resolvedValue.cid.toString()).to.equal(cid.toV1().toString())

expect(heliaRouting.get.called).to.be.true()
Expand All @@ -70,15 +71,42 @@ describe('resolve', () => {
const resolvedValue = await name.resolve(key, {
offline: true
})
expect(resolvedValue.cid.toString()).to.equal(cid.toV1().toString())

expect(heliaRouting.get.called).to.be.false()
expect(customRouting.get.called).to.be.false()
})

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

const key = await createEd25519PeerId()
const record = await name.publish(key, cid)

heliaRouting.get.resolves(marshal(record))

const resolvedValue = await name.resolve(key, {
nocache: true
})
expect(resolvedValue.cid.toString()).to.equal(cid.toV1().toString())

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

if (resolvedValue == null) {
throw new Error('Did not resolve entry')
}
it('should retrieve from local cache when resolving a record', async () => {
const cacheGetSpy = Sinon.spy(datastore, 'get')

const key = await createEd25519PeerId()
await name.publish(key, cid)

const resolvedValue = await name.resolve(key)
expect(resolvedValue.cid.toString()).to.equal(cid.toV1().toString())

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

it('should resolve a recursive record', async () => {
Expand All @@ -88,11 +116,6 @@ describe('resolve', () => {
await name.publish(key1, key2)

const resolvedValue = await name.resolve(key1)

if (resolvedValue == null) {
throw new Error('Did not resolve entry')
}

expect(resolvedValue.cid.toString()).to.equal(cid.toV1().toString())
})

Expand All @@ -103,11 +126,6 @@ describe('resolve', () => {
await name.publish(key1, key2)

const resolvedValue = await name.resolve(key1)

if (resolvedValue == null) {
throw new Error('Did not resolve entry')
}

expect(resolvedValue.cid.toString()).to.equal(cid.toV1().toString())
})

Expand Down
Loading