wrong IPNS DHT record selection algorithm

[Qmstream]

I'm not sure how to to deal with this as two modules are involved:

But the current IPNS record selection algorithm is wrong: it receives an array of records from the DHT but compares only the two first entries (and throws if only one entry is present):

js-ipfs/src/core/ipns/routing/utils.js

Line 15 in 0287a6c

selector: (k, records) => ipns.validator.select(records[0], records[1])

An alternative, correct implementation would be this:

selector: (k, records) => {
  const sequences = records.map(rec=>ipns.unmarshal(rec).sequence)
  return sequences.indexOf(Math.max.apply(Math,sequences))
}

But it's necessary to change the interface of ipns.validator.select:

https://github.com/ipfs/js-ipns/blob/ccc41788705e4bc4043a47af0aee86a29b2159a3/src/index.js#L310

(For reference: the records come from: https://github.com/libp2p/js-libp2p-kad-dht/blob/master/src/private.js#L294)

[Mapiac]

I might extend that whomever answers this, wondering if you could kindly also touch on when the DHT is expected to by out of alpha and ready for production deployments? I looked at the weekly team call logs which seemingly stopped (?) being documented in 2018. Gracias.

[alanshaw]

@vasco-santos can you speak to this? The implementation suggested by @Qmstream looks good to me. Is there another reason this was implemented like this? Does the selector function always receive 2 records?

Note, we'd have to change the implementation here as well as src/core/ipns/routing/utils.js:

https://github.com/ipfs/js-ipns/blob/ccc41788705e4bc4043a47af0aee86a29b2159a3/src/index.js#L310-L321

[vasco-santos]

Hey! thanks for reaching out @Mapiac

Something is incorrect here in fact! I believe this is valid for the selection when a peer is syncing its value back, but not for getting the record from the DHT. In this way, I would expect that we call the select function from js-ipns n-1 times, being n the length of record, in:

ipfs/js-ipfs/src/core/ipns/routing/utils.js#L15

Other option, that seems the most correct to me, would be to change the js-ipns validator.select function for what @Mapiac recommended. This way, we can do a breaking change release of js-ipns and pass the array instead in js-ipfs.

@Mapiac would you like to PR js-ipns with that change?

[alanshaw]

The bestRecord function passes all records to the selector.

https://github.com/libp2p/js-libp2p-record/blob/bd5b9944c7791e45fc905a83399e07d5771e7f7d/src/selection.js#L37

So either this function needs to change to call the selector multiple times, reducing the records down to 1, or the selector needs to change to accept multiple records (as @Qmstream suggests).

[Qmstream]

I also noticed the dht's getMany function is adding empty values for errored peers (no idea why). So it seems it would be necessary to also filter the records array:

https://github.com/libp2p/js-libp2p-kad-dht/blob/688d58844e12288a65f98706d61213185091807b/src/index.js#L366-L371

Is it really necessary to keep these empty/errored values? Why not simply:

if (rec && rec.value) {
  pathVals.push({
    val: rec.value,
    from: peer
  })
}

[edit:]

Oh, maybe you are keeping track of peers with bad records to later send them correction values? In that case perhaps it would be better to filter this list before sending them to ipns.selector.

How about changing

https://github.com/libp2p/js-libp2p-kad-dht/blob/688d58844e12288a65f98706d61213185091807b/src/private.js#L290

to

const recs = vals.map((v) => v.val).filter(Boolean)

[edit again:]

It's also necessary to change

https://github.com/libp2p/js-libp2p-kad-dht/blob/688d58844e12288a65f98706d61213185091807b/src/private.js#L329

to

 if (v.val && v.val.equals(best)) {

[alanshaw]

@Qmstream thank you reporting this - would you mind opening an issue for this on https://github.com/libp2p/js-libp2p-kad-dht ? 🙏

[RotonEvan]

Other option, that seems the most correct to me, would be to change the js-ipns validator.select function for what @Mapiac recommended. This way, we can do a breaking change release of js-ipns and pass the array instead in js-ipfs.

Should I submit a PR in js-ipns validator.select function here https://github.com/ipfs/js-ipns/blob/ccc41788705e4bc4043a47af0aee86a29b2159a3/src/index.js#L310-L321 to return the index of the best record with the greatest sequence?

And then submit another PR in js-ipfs to pass the array in selector here :

js-ipfs/src/core/ipns/routing/utils.js

Line 15 in 0287a6c

selector: (k, records) => ipns.validator.select(records[0], records[1])

[munavailable]

@vasco-santos @alanshaw looks like the js-ipns select is shared between DHT and pubsub. In pubsub also there's a problem selecting the newer record. I detailed it here: #3875

At the moment IPNS seems to be broken both on DHT and pubsub when it comes to getting the latest records.

[achingbrain]

js-ipfs is being deprecated in favor of Helia. You can #4336 and read the migration guide.

This should have been fixed in https://github.com/ipfs/helia-ipns - if it's not please open an issue on that repo.