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: add cache control to IPNS #473

Merged
merged 3 commits into from
Apr 2, 2024
Merged

Conversation

achingbrain
Copy link
Member

Adds a nocache option when resolving IPNS records similar to when resolving DNSLink records.

The record will be resolved from the local datastore and returned if it is valid (e.g. non-expired, correct key, etc).

If the record is not present it will be fetched from the routing and stored in the local datastore if it is valid.

  • nocache will skip the datastore and use the routing.
  • offline only uses the datastore and skips the routing.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

Adds a `nocache` option when resolving IPNS records similar to when
resolving DNSLink records.

The record will be resolved from the local datastore and returned if
it is valid (e.g. non-expired, correct key, etc).

If the record is not present it will be fetched from the routing and
stored in the local datastore if it is valid.

- `nocache` will skip the datastore and use the routing.
- `offline` only uses the datastore and skips the routing.
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

LGTM, one question


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

packages/ipns/src/index.ts Outdated Show resolved Hide resolved
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

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

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

Copy link
Member

@2color 2color left a comment

Choose a reason for hiding this comment

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

LGTM.

I think it would be good to also implement support for TTL by default, but not in scope for this PR.

Co-authored-by: Daniel Norman <1992255+2color@users.noreply.github.com>
@SgtPooki SgtPooki merged commit b00f682 into main Apr 2, 2024
18 checks passed
@SgtPooki SgtPooki deleted the feat/add-cache-control-to-ipns branch April 2, 2024 19:53
achingbrain added a commit that referenced this pull request Apr 3, 2024
Uses the `@libp2p/record` `timeReceived` property and the `.ttl` field
of the IPNS Record to calcuate the TTL of the record separately from
the record EOL.

This was going to be a push to #473 but it was merged prematurely.
achingbrain added a commit that referenced this pull request Apr 3, 2024
Uses the `@libp2p/record` `timeReceived` property and the `.ttl` field
of the IPNS Record to calcuate the TTL of the record separately from
the record EOL.

This was going to be a push to #473 but it was merged prematurely.
@achingbrain achingbrain mentioned this pull request Apr 3, 2024
3 tasks
achingbrain added a commit that referenced this pull request Apr 3, 2024
Uses the `@libp2p/record` `timeReceived` property and the `.ttl` field
of the IPNS Record to calculate the TTL of the record separately from
the record EOL.

This was going to be a push to #473 but it was merged prematurely.

---------

Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
@achingbrain achingbrain mentioned this pull request Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants