Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cherry pick basic DA functionality from celestia-core #536

Merged
merged 4 commits into from
Sep 21, 2021

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Sep 20, 2021

Description

Mainly cherry picks #83, with a few modifications to better fit in a newer version of tendermint, and this PR doesn't add the data availability header to the block.

part of #528

liamsi and others added 2 commits September 20, 2021 17:53
* move Messages field to the end of Block.Data

* Add some constants for share computation and the NMT:

 - also a bunch of todos regarding shares computation

* First (compiling) stab on creating shares

* Test with Evidence and fix bug discovered by test

* remove resolved todos

* introduce split method

* Introduce LenDelimitedMarshaler interface and some reformatting

* Introduce TxLenDelimitedMarshaler

* add some test cases

* fix some comments

* fix some comments & linter

* Add reserved namespaces to params

* Move ll-specific consts into a separate file (consts.go)

* Add MarshalDelimited to HexBytes

* Add tail-padding shares

* Add ComputeShares method on Data to compute all shares

* Fix compute the next square num and not the next power of two

* lints

* Unexport MakeShares function:

- it's likely to change and it doesn't have to be part of the public API

* lints 2

* First stab on computing row/column roots

* fix rebase glitches:
 - move DA related constants out of params.go

* refactor MakeBlock to take in interm. state roots and messages

* refactor state.MakeBlock too

* Add todos LenDelimitedMarshaler and extract appendShares logic

* Simplify shares computation: remove LenDelimitedMarshaler abstraction

* actually use DA header to compute the DataRoot everywhere (will lead to failing tests for sure)

* WIP: Update block related core data structures in protobuf too

* WIP: fix zero shares edge-case and get rid of Block.Data.hash (use dataAvailabilityHeader.Hash() instead)

* Fixed tests, only 3 failing tests to go: TestReapMaxBytesMaxGas, TestTxFilter, TestMempoolFilters

* Fix TestTxFilter:

 - the size of the wrapping Data{} proto message increased a few bytes

* Fix Message proto and `DataFromProto`

* Fix last 2 remaining tests related to the increased block/block.Data size

* Use infectious lib instead of leopard

* proto-lint: snake_case

* some lints and minor changes

* linter

* panic if pushing to tree fails, extend Data.ToProto()

* revert renaming in comment

* add todo about refactoring as soon as the rsmt2d allows the user to choose the merkle tree
Comment on lines -126 to +128
{20, 240, -1, 10},
{20, 240, 10, 10},
{20, 240, 15, 10},
{20, 280, -1, 10},
{20, 280, 10, 10},
{20, 280, 15, 10},
Copy link
Member Author

Choose a reason for hiding this comment

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

this is due to the increased size of proto Data

see this func

celestia-core/types/tx.go

Lines 154 to 160 in 8ea76d0

// ComputeProtoSizeForTxs wraps the transactions in tmproto.Data{} and calculates the size.
// https://developers.google.com/protocol-buffers/docs/encoding
func ComputeProtoSizeForTxs(txs []Tx) int64 {
data := Data{Txs: txs}
pdData := data.ToProto()
return int64(pdData.Size())
}

Comment on lines +112 to +121
// // fillDataAvailabilityHeader fills in any remaining DataAvailabilityHeader fields
// // that are a function of the block data.
// func (b *Block) fillDataAvailabilityHeader() {
// namespacedShares := b.Data.computeShares()
// shares := namespacedShares.RawShares()
// if len(shares) == 0 {
// // no shares -> no row/colum roots -> hash(empty)
// b.DataHash = b.DataAvailabilityHeader.Hash()
// return
// }
Copy link
Member Author

Choose a reason for hiding this comment

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

planning to do this in a different PR, as share splitting and merging isn't added yet

Comment on lines 48 to +50
Header `json:"header"`
Data `json:"data"`
Evidence EvidenceData `json:"evidence"`
LastCommit *Commit `json:"last_commit"`
LastCommit *Commit `json:"last_commit"`
Copy link
Member Author

@evan-forbes evan-forbes Sep 20, 2021

Choose a reason for hiding this comment

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

note that the data availability header is not added to the block, unlike in #83. Instead, we can simply generate the DAH and data hash, then discard it.

If we want to save it for whatever reason, then we can add it seperately to BlockMeta

ref #512

@evan-forbes evan-forbes changed the title Cherry pick basic DA func Cherry pick basic DA functionality from celestia-core Sep 20, 2021
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

👍🏼

@liamsi
Copy link
Member

liamsi commented Sep 21, 2021

Will these be squashed or merged btw?

@evan-forbes evan-forbes merged commit 6e592de into evan/merge-theirs Sep 21, 2021
@evan-forbes evan-forbes deleted the evan/cherry-pick-basic-DA-func branch September 21, 2021 16:27
@evan-forbes evan-forbes self-assigned this Sep 21, 2021
liamsi added a commit that referenced this pull request Sep 23, 2021
* Basic DA functionality  (#83)

* move Messages field to the end of Block.Data

* Add some constants for share computation and the NMT:

 - also a bunch of todos regarding shares computation

* First (compiling) stab on creating shares

* Test with Evidence and fix bug discovered by test

* remove resolved todos

* introduce split method

* Introduce LenDelimitedMarshaler interface and some reformatting

* Introduce TxLenDelimitedMarshaler

* add some test cases

* fix some comments

* fix some comments & linter

* Add reserved namespaces to params

* Move ll-specific consts into a separate file (consts.go)

* Add MarshalDelimited to HexBytes

* Add tail-padding shares

* Add ComputeShares method on Data to compute all shares

* Fix compute the next square num and not the next power of two

* lints

* Unexport MakeShares function:

- it's likely to change and it doesn't have to be part of the public API

* lints 2

* First stab on computing row/column roots

* fix rebase glitches:
 - move DA related constants out of params.go

* refactor MakeBlock to take in interm. state roots and messages

* refactor state.MakeBlock too

* Add todos LenDelimitedMarshaler and extract appendShares logic

* Simplify shares computation: remove LenDelimitedMarshaler abstraction

* actually use DA header to compute the DataRoot everywhere (will lead to failing tests for sure)

* WIP: Update block related core data structures in protobuf too

* WIP: fix zero shares edge-case and get rid of Block.Data.hash (use dataAvailabilityHeader.Hash() instead)

* Fixed tests, only 3 failing tests to go: TestReapMaxBytesMaxGas, TestTxFilter, TestMempoolFilters

* Fix TestTxFilter:

 - the size of the wrapping Data{} proto message increased a few bytes

* Fix Message proto and `DataFromProto`

* Fix last 2 remaining tests related to the increased block/block.Data size

* Use infectious lib instead of leopard

* proto-lint: snake_case

* some lints and minor changes

* linter

* panic if pushing to tree fails, extend Data.ToProto()

* revert renaming in comment

* add todo about refactoring as soon as the rsmt2d allows the user to choose the merkle tree

* clean up some unused test helper functions

* linter

* still debugging the exact right number of bytes for max data...

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>
evan-forbes pushed a commit that referenced this pull request Jun 9, 2023
* removing non updated java and kotlin guides

* upgradeing guides order

* removing stale java and kotlin guides
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants