-
Notifications
You must be signed in to change notification settings - Fork 278
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
feat!: square size independent message commitments #937
feat!: square size independent message commitments #937
Conversation
ac52881
to
c64ed25
Compare
app/test/fuzz_abci_test.go
Outdated
@@ -25,7 +25,7 @@ func TestFuzzPrepareProcessProposal(t *testing.T) { | |||
encConf := encoding.MakeConfig(app.ModuleEncodingRegisters...) | |||
signer := testutil.GenerateKeyringSigner(t, testAccName) | |||
testApp := testutil.SetupTestAppWithGenesisValSet(t) | |||
timer := time.After(time.Second * 30) | |||
timer := time.After(time.Second * 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
c64ed25
to
777b3c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work 🔥
Added couple questions, would defer to Evan for approval.
app/test/fuzz_abci_test.go
Outdated
@@ -25,7 +25,7 @@ func TestFuzzPrepareProcessProposal(t *testing.T) { | |||
encConf := encoding.MakeConfig(app.ModuleEncodingRegisters...) | |||
signer := testutil.GenerateKeyringSigner(t, testAccName) | |||
testApp := testutil.SetupTestAppWithGenesisValSet(t) | |||
timer := time.After(time.Second * 30) | |||
timer := time.After(time.Second * 10) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Sorry I can't comment on #937 (comment) but thanks for the tip! I reverted this test timeout decrease and changed my local VSCode go test timeout config. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
Deferring to Evan for final approval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had one non-blocking question and a non-blocking comment. I'm really impressed, I don't think this could have been broken up into a smaller chunk! really great work!!
- remove one type conversion by adopting generics - remove unnecessary check
Does this PR preserve the current non-interactive default rules? As mentioned in #941 (comment), the current non-interactive default rules may be required to ensure that message inclusion proofs remain O(log(n)). |
@musalbas good question and thanks for checking on this. This PR does not change the existing non-interactive defaults. This function and its usage remains unmodified https://github.com/rootulp/celestia-app/blob/671bc800acecfdeef426db580811fc0baeeda037/pkg/shares/non_interactive_defaults.go#L39-L80 only code relating to which subtree roots are used to calculate the commitment has been changed. |
Will merge this to the feature branch |
Implementation for square size independent message commitments. Note this targets `feat/square-size-independent-commitments` per #941 so that we can unblock review. <img src="https://user-images.githubusercontent.com/3699047/198996002-f20086af-187d-4ccf-adff-8f73b834152f.png" width="500">
If ADR 008 is adopted, only one message share commitment is signed so this PR renames a plural function to be singular. This is a follow-up to #937 and could've been included in that PR. Note: targets feature branch
Closes #941 ## Description This merges the feature branch `feat/square-size-independent-commitments` to `main`. The feature branch includes 3 already approved PRs: - #937 - #983 - #982 and two additional commits that haven't been reviewed yet to resolve the tasks below ## Tasks - [x] Merge in `main` and resolve merge conflicts caused by renames: - `payForData` => `payForBlob` - `x/payment` => `x/blob` - [x] Update ADR 008 to describe that it was implemented Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com> Co-authored-by: Matthew Sevey <mjsevey@gmail.com>
…elestiaorg#984) Closes celestiaorg#941 ## Description This merges the feature branch `feat/square-size-independent-commitments` to `main`. The feature branch includes 3 already approved PRs: - celestiaorg#937 - celestiaorg#983 - celestiaorg#982 and two additional commits that haven't been reviewed yet to resolve the tasks below ## Tasks - [x] Merge in `main` and resolve merge conflicts caused by renames: - `payForData` => `payForBlob` - `x/payment` => `x/blob` - [x] Update ADR 008 to describe that it was implemented Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com> Co-authored-by: Matthew Sevey <mjsevey@gmail.com>
…elestiaorg#984) Closes celestiaorg#941 ## Description This merges the feature branch `feat/square-size-independent-commitments` to `main`. The feature branch includes 3 already approved PRs: - celestiaorg#937 - celestiaorg#983 - celestiaorg#982 and two additional commits that haven't been reviewed yet to resolve the tasks below ## Tasks - [x] Merge in `main` and resolve merge conflicts caused by renames: - `payForData` => `payForBlob` - `x/payment` => `x/blob` - [x] Update ADR 008 to describe that it was implemented Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com> Co-authored-by: Matthew Sevey <mjsevey@gmail.com>
Closes celestiaorg/celestia-app#941 ## Description This merges the feature branch `feat/square-size-independent-commitments` to `main`. The feature branch includes 3 already approved PRs: - celestiaorg/celestia-app#937 - celestiaorg/celestia-app#983 - celestiaorg/celestia-app#982 and two additional commits that haven't been reviewed yet to resolve the tasks below ## Tasks - [x] Merge in `main` and resolve merge conflicts caused by renames: - `payForData` => `payForBlob` - `x/payment` => `x/blob` - [x] Update ADR 008 to describe that it was implemented Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com> Co-authored-by: Matthew Sevey <mjsevey@gmail.com>
Closes celestiaorg/celestia-app#941 ## Description This merges the feature branch `feat/square-size-independent-commitments` to `main`. The feature branch includes 3 already approved PRs: - celestiaorg/celestia-app#937 - celestiaorg/celestia-app#983 - celestiaorg/celestia-app#982 and two additional commits that haven't been reviewed yet to resolve the tasks below ## Tasks - [x] Merge in `main` and resolve merge conflicts caused by renames: - `payForData` => `payForBlob` - `x/payment` => `x/blob` - [x] Update ADR 008 to describe that it was implemented Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com> Co-authored-by: Matthew Sevey <mjsevey@gmail.com>
Implementation for square size independent message commitments. Note this targets
feat/square-size-independent-commitments
per #941 so that we can unblock review.