[Feature] UUIDs, protocol versioning, v2 protocol w/ dag-cbor messaging #332

rvagg · 2022-01-13T04:48:49Z

I've tried to rebase #323 to #313 here (other way around was more messy). It's not working, partly because #323 isn't complete, but it's directionally good and I'm keen to try reconciling the two so we're reducing and not increasing differences as #323 evolves.

Using this for now to illustrate problems inline. I'll keep on working to clean this up and make it match the dag-cbor work.

graphsync.go

hannahhoward

copied from slack for posteriety:

Let's make this the main PR and rvagg/uuid-rebasing the epic branch for this. We can close the old PRs. (also feel free to rename the branch if you all agree on something)

Because there’s some remaining work to do with tasks for each person, I suggests we start making PRs to this branch for those units of work so that we don’t clobber each other’s work.

Looking at the PR, the big question marks I see to landing this in main (still not release ready but into main) are:

We need to get two protobuf protocols so we have only one protobuf + 1 IPLD, and wherever we were using proto 1.1, we now use IPLD, which is the 1.1 protocol — I figure @rvagg is the best to do this.
We need to figure out the extensions… and… I’m not sure this is so simple. Right now it doesn’t look like the FromIPLD / ToIPLD methods are handling extensions. I believe that is because the extensions type in the wrapper message still needs to change to the types that Dan had setup earlier filtering out through the code base? And then for compatibility with protobuf, I think we’ll need to just CBOR serialize all the extension Nodes to bytes when sending over 1.0 protocol. @mvdan if you can work on this that’d be awesome.
1. This will have the effect of changing the extensions type all the way up to the top level, which will cause some breakage in libraries using graphsync. I think that is ok. The only other option is to figure out how to implement Deferred at the byte level. But I think that’s looking hard?

Once we merge to main, there are still a few protocol changes I want to make sure get shipped before release. They are:

The IPLD format should encode Metadata in the response not as an extension. I’ll write up a ticket for this
I may want to look at adding a few control mechanisms so we can eventually refactor away the data transfer libp2p protocol. (TBD)
A few other things TBD but also I get there’s a moment we need to draw the line.
(edited)

Message w3dt-datasystems-team

message/ipldbind/message.go

also added first roundtrip benchmark

Ref: #278 Closes: #279 Closes: #281

Not quite working yet, still need some upstream fixes and no extensions work has been attempted yet.

…d of []byte * Extensions now working with new dag-cbor network protocol * dag-cbor network protocol still not default, most tests are still exercising the existing v1 protocol * Metadata now using bindnode instead of cbor-gen * []byte for deferred extensions decoding is now replaced with datamodel.Node everywhere. Internal extensions now using some form of go-ipld-prime decode to convert them to local types (metadata using bindnode, others using direct inspection). * V1 protocol also using dag-cbor decode of extensions data and exporting the bytes - this may be a breaking change for exising extensions - need to check whether this should be done differently. Maybe a try-decode and if it fails export a wrapped Bytes Node?

rvagg · 2022-01-25T10:51:26Z

network/libp2p_impl.go

+	case ProtocolGraphsync_2_0_0:
+		return smh.v2MessageHandler
+	default:
+		return messageHandlerErrorer{fmt.Errorf("unrecognized protocol version: %s", protocol)}


I'd quite like to test this but don't know how! I'd need to set up a host, then connect to it with a stream with an entirely different protocol stream. Hints would be good.

hannahhoward

Overall this is looking good. I've added a few TODOs to the project backlog for this task, and let's try to PR to this branch looking forward.

You can find the burndown for the protocol refactor here: https://github.com/orgs/filecoin-project/projects/30/views/27

hannahhoward · 2022-01-25T21:56:03Z

graphsync.go

 	"github.com/ipld/go-ipld-prime/traversal"
 	"github.com/libp2p/go-libp2p-core/peer"
 )

 // RequestID is a unique identifier for a GraphSync request.
-type RequestID int32
+type RequestID struct{ string }


Just noticed this -- there a reason for a request ID to be a struct { string } as opposed to just a string? I noticed we aren't particularly uniform with this actually. CID is struct{string} while filecoin's Address and libp2p's peer.ID. I'm wonder about the tradeoffs... anyway, just food for thought.

I experimented with a few things here but landed on this after copying cid.CID's pattern with the main reasoning being that this is really a byte string, I don't want users to consider it reasonable to cast this to a user-readable string (knowing it's a UUID and seeing it's a typed string, that would be a reasonable API assumption I reckon), they really should .String() it to get the string form of a UUID. The same thing holds for CID (and probably should for other types that are not utf8 strings IMO).

And, I didn't use []byte because it needs to be used for map keys (I learnt my Go lesson about that while trying it!).

Happy to change it if you think it should be a simple string though.

message/ipldbind/message.go

hannahhoward · 2022-01-25T22:03:23Z

message/ipldbind/message.go

+	Id []byte
+
+	Status     graphsync.ResponseStatusCode
+	Metadata   []GraphSyncMetadatum


I guess we included this but didn't actually use it yet?

message/message.go

message/pb/message.pb.go

message/v1/message.go

hannahhoward · 2022-01-25T22:39:30Z

responsemanager/responseassembler/responseBuilder.go

@@ -102,7 +104,14 @@ func (eo extensionOperation) build(builder *messagequeue.Builder) {
 }

 func (eo extensionOperation) size() uint64 {
-	return uint64(len(eo.extension.Data))
+	// TODO: this incurs a double-encode, this first one is just to get the expected length;


yea this is not great. let's discuss whether there's some kind of caching strategy? Maybe not

being addressed in #340

Ref: #332 (comment)

v1.1.0 was introduced to start the transition to UUID RequestIDs. That change has since been combined with the switch to DAG-CBOR messaging format for a v2.0.0 protocol. Thus, this interim v1.1.0 format is no longer needed and has not been used at all in a released version of go-graphsync. Fixes: filecoin-project/lightning-planning#14

As per dission in #338, we are going to be erroring on extension data that is not properly dag-cbor encoded from now on

…342) * feat(metadata): new LinkMetadata iface, integrate metadata into Response type * LinkMetadata wrapper around existing metadata type to allow for easier backward-compat upgrade path * integrate metadata directly into GraphSyncResponse type, moving it from an optional extension * still deal with metadata as an extension for now—further work for v2 protocol will move it into the core message schema Ref: #335 * feat(metadata): move metadata to core protocol, only use extension in v1 proto * fix(metadata): bindnode expects Go enum strings to be at the type level * fix(metadata): minor fixes, tidy up naming * fix(metadata): make gofmt and staticcheck happy * fix(metadata): docs and minor tweaks after review Co-authored-by: Daniel Martí <[email protected]>

Closes: filecoin-project/lightning-planning#15

…e bools (#352) Closes: #345

* feat!(messagev2): tweak dag-cbor message schema For: 1. Efficiency: compacting the noisy structures into tuples representations and making top-level components of a message optional. 2. Migrations: providing a secondary mechanism to lean on for versioning if we want a gentler upgrade path than libp2p protocol versioning. Closes: #351 * fix(messagev2): adjust schema per feedback

#355) * feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID Closes: #349 * fixup! feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID * fixup! feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID when using error type T, use *T with As, rather than **T * fixup! feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID * fixup! feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID Co-authored-by: Daniel Martí <[email protected]>

…equest

If request has finished selector traversal but is still sending blocks, I think it should be possible to send updates. As a side effect, this fixes our race. Logically, this makes sense, cause our external indicator that we're done (completed response listener) has not been called.

* feat(reconciledloader): first working version of reconciled loader * feat(traversalrecorder): add better recorder for traversals * feat(reconciledloader): pipe reconciled loader through code style(lint): fix static checks * Update requestmanager/reconciledloader/injest.go Co-authored-by: Rod Vagg <[email protected]> * feat(reconciledloader): respond to PR comments Co-authored-by: Rod Vagg <[email protected]>

hannahhoward · 2022-02-18T20:55:48Z

Attn shoppers! It's time to put this baby in main!

hannahhoward

LGTM! Even if I wrote half of this.

At this point we've tested this a lot, and it makes no sense to leave it outside of main, IMHO.

We're still not tagged release ready, but closing in.

…ng (#332) * feat(net): initial dag-cbor protocol support also added first roundtrip benchmark * feat(requestid): use uuids for requestids Ref: ipfs/go-graphsync#278 Closes: ipfs/go-graphsync#279 Closes: ipfs/go-graphsync#281 * fix(requestmanager): make collect test requests with uuids sortable * fix(requestid): print requestids as string uuids in logs * fix(requestid): use string as base type for RequestId * chore(requestid): wrap requestid string in a struct * feat(libp2p): add v1.0.0 network compatibility * chore(net): resolve most cbor + uuid merge problems * feat(net): to/from ipld bindnode types, more cbor protoc improvements * feat(net): introduce 2.0.0 protocol for dag-cbor * fix(net): more bindnode dag-cbor protocol fixes Not quite working yet, still need some upstream fixes and no extensions work has been attempted yet. * chore(metadata): convert metadata to bindnode * chore(net,extensions): wire up IPLD extensions, expose as Node instead of []byte * Extensions now working with new dag-cbor network protocol * dag-cbor network protocol still not default, most tests are still exercising the existing v1 protocol * Metadata now using bindnode instead of cbor-gen * []byte for deferred extensions decoding is now replaced with datamodel.Node everywhere. Internal extensions now using some form of go-ipld-prime decode to convert them to local types (metadata using bindnode, others using direct inspection). * V1 protocol also using dag-cbor decode of extensions data and exporting the bytes - this may be a breaking change for exising extensions - need to check whether this should be done differently. Maybe a try-decode and if it fails export a wrapped Bytes Node? * fix(src): fix imports * fix(mod): clean up go.mod * fix(net): refactor message version format code to separate packages * feat(net): activate v2 network as default * fix(src): build error * chore: remove GraphSyncMessage#Loggable Ref: ipfs/go-graphsync#332 (comment) * chore: remove intermediate v1.1 pb protocol message type v1.1.0 was introduced to start the transition to UUID RequestIDs. That change has since been combined with the switch to DAG-CBOR messaging format for a v2.0.0 protocol. Thus, this interim v1.1.0 format is no longer needed and has not been used at all in a released version of go-graphsync. Fixes: filecoin-project/lightning-planning#14 * fix: clarify comments re dag-cbor extension data As per dission in ipfs/go-graphsync#338, we are going to be erroring on extension data that is not properly dag-cbor encoded from now on * feat: new LinkMetadata iface, integrate metadata into Response type (#342) * feat(metadata): new LinkMetadata iface, integrate metadata into Response type * LinkMetadata wrapper around existing metadata type to allow for easier backward-compat upgrade path * integrate metadata directly into GraphSyncResponse type, moving it from an optional extension * still deal with metadata as an extension for now—further work for v2 protocol will move it into the core message schema Ref: ipfs/go-graphsync#335 * feat(metadata): move metadata to core protocol, only use extension in v1 proto * fix(metadata): bindnode expects Go enum strings to be at the type level * fix(metadata): minor fixes, tidy up naming * fix(metadata): make gofmt and staticcheck happy * fix(metadata): docs and minor tweaks after review Co-authored-by: Daniel Martí <[email protected]> * fix: avoid double-encode for extension size estimation Closes: filecoin-project/lightning-planning#15 * feat(requesttype): introduce RequestType enum to replace cancel&update bools (#352) Closes: ipfs/go-graphsync#345 * fix(metadata): extend round-trip tests to byte representation (#350) * feat!(messagev2): tweak dag-cbor message schema (#354) * feat!(messagev2): tweak dag-cbor message schema For: 1. Efficiency: compacting the noisy structures into tuples representations and making top-level components of a message optional. 2. Migrations: providing a secondary mechanism to lean on for versioning if we want a gentler upgrade path than libp2p protocol versioning. Closes: ipfs/go-graphsync#351 * fix(messagev2): adjust schema per feedback * feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID (#355) * feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID Closes: ipfs/go-graphsync#349 * fixup! feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID * fixup! feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID when using error type T, use *T with As, rather than **T * fixup! feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID * fixup! feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID Co-authored-by: Daniel Martí <[email protected]> * feat: SendUpdates() API to send only extension data to via existing request * fix(responsemanager): send update while completing If request has finished selector traversal but is still sending blocks, I think it should be possible to send updates. As a side effect, this fixes our race. Logically, this makes sense, cause our external indicator that we're done (completed response listener) has not been called. * fix(requestmanager): revert change to pointer type * Refactor async loading for simplicity and correctness (#356) * feat(reconciledloader): first working version of reconciled loader * feat(traversalrecorder): add better recorder for traversals * feat(reconciledloader): pipe reconciled loader through code style(lint): fix static checks * Update requestmanager/reconciledloader/injest.go Co-authored-by: Rod Vagg <[email protected]> * feat(reconciledloader): respond to PR comments Co-authored-by: Rod Vagg <[email protected]> * fix(requestmanager): update test for rebase Co-authored-by: Daniel Martí <[email protected]> Co-authored-by: hannahhoward <[email protected]>

rvagg commented Jan 13, 2022

View reviewed changes

graphsync.go Show resolved Hide resolved

rvagg mentioned this pull request Jan 13, 2022

WIP: add the ipld plus dag-cbor protocol v1.1 #323

Closed

hannahhoward reviewed Jan 14, 2022

View reviewed changes

message/ipldbind/message.go Outdated Show resolved Hide resolved

message/ipldbind/message.go Outdated Show resolved Hide resolved

rvagg force-pushed the rvagg/uuid-rebasing branch from 8690c1a to eb92cea Compare January 14, 2022 05:43

rvagg mentioned this pull request Jan 14, 2022

bindnode doesn't calculate map Length() properly when properties are absent ipld/go-ipld-prime#332

Closed

mvdan and others added 2 commits January 17, 2022 16:33

feat(net): initial dag-cbor protocol support

f330fe8

also added first roundtrip benchmark

feat(requestid): use uuids for requestids

480fcf5

Ref: #278 Closes: #279 Closes: #281

rvagg force-pushed the rvagg/uuid-rebasing branch from eb92cea to 1b71aa1 Compare January 17, 2022 05:37

rvagg and others added 8 commits January 17, 2022 16:47

fix(requestmanager): make collect test requests with uuids sortable

cf6009a

fix(requestid): print requestids as string uuids in logs

af9dd52

fix(requestid): use string as base type for RequestId

a2a87fe

chore(requestid): wrap requestid string in a struct

d25c6ef

feat(libp2p): add v1.0.0 network compatibility

901b2c0

chore(net): resolve most cbor + uuid merge problems

caa4b58

feat(net): to/from ipld bindnode types, more cbor protoc improvements

8ef2008

feat(net): introduce 2.0.0 protocol for dag-cbor

ea4a06a

rvagg force-pushed the rvagg/uuid-rebasing branch from 1b71aa1 to 1f7cb30 Compare January 17, 2022 05:47

rvagg force-pushed the rvagg/uuid-rebasing branch 3 times, most recently from 20d9c2e to 5bd05cc Compare January 24, 2022 06:26

rvagg added 3 commits January 25, 2022 15:40

fix(net): more bindnode dag-cbor protocol fixes

45b87ff

Not quite working yet, still need some upstream fixes and no extensions work has been attempted yet.

chore(metadata): convert metadata to bindnode

c3649f3

rvagg force-pushed the rvagg/uuid-rebasing branch from 5bd05cc to cb45833 Compare January 25, 2022 04:49

fix(src): fix imports

9c08dd0

rvagg changed the title ~~First (bad) attempt at merging UUID & protocol versioning with dag-cbor messaging~~ [Feature] UUIDS, protocol versioning, v2 protocol w/ dag-cbor messaging Jan 25, 2022

rvagg added 3 commits January 25, 2022 15:53

fix(mod): clean up go.mod

989cf66

fix(net): refactor message version format code to separate packages

fc58e23

feat(net): activate v2 network as default

161a577

rvagg force-pushed the rvagg/uuid-rebasing branch from ff6f3ed to 161a577 Compare January 25, 2022 10:15

fix(src): build error

e997ff3

rvagg force-pushed the rvagg/uuid-rebasing branch from 1cc1fe4 to e997ff3 Compare January 25, 2022 10:38

rvagg commented Jan 25, 2022

View reviewed changes

rvagg mentioned this pull request Jan 25, 2022

feat(requestid): use uuids for requestids #313

Closed

4 tasks

hannahhoward reviewed Jan 25, 2022

View reviewed changes

rvagg added a commit that referenced this pull request Jan 31, 2022

chore: remove GraphSyncMessage#Loggable

348b34c

Ref: #332 (comment)

This was referenced Jan 31, 2022

chore: remove GraphSyncMessage#Loggable #337

Merged

feat: add dagcbor.EncodedLength(Node) to calculate byte length without encoding ipld/go-ipld-prime#346

Merged

rvagg and others added 5 commits February 1, 2022 08:32

chore: remove GraphSyncMessage#Loggable

4e57d92

Ref: #332 (comment)

fix: clarify comments re dag-cbor extension data

eb16b27

As per dission in #338, we are going to be erroring on extension data that is not properly dag-cbor encoded from now on

fix: avoid double-encode for extension size estimation

21a4546

Closes: filecoin-project/lightning-planning#15

mvdan mentioned this pull request Feb 4, 2022

Replace protobuf messages with IPLD #88

Open

rvagg and others added 8 commits February 4, 2022 13:07

feat(requesttype): introduce RequestType enum to replace cancel&updat…

ff4d7ad

…e bools (#352) Closes: #345

fix(metadata): extend round-trip tests to byte representation (#350)

586931b

feat: SendUpdates() API to send only extension data to via existing r…

e27c827

…equest

fix(requestmanager): revert change to pointer type

db297c1

fix(requestmanager): update test for rebase

c938c6b

hannahhoward approved these changes Feb 18, 2022

View reviewed changes

hannahhoward merged commit 8c66ab9 into main Feb 18, 2022

mvdan deleted the rvagg/uuid-rebasing branch March 7, 2022 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] UUIDs, protocol versioning, v2 protocol w/ dag-cbor messaging #332

[Feature] UUIDs, protocol versioning, v2 protocol w/ dag-cbor messaging #332

rvagg commented Jan 13, 2022

hannahhoward left a comment •

edited

Loading

rvagg Jan 25, 2022

hannahhoward left a comment

hannahhoward Jan 25, 2022

rvagg Jan 27, 2022

hannahhoward Jan 25, 2022

hannahhoward Jan 25, 2022

rvagg Feb 2, 2022

hannahhoward commented Feb 18, 2022

hannahhoward left a comment

[Feature] UUIDs, protocol versioning, v2 protocol w/ dag-cbor messaging #332

[Feature] UUIDs, protocol versioning, v2 protocol w/ dag-cbor messaging #332

Conversation

rvagg commented Jan 13, 2022

hannahhoward left a comment • edited Loading

Choose a reason for hiding this comment

rvagg Jan 25, 2022

Choose a reason for hiding this comment

hannahhoward left a comment

Choose a reason for hiding this comment

hannahhoward Jan 25, 2022

Choose a reason for hiding this comment

rvagg Jan 27, 2022

Choose a reason for hiding this comment

hannahhoward Jan 25, 2022

Choose a reason for hiding this comment

hannahhoward Jan 25, 2022

Choose a reason for hiding this comment

rvagg Feb 2, 2022

Choose a reason for hiding this comment

hannahhoward commented Feb 18, 2022

hannahhoward left a comment

Choose a reason for hiding this comment

hannahhoward left a comment •

edited

Loading