feat(ProcessProposal)!: Check for message inclusion using subtree roots #747
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
evan-forbes
added
C: Celestia app
consensus breaking
evan-forbes
requested review from
adlerjohn,
rootulp,
mojtaba-esk and
rach-id
|
still have to rebase to fix the git history, so leaving as a draft |
… used to. The current way causes bugs as its not doing what we thought it was.
This reverts commit 9e01bd3.
evan-forbes
force-pushed
the
evan/NID-ProcessProposalRefactor
branch
from
b127e43 to
555c436
Compare
Codecov Report
@@ Coverage Diff @@
## main #747 +/- ##
==========================================
- Coverage 42.16% 42.07% -0.10%
==========================================
Files 42 42
Lines 4383 4383
==========================================
- Hits 1848 1844 -4
- Misses 2387 2390 +3
- Partials 148 149 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
evan-forbes
commented
Sep 20, 2022
Comment on lines
-153
to
+160
| tree := wrapper.NewErasuredNamespacedMerkleTree(appconsts.MaxSquareSize) | ||
| // create the nmt todo(evan) use nmt wrapper | ||
| tree := nmt.New(sha256.New()) | ||
| for _, leaf := range set { | ||
| nsLeaf := append(make([]byte, 0), append(namespace, leaf...)...) | ||
| // here we hardcode pushing as axis 0 cell 0 because we never want | ||
| // to add the parity namespace to our shares when we create roots. | ||
| tree.Push(nsLeaf, rsmt2d.SquareIndex{Axis: 0, Cell: 0}) | ||
| err := tree.Push(nsLeaf) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
had to revert this change from #618 to get the commitments to match, which was really weird. I'm still unsure why this is and requires further debugging. We are using the nmt wrapper to create row and col roots, so it only makes sense to use them when we create commitment so that they match.
Comment on lines
+12
to
+14
| func DelimLen(size uint64) int { | ||
| lenBuf := make([]byte, binary.MaxVarintLen64) | ||
| return binary.PutUvarint(lenBuf, size) |
There was a problem hiding this comment.
reverted this change to calculate the delim len how we used to, as the new way wasn't doing what we thought. We still need to add a unit test to make sure we don't make the same mistake again. This is implicitly tested in the new fuzzer added in this PR, and message inclusion checks.
rootulp
previously approved these changes
Sep 21, 2022
Co-authored-by: Rootul P <[email protected]>
rootulp
approved these changes
Sep 21, 2022
This was referenced Sep 28, 2022
evan-forbes
added a commit
that referenced
this pull request
Sep 28, 2022
as noticed in #804 (comment), I forgot to delete the old `DelimLen` function after fixing a bug in #747.
rach-id
pushed a commit
to rach-id/celestia-app
that referenced
this pull request
Nov 16, 2022
…ts (celestiaorg#747) Co-authored-by: Rootul P <[email protected]>
rach-id
pushed a commit
to rach-id/celestia-app
that referenced
this pull request
Nov 16, 2022
as noticed in celestiaorg#804 (comment), I forgot to delete the old `DelimLen` function after fixing a bug in celestiaorg#747.
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.
This PR updates
ProcessProposalto use subtree roots to check for message inclusion, instead of only relying on comparing the commitments.This is last non-interactive defaults PR 🎉
closes #383