Skip to content

Conversation

@lidel
Copy link
Member

@lidel lidel commented Aug 21, 2024

@lidel lidel added the skip/changelog This change does NOT require a changelog entry label Aug 21, 2024
@lidel lidel changed the title chore: go-libp2p-kad-dht v0.26.0 chore: update go-libp2p-kad-dht Aug 21, 2024
@lidel lidel mentioned this pull request Aug 21, 2024
32 tasks
v0.26.0 missed some commits, checking if icluding them helps with
failing helia interop

libp2p/go-libp2p-kad-dht#980
go.mod Outdated
github.com/libp2p/go-libp2p v0.36.2
github.com/libp2p/go-libp2p-http v0.5.0
github.com/libp2p/go-libp2p-kad-dht v0.25.2
github.com/libp2p/go-libp2p-kad-dht v0.26.0
Copy link
Member Author

Choose a reason for hiding this comment

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

go-libp2p-kad-dht v0.26.0 breaks helia-interop (log)

2024-08-21_16-15

Copy link
Member Author

Choose a reason for hiding this comment

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

v0.26.0 was missing some commits, but testing with libp2p/go-libp2p-kad-dht#980 also fails in the same place (log).

Needs analysis. Kubo being able to resolve thing when it is not expected to is a weird regression, I suspect kad-dht client got better somehow, and helia-interop test no longer making sense, may be requiring update.

Copy link
Member Author

@lidel lidel Aug 21, 2024

Choose a reason for hiding this comment

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

Uppon inspection it feels the failing "helia→pubsub→kubo" test is a bit hacky and does not follow what is done is his sibling "kubo→pubsub→helia":

https://github.com/ipfs/helia/blob/777d868c2c27e3ef0eaafab5c255a4f60e275874/packages/interop/src/ipns-pubsub.spec.ts#L81-L88:

// first publish should fail because kubo isn't subscribed to key update channel
await expect(name.publish(peerId, cid)).to.eventually.be.rejected()
  .with.property('message', 'PublishError.NoPeersSubscribedToTopic')

// should fail to resolve the first time as kubo was not subscribed to the pubsub channel
await expect(last(kubo.api.name.resolve(peerId, {
  timeout: 100
}))).to.eventually.be.undefined()

This is brittle, it calls name.publish when it should not, and likely being prone to race bugs like the one in this PR, likely due to changes in libp2p networking stack.

Since this test is not related to DHT, I think we've hit the brittle surface.
Good news is that there are better ways of confirming that peers are or are not subscribed to a topic.

I've submitted upstream fix in ipfs/helia#584, will switch to it before merging this PR.

lidel added a commit to ipfs/helia that referenced this pull request Aug 21, 2024
lidel added a commit to ipfs/helia that referenced this pull request Aug 21, 2024
lidel added a commit to ipfs/helia that referenced this pull request Aug 21, 2024
@lidel lidel marked this pull request as ready for review August 21, 2024 21:52
@lidel lidel requested a review from a team as a code owner August 21, 2024 21:52
@lidel lidel changed the title chore: update go-libp2p-kad-dht chore: go-libp2p-kad-dht v0.26.1 Aug 21, 2024
Copy link
Member Author

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Merging as this dependency update finally fixes problem @2color fixed in libp2p/go-libp2p-kad-dht#976

This is also unblocking us to start Kubo 0.30.0-rc1 release dance in #10436

@lidel lidel merged commit ca95b63 into master Aug 21, 2024
@lidel lidel deleted the chore/update-go-libp2p-kad-dht branch August 21, 2024 21:56
achingbrain pushed a commit to ipfs/helia that referenced this pull request Sep 13, 2024
IPNS interop tests for helia→pubsub→kubo did not execute the same tests as kubo→pubsub→helia.

Namely, publishing to IPNS was executed with assumption it will fail, as a convoluted way of confirming the kubo is not subscribed to the topic, and also kubo connectivity state is somehow taken on belief, rather than being programmatically verified:

```js
// first publish should fail because kubo isn't subscribed to key update channel
await expect(name.publish(peerId, cid)).to.eventually.be.rejected()
  .with.property('message', 'PublishError.NoPeersSubscribedToTopic')

// should fail to resolve the first time as kubo was not subscribed to the pubsub channel
await expect(last(kubo.api.name.resolve(peerId, {
  timeout: 100
}))).to.eventually.be.undefined()
```

Good news is that there are native APIs for inspecting subsub topic subscriptions, and this PR refactors test to use them and have tests in both directions do more-or-less the same thing with the same asserts.

This should make interop less brittle.

ipfs/kubo#10488 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip/changelog This change does NOT require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants