pallet-randomness-beacon: security and genesis #133
+208
−55
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
colemanirby
reviewed
Mar 10, 2025
pallets/randomness-beacon/src/lib.rs
Outdated
| @@ -173,6 +181,13 @@ pub mod pallet { | |||
| GenesisRoundNotSet, | |||
| /// The genesis is already set. | |||
| GenesisRoundAlreadySet, | |||
| /// There must be at least one signature to construct an asig | |||
| ZeroHeightProvided, | |||
| /// There number of aggregated signatures exceeds the maximum rounds we can verify per | |||
There was a problem hiding this comment.
Typo here. I think it should say "The number of..."
juangirini
reviewed
Mar 11, 2025
| fn check_inherent(call: &Self::Call, _data: &InherentData) -> Result<(), Self::Error> { | ||
| match call { | ||
| Call::try_submit_asig { .. } => Ok(()), | ||
| _ => unreachable!("other calls are not inherents"), |
There was a problem hiding this comment.
I've seen this elsewhere in Substrate, do you know why panicing instead of returning an error?
juangirini
reviewed
Mar 11, 2025
Comment on lines
+256
to
+261
| /// A dummy `on_initialize` to return the amount of weight that `on_finalize` requires to | ||
| /// execute. | ||
| fn on_initialize(_n: BlockNumberFor<T>) -> Weight { | ||
| // weight of `on_finalize` | ||
| T::WeightInfo::on_finalize() | ||
| } |
There was a problem hiding this comment.
why do we return the on_finalize weight?
juangirini
reviewed
Mar 11, 2025
Comment on lines
+263
to
+264
| /// At the end of block execution, the `on_finalize` hook checks that the timestamp was | ||
| /// updated. Upon success, it removes the boolean value from storage. If the value resolves |
There was a problem hiding this comment.
I guess you meant the 'aggregated signatures' instead of 'timestamp'?
juangirini
reviewed
Mar 11, 2025
| // aggregate old asig/apk with the new one and verify the aggregation | ||
| // Q: do we care about the entire linear history of message hashes? |
There was a problem hiding this comment.
This is useful for searching for the keyword 'todo'
Suggested change
| // Q: do we care about the entire linear history of message hashes? | |
| // TODO: do we care about the entire linear history of message hashes? |
juangirini
approved these changes
Mar 11, 2025
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 focuses on ensuring the integrity and security of the randomness beacon pallet.
try_sumit_asigextrinsic to be callable a single time per block. While the initial intention was to use a signed payload with an unsigned transaction to provide security to the extrinsic, this proved infeasible unless an offchain worker is used. OCWs do not guarantee execution, making this unviable. More simply, the extrinsic operates similarly to pallet_timestamp, simply ensuring that the extrinsic can be called once per block. Since the call is submitted as an inherent, the validator-authored call is the first one in the transaction pool that is executed even if others try to submit calls. In addition, subsequent calls within the same block would fail regardless, as invalid (forgery attack) and duplicate signatures are rejected (replay attack). This closes pallet-randomness-beacon: inherent security #117