Skip to content
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

Change Author Slot Filter to rely on pallet_randomness (Moonbase only) #1956

Merged
merged 14 commits into from Dec 21, 2022

Conversation

nbaztec
Copy link
Contributor

@nbaztec nbaztec commented Nov 16, 2022

What does it do?

attempts using pallet_randomness for auth_slot_filter.

The randomness seed is the LocalVrfOutput of the previous block, salted with the subject, which includes the relay block number for collision prevention when the vrf output does not change (in case block production is skipped).
The genesis block will have the default 0x00... value of the hash.

Breaking

This changes might break tools monitoring Author selection as it now relies on a different input (from collective flip previously, to moonbeam pallet randomness) to ensure the correct author

What important points reviewers should know?

pallet-randomness-collective uses a low-entropy randomness, while LocalVRF is high-entropy (allegedly). This should ideally be okay IMO since high-entropy yields a better randomness.

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@nbaztec nbaztec added the B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes label Nov 16, 2022
runtime/moonbase/src/lib.rs Outdated Show resolved Hide resolved
@nbaztec nbaztec added the D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Nov 25, 2022
@nbaztec nbaztec marked this pull request as ready for review November 25, 2022 11:21
Copy link
Collaborator

@librelois librelois left a comment

Choose a reason for hiding this comment

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

PreviousLocalVrfOutput don't need to be an Option. By default (at genesis or first block after upgrade) this will be the default hash (0x0000...0000).

pallets/randomness/src/lib.rs Outdated Show resolved Hide resolved
@nbaztec nbaztec changed the title [MOON-1812] attempt using pallet_randomness for auth_slot_filter [RFC] [MOON-1812] attempt using pallet_randomness for auth_slot_filter Nov 25, 2022
/// 2. First 2 bytes of index.to_le_bytes() when selecting the ith eligible author
/// 3. First 4 bytes of slot_number.to_be_bytes()
///
/// Note: This needs to be updated when asynchronous backing is in effect,
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Note/TODO/ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:D I'm agnostic here. Will create a tracking ticket for it anyhow. Not a huge fan of TODOs if they're not currently actionable.
Didn't we wanna remove TODOs from our codebase though?

@nbaztec nbaztec merged commit 2c7c152 into master Dec 21, 2022
@nbaztec nbaztec deleted the nish-randomness-author-slot-filter branch December 21, 2022 09:37
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jan 11, 2023
@crystalin crystalin changed the title [MOON-1812] attempt using pallet_randomness for auth_slot_filter Change Author Slot Filter to rely on pallet_randomness Jan 20, 2023
@crystalin crystalin changed the title Change Author Slot Filter to rely on pallet_randomness Change Author Slot Filter to rely on pallet_randomness (Moonbase only) Jan 20, 2023
@crystalin crystalin added the breaking Needs to be mentioned in breaking changes label Jan 20, 2023
imstar15 pushed a commit to OAK-Foundation/moonbeam that referenced this pull request May 16, 2023
…nbeam-foundation#1956)

* attempt using pallet_randomness for auth_slot_filter

* use subject with local vrf output for randomness

* fix slice copy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants