Commit to the extended data square #540

evan-forbes · 2021-09-22T00:02:42Z

Description

This PR generates the DataHash using erasured block data. Instead of creating a new method, like what is done using fillDataAvailabilityHeader in the current celestia-core master, this PR simply modifies the prexisting Hash method of Data. This approach has the benefit of being slightly closer to tendermint in that we have to change less lines of code, along with being a convenient way to not include the DataAvailabilityHeader inside of Block

Closes: #512
Part of #528
blocked by #537 and #539

* 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

* Export block data compute shares. * Refactor to use ShareSize constant directly. * Change message splitting to prefix namespace ID. * Implement chunking for contiguous. * Add termination condition. * Rename append contiguous to split contiguous. * Update test for small tx. * Add test for two contiguous. * Make tx and msg adjusted share sizes exported constants. * Panic on hopefully-unreachable condition instead of silently skipping. * Update hardcoded response for block format. Co-authored-by: Ismail Khoffi <[email protected]>

* fix overwrite bug and stop splitting shares of size MsgShareSize * remove ineffectual code * review feedback: better docs Co-authored-by: Ismail Khoffi <[email protected]> * remove uneeded copy and only fix the source of the bug Co-authored-by: Ismail Khoffi <[email protected]> * fix overwrite bug while also being consistent with using NamespacedShares * update to the latest rsmt2d for the nmt wrapper Co-authored-by: Ismail Khoffi <[email protected]>

* start spec compliant share merging * refactor and finish unit testing * whoops * linter gods * fix initial changes and use constants * use constant * more polish * docs fix* review feedback: docs and out of range panic protection * review feedback: add panic protection from empty input * use constant instead of recalculating `ShareSize`* don't redeclare existing var* be more explicit with returned nil* use constant instead of recalculating `ShareSize`* review feedback: use consistent capitalization * stop accepting reserved namespaces as normal messages * use a descriptive var name for message length * linter and comparison fix * reorg tests, add test for parse delimiter, DataFromBlock and fix evidence marshal bug * catch error for linter * update test MakeShares to include length delimiters for the SHARE_RESERVED_BYTE * minor iteration change * refactor share splitting to fix bug * fix all bugs with third and final refactor * fix conflict * revert unnecessary changes * review feedback: better docs* reivew feedback: add comment for safeLen * review feedback: remove unnecessay comments * review feedback: split up share merging and splitting into their own files * review feedback: more descriptive var names * fix accidental change * add some constant docs * spelling error Co-authored-by: Hlib Kanunnikov <[email protected]> Co-authored-by: John Adler <[email protected]> Co-authored-by: Ismail Khoffi <[email protected]>

Co-authored-by: rene <[email protected]>

* version docs * stash commits * fix url * remove master from sidebar * fix build errors * move to consensus connection * add process * add proof to block * add block metadata in beginBlcok * fix testing * regenerate mocks * add in needed proto changes * regenerate mocks * fix makeblock calls * Apply suggestions from code review * fix rpc tests * fix linting and ci errors * fix consensus tests * add preprocess to e2e * add preprocess to counter app * replace meta_data with messages * fix linting * Update state/execution.go Co-authored-by: Ismail Khoffi <[email protected]> * fix comment * fix e2e tests Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Ismail Khoffi <[email protected]>

evan-forbes · 2021-09-22T00:04:24Z

types/block.go

+ // todo(evan): add the non redundant shares back into the header
+ shares, _ := data.ComputeShares()


this is personal note to properly write an issue to handle the second returned value from ComputeShares and include it in the header. I think it should be handled in a different PR tho, as it an optimization. That being said, if anyone thinks we should handle this now, we can do that too 🙂

evan-forbes · 2021-09-22T00:06:57Z

types/block.go

+ eds, err := da.ExtendShares(squareSize, rawShares)
+ if err != nil {
+ panic(err)
 }


we panicked before in fillDataAvailabilityHeader, but I think this time it might be worse, as ExtendShares will return an error if the square size is too large!

This is another personal note to properly write an issue to document this, as it will definitely have to be protected against by the transaction reaping from the mempool mechanism if we don't handle the error straight up. ref #77

Cherry pick PreprocessTxs

…mit-to-extended-data

evan-forbes · 2021-09-22T19:10:55Z

All the tests are passing for me locally, and the CI is showing some weird errors on all the "failing" tests.

Error: command [git diff] exited with code 128. message: fatal: ambiguous argument 'get-diff-action/evan/cherry-pick-share-merging-splitting...get-diff-action/pull/540/merge': unknown revision or path not in the working tree.```

liamsi · 2021-09-22T22:20:43Z

Sounds like this is a github-action only problem? Maybe it's just caching? Did you try restarting the process manually?

liamsi · 2021-09-22T22:32:31Z

What I would try to make sure this isn't some super weird cache issue, I'd create a new branch and see if the error still happens there. This issue sounds related: technote-space/get-diff-action#164

evan-forbes · 2021-09-22T22:44:43Z

Thanks for helping me debug, I tried to debug quite a few times, not sure why it started working now. I'll merge this as soon as the CI passes.

liamsi and others added 15 commits September 20, 2021 17:53

clean up some unused test helper functions

434499c

linter

0f2489e

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

c65d555

refactor to better accomodate real world use cases (celestia node)

5ee5b96

Co-authored-by: rene <[email protected]>

thank you linter

422eb4d

commit to extended data instead of just the hash of the txs

e4d922b

fix test so that it doesn't modify block data after generating the hash

1aaa2dd

fix missed make block api change

05b6281

run make mockery

7eb51b1

add some docs

abdd192

evan-forbes commented Sep 22, 2021

View reviewed changes

evan-forbes mentioned this pull request Sep 22, 2021

Cherry pick PreprocessTxs #541

Merged

fix hardcoded test

c19690a

evan-forbes self-assigned this Sep 22, 2021

Merge pull request #541 from celestiaorg/evan/cherry-pick-preprocesstxs

f8775ef

Cherry pick PreprocessTxs

evan-forbes force-pushed the evan/erasured-data-hash-and-dah-creation-refactor branch from 422eb4d to ee4a20b Compare September 22, 2021 16:45

Base automatically changed from evan/erasured-data-hash-and-dah-creation-refactor to evan/merge-theirs September 22, 2021 17:16

evan-forbes changed the base branch from evan/merge-theirs to evan/cherry-pick-share-merging-splitting September 22, 2021 17:28

evan-forbes added 3 commits September 22, 2021 12:47

Merge branch 'evan/cherry-pick-share-merging-splitting' into evan/com…

99d44ca

…mit-to-extended-data

fix linter

bc00ed9

fix hard coded test

17720e8

Base automatically changed from evan/cherry-pick-share-merging-splitting to evan/merge-theirs September 22, 2021 18:14

evan-forbes mentioned this pull request Sep 22, 2021

Evan/commit to extended data2 #544

Closed

evan-forbes merged commit 14431e1 into evan/merge-theirs Sep 22, 2021

evan-forbes deleted the evan/commit-to-extended-data branch September 22, 2021 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Commit to the extended data square #540

Commit to the extended data square #540

evan-forbes commented Sep 22, 2021

evan-forbes Sep 22, 2021

evan-forbes Sep 22, 2021 •

edited

Loading

evan-forbes commented Sep 22, 2021

liamsi commented Sep 22, 2021

liamsi commented Sep 22, 2021

evan-forbes commented Sep 22, 2021

		// todo(evan): add the non redundant shares back into the header
		shares, _ := data.ComputeShares()

Commit to the extended data square #540

Commit to the extended data square #540

Conversation

evan-forbes commented Sep 22, 2021

Description

evan-forbes Sep 22, 2021

Choose a reason for hiding this comment

evan-forbes Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

evan-forbes commented Sep 22, 2021

liamsi commented Sep 22, 2021

liamsi commented Sep 22, 2021

evan-forbes commented Sep 22, 2021

evan-forbes Sep 22, 2021 •

edited

Loading