Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix(pubsub): multibase in pubsub http rpc #3922

Merged
merged 15 commits into from
Dec 15, 2021

Conversation

lidel
Copy link
Member

@lidel lidel commented Oct 19, 2021

This PR aims to restore interop with go-ipfs by applying the same changes as in ipfs/kubo#8183

TLDR is that we clean up and unify the API.

BREAKING CHANGE

We had to make breaking changes to pubsub commands sent over HTTP RPC to fix data corruption caused by topic names and payload bytes that included \n.
More details in ipfs/kubo#7939 and ipfs/kubo#8183

TODO

  • ipfs-http-client (JS lib used by people to talk to /api/v0 exposed by js-ipfs or go-ipfs)
  • ipfs-http-server (/api/v0 exposed by js-ipfs)
  • js-ipfs cli (file instead of "file per stin line" descoped. Independent of the programmatic JS API.
  • update pubsub tests in fix: multibase in pubsub http rpc interop tests interop#387 to test v1 and v2 circuits to the degree possible

This only adds comments marking places that need changes
to restore interop with go-ipfs after changes from
ipfs/kubo#8183 land
@lidel lidel force-pushed the feat/pubsub-require-multibase branch 6 times, most recently from 461224d to 7a2c68f Compare October 20, 2021 19:19
@lidel lidel force-pushed the feat/pubsub-require-multibase branch from 7a2c68f to be30a52 Compare October 22, 2021 13:56
@lidel lidel force-pushed the feat/pubsub-require-multibase branch from be30a52 to 1dcac76 Compare October 22, 2021 15:58
lidel added a commit to ipfs/interop that referenced this pull request Oct 25, 2021
This temporarily disables pubsub tests as we are unable to test interop
until both JS and GO changes land:
- ipfs/kubo#8183
- ipfs/js-ipfs#3922
@lidel lidel marked this pull request as ready for review October 25, 2021 14:06
lidel added a commit to ipfs/interop that referenced this pull request Oct 25, 2021
This temporarily disables pubsub tests as we are unable to test interop
until both JS and GO changes land:
- ipfs/kubo#8183
- ipfs/js-ipfs#3922

When we have that, we will re-enable skipped tests
in #387
@lidel
Copy link
Member Author

lidel commented Nov 8, 2021

@achingbrain i believe we want to merge this when we ship go-ipfs 0.11.0-rc1, so people can test the new wire format with @next version of http-client.
fysa the updated interop tests that use code from this PR can be previewed in ipfs/interop#387

lidel added a commit to coryschwartz/go-ipfs that referenced this pull request Nov 29, 2021
lidel added a commit to ipfs/kubo that referenced this pull request Nov 29, 2021
* multibase encoding on pubsub
* emit multibase for json clients
* refactor(pubsub): base64url for all URL args

This makes it easier to reason about.
Also added better helptext to each command explaining how the binary
data is encoded on the wire, and how to process it in userland.

* refactor: remove ndpayload and lenpayload

Those output formats are undocumented and seem to be only used in tests.
This change removes their implementation and replaces it with error
message to use JSON instead.

I also refactored tests to test the --enc=json response format instead
of imaginary one, making tests more useful as they also act as
regression tests for HTTP RPC.

* test(pubsub): go-ipfs-api

Testing against compatible version from
ipfs/go-ipfs-api#255

* refactor: safeTextListEncoder

Making it clear what it does and why

* refactor(pubsub): unify peerids

This ensures `ipfs pubsub sub` returns the same peerids in the `From`
field as `ipfs pubsub peers`.

libp2p already uses base encoding, no need to double wrap or use custom
multibase.

* test(pubsub): go-ipfs-http-client

* refactor(pubsub): make pub command read from a file

We want to send payload in the body as multipart so users can use
existing tools like curl for publishing arbitrary bytes to a topic.

StringArg was created for "one message per line" use case, and if data
has `\n` or `\r\n` byte sequences, it will cause payload to be split. It
is not possible to undo this, because mentioned sequences are lost, so
we are not able to tell if it was `\n` or `\r\n`

We already avoid this problem in `block put` and `dht put` by reading
payload via FileArg which does not mangle binary data and send it as-is.
It feel like `pubsub pub` should be using it in the first place anyway,
so this commit replaces StringArg with FileArg.

This also closes #8454
and makes rpc in go-ipfs easier to code against.

* test(pubsub): publishing with line breaks

Making sure we don't see regressions in the future.
Ref. #7939

* chore: disable pubsub interop for now

See
ipfs/interop@344f692

* test: t0322-pubsub-http-rpc.sh

- Adds HTTP RPC regression test that ensures topic is encoded as URL-safe
  multibase.
- Moves pubsub tests to live in unique range ./t032x

* fix(ci):  js-ipfs with  fixed pubsub wire format

uses js-ipfs from ipfs/js-ipfs#3922 
until js-ipfs release can ship with dependency on go-ipfs 0.11.0-rc1

Co-authored-by: Marcin Rataj <lidel@lidel.org>
Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com>
@lidel
Copy link
Member Author

lidel commented Dec 1, 2021

Resolved conflicts and switched to go-ipfs 0.11.0-rc1 and interop from ipfs/interop#feat/pubsub-require-multibase branch (ipfs/interop#387) – pubsub interop is passing.

@achingbrain my understanding is that this is ready, but:

const subName = 'sub-name'
const data = 'data'
const subName = 'sub-name-1'
const data = 'data\r\nfirst\nZażółć gęślą jaźń😇'
Copy link
Member

Choose a reason for hiding this comment

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

Yellow goose self?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds better in Polish ;) (it is a valid sentence that includes all polish diacritics, good for testing useful utf8)

SgtPooki referenced this pull request in ipfs/js-kubo-rpc-client Aug 18, 2022
This PR  aims to restore interop with go-ipfs by applying the same changes as in ipfs/kubo#8183 

TLDR is that we clean up and unify the API. 

BREAKING CHANGE: We had to make breaking changes to `pubsub` commands sent over HTTP RPC  to fix data corruption caused by topic names and payload bytes that included `\n`. More details in ipfs/kubo#7939 and ipfs/kubo#8183
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants