refactor: Change from protobuf to cbor for IPLD #2604

fredcarle · 2024-05-09T02:16:22Z

Relevant issue(s)

Resolves #2603

Description

This PR changes the encoding of the blocks in the blockstore to use cbor instead of protobof. As part of this, we move away from encoding/decoding individual deltas (no more Marshalling and Unmarshalling deltas) and instead, encode/decode at the block level. To do this, we use the new github.com/ipld/go-ipld-prime package that uses IPLD schemas to map the encoded blocks to the appropriate fields and CRDT types. The new core/block package handles that part of the change.

The net package is also affected by that change with the DAGSyncer being replaced by the new ipld/linking.LinkSystem. I expect further usage of the LinkSystem in a couple subsequent PRs.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test

Specify the platform(s) on which this was tested:

MacOS

AndrewSisley

Looks really good Fred, thanks :) Code is much nicer. I only have a handful of small comments for you :)

AndrewSisley · 2024-05-09T14:21:29Z

events/db_update.go

-	// Priority is used to determine the order in which concurrent updates are applied.
-	Priority uint64
+	// IsCreate is true if this update is the creation of a new document.
+	IsCreate bool


praise: Thanks for simplifying this :)

question: Is this going to work long term? Is there still a priority encoded within the Block?

Yes the priority is in the block itself and it's not used while syncing. Only for merging.

AndrewSisley · 2024-05-09T14:31:45Z

internal/core/block/block.go

+}
+
+func mustSetSchema() schema.Type {
+	ts, err := ipld.LoadSchemaBytes([]byte(`


todo: I can see this getting out of sync quite easily, and whilst that will probably get flagged pretty quickly by tests, it may not be obvious what the error is to someone who doesnt work much in this space (and it is better that they dont mess up in the first place).

Particularly problematic may be the FooDeltas, as they don't live in this package even.

I have two suggestions to improve this, firstly that this string is broken up, so that the types define their respective schema (making the string and type closer in proximity) - mustSetSchema then will concat all the strings required, the strings could be consts, or they could perhaps be consts within a function on a new interface implemented by each type.

Secondly, please document that this string exists, on the types that are defined within it, including the ones in this file.

internal/core/block/block.go

internal/planner/commit.go

net/peer.go

net/process.go

net/server.go

tests/integration/events/simple/with_update_test.go

jsimnz · 2024-05-10T23:09:26Z

reviewing now

AndrewSisley

LGTM, one tiny todo in the PR, and another possible small one messaged over discord :)

AndrewSisley · 2024-05-13T13:37:49Z

internal/core/block/block.go

+// IPLDSchemaBytes returns the IPLD schema representation for the DAGLink.
+//
+// This needs to match the [DAGLink] struct or [mustSetSchema] will panic on init.
+func (l DAGLink) IPLDSchemaBytes() []byte {


praise: Thanks for moving this Fred, I like the new way of organising these :)

todo: Please add a var _ schemaDefinition = (*DAGLink)(nil) line, and similar for all relevant types. If nothing else it is more useful (and safer) documentation than the comment IMO.

This is already taken care of by a compile time check on line 37 of block.go. No need to worry about doing that for every type individually. It will automatically be taken care of for every new type that we add to mustSetSchema.

jsimnz

overall, big fan of these changes, and even moreso knowing what your next steps are, especially with the net package.

Couple of things ive noted in this PR, nothing giant, but one or two notable items.

jsimnz · 2024-05-10T23:10:19Z

internal/core/block/block.go

+// This needs to match the [Block] struct or [mustSetSchema] will panic on init.
+func (b Block) IPLDSchemaBytes() []byte {
+	return []byte(`
+	type Block struct {


suggestion/question: This might be a good time to add a block format version number to the blocks, for future upgradeability.

This one is worth a large discussion likely

It might be simpler for the version to be specified as a CID multicodec. I think its worth a larger discussion like you said.

Issue created: #2622

We can figure this out before the end of the cycle. Let's not block this PR for this.

internal/core/block/block.go

jsimnz · 2024-05-15T07:19:34Z

internal/core/crdt/composite.go

+// This needs to match the [CompositeDAGDelta] struct or [coreblock.mustSetSchema] will panic on init.
+func (delta *CompositeDAGDelta) IPLDSchemaBytes() []byte {
+	return []byte(`
+	type CompositeDAGDelta struct {


thought: Im not the biggest fan of these IPLD schema definitions defined inline like this. Its not a hug problem.

One possibility is to define the IPLD schemas in their own files, similar to a .proto file, and use the go embed directives to have access to the bytes.

Since it's a small amount of code/text, I find it more convenient to have it right there as you read the Go code. I don't see the benefit of separating them into their own files.

If we were to split them into their own files we'd probably want to force a one-one file correspondence with the Go code too otherwise which .proto file goes with which .go file would probably be a bit confusing - and I'm not sure we'd want to force that on the Go code, and splitting the go files would be better done in another PR anyway (I've not checked to be sure, but I don't think they are one type per file atm)

They are one type per file :)

Its just A) Somewhat ugly B) not well structured, and doesn't lend itself to working with all the IPLD schemas independent of the delta implementations.

Not necessarily about the amount of code the schemas create.

A) That is subjective.
B.1) It's well structured in the sense that it is right next to the Go type that represents it.
B.2) Why would we want to use them independently of the delta structs? They are merely a representation of the same thing. The IPLD schema is used to help translate from an IPLD node representation to the corresponding Go struct.

A) absolutely
B1) I meant not well structured as it's just some arbitrary inline byte string inside a method
B2) Now that we have dedicated IPLD schemas defined, it opens up a whole world of things we can do with them. Eg generate non Go serialization bindings, use the schemas in docs, can have util functions that operate on the schemas themselves, etc.

The reason I made the proto analogy, is that much like protobuf it actually has nothing to do with Go. IPLD schemas are general and language independent, and we're losing out on some potential functionally to have them inline like this.

This isn't a PR blocker, just a general design point for this new concept being brought into the codebase with this PR

B2) Ah!. The usage outside of Go entirely is something I wasn't considering in this conversation. That is not what I interpreted when you said "independent of the delta implementation". That merits a fair amount of weight in how we decide to structure things. Added an issue to track this: #2623

internal/core/delta.go

internal/merkle/crdt/merklecrdt.go

net/dag.go

jsimnz · 2024-05-15T08:04:31Z

net/server_test.go

@@ -247,6 +250,27 @@ func TestDocQueue(t *testing.T) {
 	q.mu.Unlock()
 }

+func getHead(ctx context.Context, db client.DB, docID client.DocID) (cid.Cid, error) {


todo: We shouldn't have a duplicate getHead function, with its own implementation, since there is already all this functionality within the merkle/clock package to be able to get a specific head CID of a document.

Its not the nicest to use, but if there is going to be some util function that takes a store and a docID and returns the head CID, it should be along side the current implementation in the other package.

Note: We also probably don't need the updated Headstore method on the client.DB interface, since the datastore.Multistore already has this method, and the entire client.DB interface shouldn't be a argument to this function just to get a head CID of a document.

net package test code should not be referencing the internal/merkle/clock package. If I had to chose between duplicating code and referencing an internal method, I would overwhelmingly support duplicating the code (or just don't bother and delete the unit tests...).

There are a few problems here. One is like Andy mentioned, needing to deal with internal/merkle/clock for net tests is not ideal. Also, before this PR, the Headstore was only available through MultiStore with an explicit txn which those test don't use. Saying the entire client.DB shouldn't be an argument to this function is correct and that is something I want to fix in a lot of places in our codebase. We often use large interfaces when a locally specific one would be more appropriate.

I'd rather not get into this within this PR but I will try to improve the Headstore utilities shortly.

Issue created: #2621

Usage within a test or not doesn't make much difference here. This function is reimplementing a somewhat sensitive flow and is relying on the behavior of getHead to be inline with the DB.

If somewhat makes changes to the actual headset or head store for whatever reason, it's not necessarily going to be clear when they run the tests and the net package tests are failing.

The fact that the merkleclock is internal or not also has no bearing.

We have a specific implementation, that is rather core to the entire correctness of the DB with the headset implementation, and that behavior must be consistent.

We agree that it needs to be consistant. This will be fixed in a subsequent PR.

Yupp perfect

(Note my comment was primarily in response to that it's a test and it's ok to duplicate the functionality, not the need for it to be in the pr)

Cheers

jsimnz

Lgtm thanks for extracting the follow up items from my review

fredcarle added area/crdt Related to the (Merkle) CRDT system refactor This issue specific to or requires *notable* refactoring of existing codebases and components labels May 9, 2024

fredcarle added this to the DefraDB v0.12 milestone May 9, 2024

fredcarle requested a review from a team May 9, 2024 02:16

fredcarle self-assigned this May 9, 2024

fredcarle force-pushed the fredcarle/refactor/ipld-protobuf-to-cbor branch 4 times, most recently from e08fe9a to e9a6d3a Compare May 9, 2024 15:46

AndrewSisley approved these changes May 9, 2024

View reviewed changes

fredcarle force-pushed the fredcarle/refactor/ipld-protobuf-to-cbor branch from e9a6d3a to 6f7d5c0 Compare May 10, 2024 21:26

fredcarle requested a review from AndrewSisley May 10, 2024 21:26

AndrewSisley approved these changes May 13, 2024

View reviewed changes

jsimnz requested changes May 15, 2024

View reviewed changes

fredcarle added 8 commits May 15, 2024 14:27

change ipld protobuf to cbor

77081e9

add breaking change documentation

8d4a391

fix commit delta type

921ce47

apply feedback

98b81e7

improve validation of IPLD schemas

bb1fedf

fix copy pasted sort.

523c627

switch to constructor

62a832a

change comment

1cf1e1f

fredcarle mentioned this pull request May 15, 2024

Explore adding version number to IPLD blocks #2622

Open

fredcarle force-pushed the fredcarle/refactor/ipld-protobuf-to-cbor branch from 7ef1e57 to 54a3f05 Compare May 15, 2024 19:39

fredcarle mentioned this pull request May 15, 2024

Explore making IPLD schema definitions their own thing. #2623

Open

fredcarle added 2 commits May 15, 2024 16:54

apply feedback

fec8e31

bypass processRemoteBlock in dagworker

affd3a9

fredcarle force-pushed the fredcarle/refactor/ipld-protobuf-to-cbor branch from 54a3f05 to affd3a9 Compare May 15, 2024 21:10

fredcarle requested a review from jsimnz May 15, 2024 21:11

jsimnz approved these changes May 15, 2024

View reviewed changes

fredcarle merged commit 683f78c into sourcenetwork:develop May 16, 2024
29 of 30 checks passed

fredcarle deleted the fredcarle/refactor/ipld-protobuf-to-cbor branch May 16, 2024 03:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Change from protobuf to cbor for IPLD #2604

refactor: Change from protobuf to cbor for IPLD #2604

fredcarle commented May 9, 2024 •

edited

Loading

AndrewSisley left a comment

AndrewSisley May 9, 2024

fredcarle May 10, 2024

AndrewSisley May 9, 2024

fredcarle May 10, 2024

jsimnz commented May 10, 2024

AndrewSisley left a comment

AndrewSisley May 13, 2024

fredcarle May 13, 2024

jsimnz left a comment

jsimnz May 10, 2024

jsimnz May 15, 2024

nasdf May 15, 2024

fredcarle May 15, 2024

jsimnz May 15, 2024 •

edited

Loading

fredcarle May 15, 2024

AndrewSisley May 15, 2024

fredcarle May 15, 2024

jsimnz May 15, 2024 •

edited

Loading

fredcarle May 15, 2024

jsimnz May 15, 2024

fredcarle May 15, 2024

jsimnz May 15, 2024

jsimnz May 15, 2024

AndrewSisley May 15, 2024 •

edited

Loading

fredcarle May 15, 2024

fredcarle May 15, 2024

jsimnz May 15, 2024

fredcarle May 15, 2024

jsimnz May 15, 2024

jsimnz left a comment

refactor: Change from protobuf to cbor for IPLD #2604

refactor: Change from protobuf to cbor for IPLD #2604

Conversation

fredcarle commented May 9, 2024 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimnz commented May 10, 2024

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimnz May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimnz May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

fredcarle commented May 9, 2024 •

edited

Loading

jsimnz May 15, 2024 •

edited

Loading

jsimnz May 15, 2024 •

edited

Loading

AndrewSisley May 15, 2024 •

edited

Loading