feat: add support for Blobstream API #3470

rach-id · 2024-06-05T11:17:00Z

rach-id · 2024-06-05T11:18:21Z

go.mod

 	github.com/celestiaorg/go-fraud v0.2.1
 	github.com/celestiaorg/go-header v0.6.1
 	github.com/celestiaorg/go-libp2p-messenger v0.2.0
-	github.com/celestiaorg/nmt v0.21.0
+	github.com/celestiaorg/nmt v0.21.1-0.20240602221058-a81b748b6f51 // TODO replace with release when ready


will be replaced once we release nmt

rach-id · 2024-06-05T11:19:18Z

nodebuilder/blobstream/service.go

+}
+
+// ProveShares generates a share proof for a share range.
+// Note: queries the whole EDS to generate the proof.


should I create an issue for this?

nodebuilder/blobstream/service.go

rach-id · 2024-06-05T11:19:55Z

nodebuilder/blobstream/service.go

+	var subtreeRoots [][]byte
+	var dataCursor int
+	for _, proof := range nmtProofs {
+		// TODO: do we want directly use the default subtree root threshold or want to allow specifying which version to use?


[note to reviewers]
do we want directly use the default subtree root threshold or want to allow specifying which version to use?

rach-id · 2024-06-05T11:20:53Z

nodebuilder/blobstream/service.go

+// Note: this method is not part of the API. It will not be served by any endpoint, however,
+// it can be called directly programmatically.
+func ProveSubtreeRootToCommitment(subtreeRoots [][]byte, subtreeRootIndex uint64) (*ResultSubtreeRootToCommitmentProof, error) {
+	_, proofs := merkle.ProofsFromByteSlices(subtreeRoots)
+	return &ResultSubtreeRootToCommitmentProof{
+		SubtreeRootToCommitmentProof: SubtreeRootToCommitmentProof{
+			Proof: *proofs[subtreeRootIndex],
+		},
+	}, nil
+}
+
+// ProveShareToSubtreeRoot generates a share to subtree root inclusion proof
+// Note: this method is not part of the API. It will not be served by any endpoint, however,
+// it can be called directly programmatically.
+func ProveShareToSubtreeRoot(shares [][]byte, shareIndex uint64) (*ResultShareToSubtreeRootProof, error) {
+	_, proofs := merkle.ProofsFromByteSlices(shares)
+	return &ResultShareToSubtreeRootProof{
+		ShareToSubtreeRootProof: ShareToSubtreeRootProof{
+			Proof: *proofs[shareIndex],
+		},
+	}, nil


[note to reviewers]
these two methods are not used in the API but I see them as useful for people using the API. should we keep them here? move them somewhere special? create a utils.go or helpers.go and put them there?

rach-id · 2024-06-05T11:22:01Z

nodebuilder/blobstream/service_test.go

+		})
+	}
+}
+


instead of using the generated mock. I decided to implement a custom mock here so that I can test a lot of combinations without having to worry about the mocked p2p network etc.

If you check the TestProveCommitmentAllCombinations test, you will see that it's kind of fuzzing tons of combinations.

nodebuilder/blobstream/types.go

nodebuilder/blobstream/service.go

# Conflicts: # go.mod # go.sum

…--api

codecov-commenter · 2024-06-05T12:12:54Z

Codecov Report

Attention: Patch coverage is 64.69388% with 173 lines in your changes missing coverage. Please review.

Project coverage is 45.30%. Comparing base (2469e7a) to head (f8a3066).
Report is 106 commits behind head on main.

Files	Patch %	Lines
nodebuilder/blobstream/service.go	77.87%	42 Missing and 37 partials ⚠️
nodebuilder/blobstream/types.go	37.87%	32 Missing and 9 partials ⚠️
nodebuilder/blobstream/mocks/api.go	9.52%	38 Missing ⚠️
nodebuilder/blobstream/blobstream.go	0.00%	8 Missing ⚠️
nodebuilder/blobstream/module.go	0.00%	6 Missing ⚠️
nodebuilder/rpc/constructors.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3470      +/-   ##
==========================================
+ Coverage   44.83%   45.30%   +0.47%     
==========================================
  Files         265      278      +13     
  Lines       14620    15851    +1231     
==========================================
+ Hits         6555     7182     +627     
- Misses       7313     7828     +515     
- Partials      752      841      +89

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nodebuilder/blobstream/blobstream.go

insiderbyte

Nodebuilder folder is using only for the modules creation. Maybe you should add a blobstream package and move all logic there?

nodebuilder/blobstream/blobstream.go

nodebuilder/blobstream/types.go

ramin · 2024-06-18T12:20:15Z

@renaynay raised a good point i agree with that choosing a more generic name for this module and API, something other than "blobstream", might be better. As although it is for the purpose of supporting blobstream initially, it can and should serve a generic purpose.

Any suggestions for better names @renaynay @adlerjohn @liamsi?

@vgonkivs also mentioned potentially moving it into the blob module itself i believe

liamsi · 2024-06-18T13:26:01Z

Any suggestions for better names @renaynay @adlerjohn @liamsi?

Yeah, I also raised this somewhere else. So ideally:

anything shares related would be folded into the shares "module"
hence, ProveShares and ProveCommitment could be moved into the shares module
even better: shares.GetProof would actually accommodate ProveShares
GetDataCommitment and DataRootInclusionProof are really very blobstream specific but they could live in a "blocks" module as they operate over a range of blocks/block heights. The closest we have for this is the header module.

distractedm1nd

Looks great so far, I really appreciate the extensive debug logging and explicit error handling everywhere.

My comments are all about the representation of the responses and an alternative, more golang idiomatic/node-native way to structure the types in the package

Please also add example types in api/docgen/examples.go

nodebuilder/blobstream/blobstream.go

api/rpc_test.go

api/rpc/client/client.go

nodebuilder/blobstream/types.go

distractedm1nd · 2024-06-18T13:28:27Z

nodebuilder/blobstream/types.go

+// ResultDataCommitment is the API response containing a data
+// commitment, aka data root tuple root.
+type ResultDataCommitment struct {
+	DataCommitment bytes.HexBytes `json:"data_commitment"`
+}
+
+// ResultDataRootInclusionProof is the API response containing the binary merkle
+// inclusion proof of a height to a data commitment.
+type ResultDataRootInclusionProof struct {
+	Proof merkle.Proof `json:"proof"`
+}
+
+// ResultShareProof is the API response that contains a ShareProof.
+// A share proof is a proof of a set of shares to the data root.
+type ResultShareProof struct {
+	ShareProof types.ShareProof `json:"share_proof"`
+}
+
+// ResultCommitmentProof is an API response that contains a CommitmentProof.
+// A commitment proof is a proof of a blob share commitment to the data root.
+type ResultCommitmentProof struct {
+	CommitmentProof CommitmentProof `json:"commitment_proof"`
+}


I don't think we should wrap these types in a Result struct if there are no methods defined on them.

What does make sense is to define type aliases, so the API return types are clear, and docs can be kept. Future methods on these types will also be cleaner if the aliases are already defined.

So instead of ResultDataCommitment, it would be something like

// DataCommitment is the API response containing a data // commitment, aka data root tuple root. type DataCommitment bytes.HexBytes

And the response will be read out directly from result instead of by accessing an inner JSON field

I am wrapping the types so that if we decided in the future to add new fields to the struct, we can do it in a backwards compatible way. Also, this is the way it is done in core: https://github.com/celestiaorg/celestia-core/blob/ff2bff4a29aec6851499e34b7d84349d8e774dd9/rpc/core/types/responses.go#L52-L55

Do you still prefer if I unwrap them?

Hlib and I discussed this shortly, and we decided we would still prefer them to be unwrapped - we think backwards compatibility is liekly overkill here, or do you have a good example of something that may be added?

My thinking here is that these are actually pretty much low-level types that are processed directly in the smart contracts (as far as I imagine? haven't read them) and that they won't be doing any parsing out of extra additional data

Agreed, but I guess the same reasoning lead to having the blob proof to be []*nmt.Proof and ended up needing to be a breaking change to change it to contain a proof to the data root.

From my experience, it's always good to separate the problematical types from the API ones. But I am fine with any. Let me know if you prefer them to be unwrapped and I'll do it

i think consensus it to unwrap please @rach-id. @distractedm1nd and @Wondertan do you agree?

cool, I'll unwrap as soon as I get confirmation.

nodebuilder/blobstream/service.go

distractedm1nd · 2024-06-18T14:54:08Z

nodebuilder/blobstream/service.go

+// DataRootTuple contains the data that will be used to create the QGB commitments.
+// The commitments will be signed by orchestrators and submitted to an EVM chain via a relayer.
+// For more information: https://github.com/celestiaorg/quantum-gravity-bridge/blob/master/src/DataRootTuple.sol
+type DataRootTuple struct {


Related to my other comments, wdyt about ~~returning DataRootTuple directly~~, and then implementing:

MarshalBinary
UnmarshalBinary
~~MarshalJSON (just outputs the base64 of MarshalBinary output for example)~~
~~UnmarshalJSON~~

Then you won't need EncodeDataRootTuple I believe? ~~And marshalling for the RPC ouput is handled automatically by fulfilling the interface.~~

Ah I see, I misinterpreted the original godoc of ResultDataCommitment. So GetDataCommitment should only return the datahash part

Fulfilling the encoding/binary interface is still probably a more idiomatic way to do this though

what does this mean? what needs to be changed?

This means implementing MarshalBinary and UnmarshalBinary onto DataRootTuple to fulfill the encoding/binary interface, to replace EncodeDataRootTuple. I can gladly help out here or send a diff if it helps!

But we only need to encode it. do you mean fulfilling the interface and making the unmarshalling a no-op?

nodebuilder/blobstream/service.go

@vgonkivs

…ed by @vgonkivs

rach-id · 2024-06-21T11:02:02Z

Nodebuilder folder is using only for the modules creation. Maybe you should add a blobstream package and move all logic there?

@insiderbyte where do you mean I should put the blobstream package? in the root?

ramin · 2024-07-01T11:12:09Z

@rach-id so there is a general desire to follow @liamsi 's comment about decomposing this, such that instead of "blobstream" being a whole module, the new API functionality lives in each of their relative existing modules, with some functionality moved to another package (like the "blocks" one that he also suggested). There was a promise that everyone would chime in on reviews and suggestions of where to place things, I'll chase that - but we can work on together too if you want to suggest a diff layout...

rach-id · 2024-07-01T11:32:52Z

@ramin Got you 👍 I was under the impression that this PR should be merged, and then we do the refactor. However, if we want to do it here, I'm fine with that.

The proposed structure would be:

header module:
- GetDataCommitment
- GetDataRootInclusionProof which should maybe be renamed to GetDataRootTupleInclusionProof because we're proving the data root tuple, i.e. (height, data_root) and not just the data root
blob module:
- ProveCommitment: or we replace in this PR even GetProof to return the correct proof instead of the current one. Discussed in here: feat: add support for Blobstream API #3470 (comment)
share module:
- ProveShares

If everyone agrees, I can proceed with the refactor.

liamsi · 2024-07-01T13:57:44Z

I like header better than block.

distractedm1nd · 2024-07-02T10:56:35Z

agreed with proposed structure

rach-id added 4 commits June 4, 2024 19:49

feat: add support for the Blobstream API in node

dda88ee

chore: add permissions to methods

41bfd50

fix: use the share proofs to generate the commitment proofs

a3a32f3

chore: gofumpt

e5d2e1d

rach-id requested review from renaynay, Wondertan, vgonkivs, distractedm1nd, walldiss, ramin and cristaloleg as code owners June 5, 2024 11:17

rach-id requested a review from evan-forbes June 5, 2024 11:17

github-actions bot added the external Issues created by non node team members label Jun 5, 2024

rach-id marked this pull request as draft June 5, 2024 11:17

chore: docs

cb6f660

rach-id commented Jun 5, 2024

View reviewed changes

Update nodebuilder/blobstream/service.go

8220364

rach-id self-assigned this Jun 5, 2024

rach-id added 3 commits June 5, 2024 15:26

Merge branch 'main' into blobstream--api

4309614

# Conflicts: # go.mod # go.sum

chore: go mod tidy

15b59dd

Merge remote-tracking branch 'origin/blobstream--api' into blobstream…

8231ade

…--api

rach-id marked this pull request as ready for review June 5, 2024 11:27

rach-id added 5 commits June 5, 2024 15:45

chore: lint

d31ee83

chore: lint

a784be9

chore: gofumpt

4463b4a

chore: gofumpt

1953fc5

chore: gofumpt

eb0b29e

renaynay added the needs:triage label Jun 6, 2024

rach-id mentioned this pull request Jun 6, 2024

[Feature Request]: Comprehensive proof API #3473

Open

cristaloleg reviewed Jun 7, 2024

View reviewed changes

nodebuilder/blobstream/blobstream.go Outdated Show resolved Hide resolved

chore: internal struct definition inside API

f8a3066

renaynay added v0.15.0 Intended for v0.15.0 release and removed needs:triage labels Jun 13, 2024

insiderbyte suggested changes Jun 17, 2024

View reviewed changes

vgonkivs reviewed Jun 17, 2024

View reviewed changes

nodebuilder/blobstream/blobstream.go Outdated Show resolved Hide resolved

nodebuilder/blobstream/blobstream.go Outdated Show resolved Hide resolved

nodebuilder/blobstream/types.go Show resolved Hide resolved

vgonkivs added kind:feat Attached to feature PRs area:blobstream Related to the Blobstream feature and removed external Issues created by non node team members labels Jun 17, 2024

cristaloleg reviewed Jun 18, 2024

View reviewed changes

nodebuilder/blobstream/types.go Outdated Show resolved Hide resolved

distractedm1nd requested changes Jun 18, 2024

View reviewed changes

rach-id added 9 commits June 20, 2024 12:27

chore: fmt

a409722

chore: DataCommitment to GetDataCommitment as suggested by @vgonkivs

44f17a1

chore: DataCommitment to GetDataCommitment as suggested by @vgonkivs

080101a

chore: DataRootInclusionProof to GetDataRootInclusionProof as suggest…

5fc0203

…ed by @vgonkivs

chore: remove todo

350d950

chore: commitment proof pointer receiver

411fc20

fix: subtreeRootProofs instead of SubtreeRoots in error message

9e48e55

chore: new line

f24171e

chore: revert unnecessary changes

9b79dba

rach-id mentioned this pull request Jun 20, 2024

fix GetProof to return the blob proof #3516

Open

Update nodebuilder/blobstream/service.go

7db0ab9

renaynay removed the v0.15.0 Intended for v0.15.0 release label Jul 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for Blobstream API #3470

feat: add support for Blobstream API #3470

rach-id commented Jun 5, 2024

rach-id Jun 5, 2024

rach-id Jun 5, 2024

rach-id Jun 5, 2024

rach-id Jun 5, 2024

rach-id Jun 5, 2024

codecov-commenter commented Jun 5, 2024 •

edited

Loading

insiderbyte left a comment •

edited

Loading

ramin commented Jun 18, 2024

liamsi commented Jun 18, 2024

distractedm1nd left a comment

distractedm1nd Jun 18, 2024

rach-id Jun 20, 2024

distractedm1nd Jun 21, 2024

rach-id Jun 21, 2024

ramin Jul 1, 2024

rach-id Jul 1, 2024

distractedm1nd Jun 18, 2024 •

edited

Loading

distractedm1nd Jun 18, 2024

rach-id Jun 20, 2024

distractedm1nd Jun 21, 2024

rach-id Jun 21, 2024

rach-id commented Jun 21, 2024

ramin commented Jul 1, 2024

rach-id commented Jul 1, 2024

liamsi commented Jul 1, 2024

distractedm1nd commented Jul 2, 2024 •

edited

Loading

feat: add support for Blobstream API #3470

Are you sure you want to change the base?

feat: add support for Blobstream API #3470

Conversation

rach-id commented Jun 5, 2024

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

codecov-commenter commented Jun 5, 2024 • edited Loading

Codecov Report

insiderbyte left a comment • edited Loading

Choose a reason for hiding this comment

ramin commented Jun 18, 2024

liamsi commented Jun 18, 2024

distractedm1nd 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

distractedm1nd Jun 18, 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

rach-id commented Jun 21, 2024

ramin commented Jul 1, 2024

rach-id commented Jul 1, 2024

liamsi commented Jul 1, 2024

distractedm1nd commented Jul 2, 2024 • edited Loading

codecov-commenter commented Jun 5, 2024 •

edited

Loading

insiderbyte left a comment •

edited

Loading

distractedm1nd Jun 18, 2024 •

edited

Loading

distractedm1nd commented Jul 2, 2024 •

edited

Loading