Commit to the extended data square #540
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* 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
commented
Sep 22, 2021
Comment on lines
+1046
to
+1047
| // todo(evan): add the non redundant shares back into the header | ||
| shares, _ := data.ComputeShares() |
There was a problem hiding this comment.
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 🙂
Comment on lines
+1052
to
1055
| eds, err := da.ExtendShares(squareSize, rawShares) | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
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
evan-forbes
force-pushed
the
evan/erasured-data-hash-and-dah-creation-refactor
branch
from
422eb4d to
ee4a20b
Compare
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
Base automatically changed from
evan/cherry-pick-share-merging-splitting
to
evan/merge-theirs
September 22, 2021 18:14
|
All the tests are passing for me locally, and the CI is showing some weird errors on all the "failing" tests. |
|
Sounds like this is a github-action only problem? Maybe it's just caching? Did you try restarting the process manually? |
|
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 |
|
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR generates the
DataHashusing erasured block data. Instead of creating a new method, like what is done usingfillDataAvailabilityHeaderin the current celestia-core master, this PR simply modifies the prexistingHashmethod ofData. 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 theDataAvailabilityHeaderinside ofBlockCloses: #512
Part of #528
blocked by #537 and #539