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

BEEFY: Simplify hashing for pallet-beefy-mmr #12393

Merged
merged 8 commits into from
Oct 4, 2022
Merged

BEEFY: Simplify hashing for pallet-beefy-mmr #12393

merged 8 commits into from
Oct 4, 2022

Conversation

serban300
Copy link
Contributor

@serban300 serban300 commented Sep 30, 2022 •
edited
Loading

Reuse sp_runtime::traits::Hash instead of defining a new Hasher trait for beefy.

Addresses both items in issue #12387

polkadot companion: paritytech/polkadot#6098

@serban300 serban300 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Sep 30, 2022
@serban300 serban300 self-assigned this Sep 30, 2022
acatangiu
acatangiu approved these changes Oct 3, 2022
View reviewed changes
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@acatangiu
Copy link
Contributor

PR needs a Polkadot companion

@serban300 serban300 mentioned this pull request Oct 3, 2022
Merged
@serban300
Copy link
Contributor Author

PR needs a Polkadot companion

Sorry, I missed this. Done. Also reduced the number of generic parameters for merkle_root() and other functions.

svyatonik
svyatonik approved these changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure there has been some reason to have this custom trait - e.g. to explicitly voice that it'll only work with H256 (to make it compatible with other H256 chains) and do not confuse this hash with the "base" hash, used on the chain. But I agree that it is harder to develop with two similar traits. If we'll ever need some restrictions on the hash type, we may use constraints for that.

frame/beefy-mmr/primitives/src/lib.rs Outdated
{
let upper = Vec::with_capacity(leaves.size_hint().0);
let upper = Vec::with_capacity(leaves.size_hint().1.unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you, please, change .unwrap() to .expect("<why-you-think-noone-will-ever-see-this-text; qed>") (see https://github.com/paritytech/substrate/blob/master/docs/STYLE_GUIDE.md#style for details).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I would just change it to

Suggested change
let upper = Vec::with_capacity(leaves.size_hint().1.unwrap());
let upper = Vec::with_capacity(leaves.size_hint().1.unwrap_or(0));

Copy link
Contributor Author

@serban300 serban300 Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this was just an experiment that I forgot about. Normally for Vec size_hint().1 will always return Some(len), but probably it doesn't make sense for other type of iterators. I rolled it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm unwrap_or(0) also sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Changed it to leaves.size_hint().1.unwrap_or(0).saturating_add(1)) / 2

@acatangiu
Copy link
Contributor

bot merge

@paritytech-processbot paritytech-processbot bot merged commit e77cbe3 into paritytech:master Oct 4, 2022
@serban300 serban300 mentioned this pull request Oct 4, 2022
Closed
serban300 added a commit that referenced this pull request Oct 4, 2022
@serban300
BEEFY: Simplify hashing for pallet-beefy-mmr (#12393)
eed30e4 
* beefy-mmr: reuse sp_runtime::traits::Keccak256

* beefy-mmr: use sp_runtime::traits:Hash for generating merkle proofs

* beefy-mmr: use sp_runtime::traits:Hash for validating merkle proofs

* beefy-mmr: remove primitives::Hasher and primitives::Hash

* fixes

* beefy-mmr: reduce the number of generic parameters for merkle_root()

* fix

* compute upper Vec capacity more accurately
@serban300 serban300 mentioned this pull request Oct 4, 2022
Open
ordian added a commit that referenced this pull request Oct 5, 2022
@ordian
Merge branch 'master' into ao-update-kvdb-and-co
bd29c35 
* master: (42 commits)
  Adapt `pallet-contracts` to WeightV2 (#12421)
  Improved election pallet testing (#12327)
  Bump prost to 0.11+ (#12419)
  Use saturating add for alliance::disband witness data (#12418)
  [Fix] Rename VoterBagsList -> VoterList to match pdot (#12416)
  client/beefy: small code improvements (#12414)
  BEEFY: Simplify hashing for pallet-beefy-mmr (#12393)
  Add @koute to `docs/CODEOWNERS` and update stale paths (#12408)
  docs/CODEOWNERS: add @acatangiu as MMR owner (#12406)
  Remove unnecessary Clone trait bounds on CountedStorageMap (#12402)
  Fix `Weight::is_zero` (#12396)
  Beefy on-demand justifications as a custom RequestResponse protocol (#12124)
  Remove contracts RPCs (#12358)
  pallet-mmr: generate historical proofs (#12324)
  unsafe_pruning flag removed (#12385)
  Carry over where clauses defined in Config to Call and Hook (#12388)
  Properly set the max proof size weight on defaults and tests (#12383)
  BEEFY: impl TypeInfo for SignedCommitment (#12382)
  bounding staking: `BoundedElectionProvider` trait (#12362)
  New Pallet: Root offences (#11943)
  ...
serban300 added a commit that referenced this pull request Oct 7, 2022
@serban300
BEEFY: Simplify hashing for pallet-beefy-mmr (#12393)
9354359 
* beefy-mmr: reuse sp_runtime::traits::Keccak256

* beefy-mmr: use sp_runtime::traits:Hash for generating merkle proofs

* beefy-mmr: use sp_runtime::traits:Hash for validating merkle proofs

* beefy-mmr: remove primitives::Hasher and primitives::Hash

* fixes

* beefy-mmr: reduce the number of generic parameters for merkle_root()

* fix

* compute upper Vec capacity more accurately
@arielwang829 arielwang829 mentioned this pull request Nov 3, 2022
Merged
@serban300 serban300 deleted the mmr_hashing branch December 14, 2022 10:04
@AurevoirXavier AurevoirXavier mentioned this pull request Jan 10, 2023
Closed
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@serban300 @ark0f
BEEFY: Simplify hashing for pallet-beefy-mmr (paritytech#12393)
0ae9038 
* beefy-mmr: reuse sp_runtime::traits::Keccak256

* beefy-mmr: use sp_runtime::traits:Hash for generating merkle proofs

* beefy-mmr: use sp_runtime::traits:Hash for validating merkle proofs

* beefy-mmr: remove primitives::Hasher and primitives::Hash

* fixes

* beefy-mmr: reduce the number of generic parameters for merkle_root()

* fix

* compute upper Vec capacity more accurately
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@acatangiu acatangiu acatangiu approved these changes

@sandreim sandreim sandreim approved these changes

@svyatonik svyatonik svyatonik approved these changes

@Lederstrumpf Lederstrumpf Awaiting requested review from Lederstrumpf

Assignees

@serban300 serban300

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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@serban300 @acatangiu @svyatonik @sandreim