Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Revert "Optimize merkle proofs for efficient verification in Solidity (#12857)" #14176

Merged
merged 2 commits into from
May 22, 2023
Merged

Revert "Optimize merkle proofs for efficient verification in Solidity (#12857)" #14176

merged 2 commits into from
May 22, 2023

Conversation

Lederstrumpf
Copy link
Contributor

Reverts the lexical ordering of hashing of the BEEFY authority merkle tree #12857 since we still require commitment to the leaf indices, which the lexical ordering removes and would require another mechanism for enforcement.
See #12820 for context. Some candidates for salvaging the gas improvement down the line are described here, albeit they currently still require benchmarking.

cc @doubledup & @vgeddes (can't request review), since you'll adjust correspondingly in Solidity, and in case you've had changes to the verifier affected by this (other than the lexical ordering).

@Lederstrumpf Lederstrumpf added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels May 18, 2023
@acatangiu
Copy link
Contributor

@vgeddes @seunlanlege can we get an explicit ack here? I don't want you to be surprised by this protocol change.

@vgeddes
Copy link
Contributor

vgeddes commented May 19, 2023

@vgeddes @seunlanlege can we get an explicit ack here? I don't want you to be surprised by this protocol change.

Thanks, yes, we would definitely like this to be merged.

@seunlanlege
Copy link
Contributor

@vgeddes @seunlanlege can we get an explicit ack here? I don't want you to be surprised by this protocol change.

No objections here

@Lederstrumpf Lederstrumpf marked this pull request as ready for review May 22, 2023 11:33
@acatangiu acatangiu requested a review from serban300 May 22, 2023 11:40
@Lederstrumpf
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 20bd05c into paritytech:master May 22, 2023
gpestana pushed a commit that referenced this pull request May 27, 2023
@Lederstrumpf @gpestana
Revert "Optimize merkle proofs for efficient verification in Solidity (…
cfa7181 
…#12857)" (#14176)

* Revert "Optimize merkle proofs for efficient verification in Solidity (#12857)"

This reverts commit f9d1dcd since we
still require commitment to the leaves - see #12820.

* remove PartialOrd trait from mmr hash type
@doubledup doubledup mentioned this pull request Jun 15, 2023
Merged
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/exploring-beefy-authority-set-subsampling-optimisations/3106/1

@kiltbot kiltbot mentioned this pull request Jun 21, 2023
Closed
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@Lederstrumpf @nathanwhit
Revert "Optimize merkle proofs for efficient verification in Solidity (…
2a73694 
…paritytech#12857)" (paritytech#14176)

* Revert "Optimize merkle proofs for efficient verification in Solidity (paritytech#12857)"

This reverts commit f9d1dcd since we
still require commitment to the leaves - see paritytech#12820.

* remove PartialOrd trait from mmr hash type
@seunlanlege
Copy link
Contributor

Lol this broke my merkle multi proofs. Not even sure how you're arriving at the root.

7v1yas

@Lederstrumpf
Copy link
Contributor Author

Lol this broke my merkle multi proofs. Not even sure how you're arriving at the root.

7v1yas

This PR, i.e. the reversion, broke your multiproofs?

@seunlanlege
Copy link
Contributor

Lol this broke my merkle multi proofs. Not even sure how you're arriving at the root.

7v1yas

This PR, i.e. the reversion, broke your multiproofs?

Yup, I had multi proofs for unsorted nodes (v1 Merkle scheme)

Then it was changed to sorted nodes, and I designed another multi proof scheme for this

Now that the sorting has been removed. Neither of my previous designs work anymore. Can we just use the first version?

@acatangiu
Copy link
Contributor

Now that the sorting has been removed. Neither of my previous designs work anymore. Can we just use the first version?

I'm confused, after this revert we're actually using the "first version", no?

@seunlanlege
Copy link
Contributor

seunlanlege commented Aug 15, 2023 •
edited
Loading

I'm confused, after this revert we're actually using the "first version", no?

Apologies, Yeah this is now the case. I was unwittingly pre-sorting leaves in my tests. An artifact of the previous scheme

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@acatangiu acatangiu acatangiu approved these changes

@serban300 serban300 serban300 approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Lederstrumpf @acatangiu @vgeddes @seunlanlege @Polkadot-Forum @serban300