-
Notifications
You must be signed in to change notification settings - Fork 2.6k
babe: make secondary slot randomness available on-chain #7053
Changes from 5 commits
7762a5e
7ee26e5
792423f
1bdf010
00e5aa8
19d1d2e
6ad70aa
b351f28
350fadf
79edaa9
914f20d
7063bb5
de1c4d9
417f449
ec68c40
8c7f07d
577c215
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -203,6 +203,10 @@ decl_storage! { | |||||
/// if per-block initialization has already been called for current block. | ||||||
Initialized get(fn initialized): Option<MaybeRandomness>; | ||||||
|
||||||
/// Temporary value (cleared at block finalization) which is `Some` if we | ||||||
/// have generated VRF randomness. | ||||||
AuthorVrfRandomness get(fn author_vrf_randomness): Option<MaybeRandomness>; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know that this is not related to this PR, but we're essentially declaring this as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well for substrate/frame/babe/src/lib.rs Line 464 in e65a60c
However for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah right the semantics for |
||||||
|
||||||
/// How late the current block is compared to its parent. | ||||||
/// | ||||||
/// This entry is populated as part of block execution and is cleaned up | ||||||
|
@@ -248,6 +252,9 @@ decl_module! { | |||||
Self::deposit_randomness(&randomness); | ||||||
} | ||||||
|
||||||
// The stored author generated VRF randomness is ephemeral. | ||||||
AuthorVrfRandomness::take(); | ||||||
|
||||||
// remove temporary "environment" entry from storage | ||||||
Lateness::<T>::kill(); | ||||||
} | ||||||
|
@@ -542,6 +549,8 @@ impl<T: Trait> Module<T> { | |||||
}) | ||||||
.next(); | ||||||
|
||||||
let is_primary = matches!(maybe_pre_digest, Some(PreDigest::Primary(..))); | ||||||
|
||||||
let maybe_randomness: Option<schnorrkel::Randomness> = maybe_pre_digest.and_then(|digest| { | ||||||
// on the first non-zero block (i.e. block #1) | ||||||
// this is where the first epoch (epoch #0) actually starts. | ||||||
|
@@ -571,38 +580,47 @@ impl<T: Trait> Module<T> { | |||||
Lateness::<T>::put(lateness); | ||||||
CurrentSlot::put(current_slot); | ||||||
|
||||||
if let PreDigest::Primary(primary) = digest { | ||||||
// place the VRF output into the `Initialized` storage item | ||||||
// and it'll be put onto the under-construction randomness | ||||||
// later, once we've decided which epoch this block is in. | ||||||
// | ||||||
// Reconstruct the bytes of VRFInOut using the authority id. | ||||||
Authorities::get() | ||||||
.get(primary.authority_index as usize) | ||||||
.and_then(|author| { | ||||||
schnorrkel::PublicKey::from_bytes(author.0.as_slice()).ok() | ||||||
}) | ||||||
.and_then(|pubkey| { | ||||||
let transcript = sp_consensus_babe::make_transcript( | ||||||
&Self::randomness(), | ||||||
current_slot, | ||||||
EpochIndex::get(), | ||||||
); | ||||||
|
||||||
primary.vrf_output.0.attach_input_hash( | ||||||
&pubkey, | ||||||
transcript | ||||||
).ok() | ||||||
}) | ||||||
.map(|inout| { | ||||||
inout.make_bytes(&sp_consensus_babe::BABE_VRF_INOUT_CONTEXT) | ||||||
}) | ||||||
} else { | ||||||
None | ||||||
} | ||||||
let authority_index = digest.authority_index(); | ||||||
|
||||||
// Extract out the VRF output if we have it | ||||||
digest | ||||||
.vrf_output() | ||||||
.and_then(|vrf_output| { | ||||||
// place the VRF output into the `Initialized` storage item | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is now outdated. Seems like it should move to below. |
||||||
// and it'll be put onto the under-construction randomness | ||||||
// later, once we've decided which epoch this block is in. | ||||||
// | ||||||
// Place the both primary and secondary VRF output into the | ||||||
// `AuthorVrfRandomness` storage item. | ||||||
// | ||||||
// Reconstruct the bytes of VRFInOut using the authority id. | ||||||
Authorities::get() | ||||||
.get(authority_index as usize) | ||||||
.and_then(|author| { | ||||||
schnorrkel::PublicKey::from_bytes(author.0.as_slice()).ok() | ||||||
}) | ||||||
.and_then(|pubkey| { | ||||||
let transcript = sp_consensus_babe::make_transcript( | ||||||
&Self::randomness(), | ||||||
current_slot, | ||||||
EpochIndex::get(), | ||||||
); | ||||||
|
||||||
vrf_output.0.attach_input_hash( | ||||||
&pubkey, | ||||||
transcript | ||||||
).ok() | ||||||
}) | ||||||
.map(|inout| { | ||||||
inout.make_bytes(&sp_consensus_babe::BABE_VRF_INOUT_CONTEXT) | ||||||
}) | ||||||
}) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
}); | ||||||
|
||||||
Initialized::put(maybe_randomness); | ||||||
let maybe_primary_randomness = maybe_randomness.filter(|_| is_primary); | ||||||
|
||||||
Initialized::put(maybe_primary_randomness); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would find this cleaner to read just as a single There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this is not exactly the same as what the existing version does, which puts a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could do Initialized::put(if is_primary { maybe_randomness } else { None }); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The base value of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried the suggested approach of substrate/frame/babe/src/lib.rs Line 464 in e65a60c
|
||||||
AuthorVrfRandomness::put(maybe_randomness); | ||||||
|
||||||
// enact epoch change, if necessary. | ||||||
T::EpochChangeTrigger::trigger::<T>(now) | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,13 +100,15 @@ fn first_block_epoch_zero_start() { | |
assert_eq!(Babe::genesis_slot(), genesis_slot); | ||
assert_eq!(Babe::current_slot(), genesis_slot); | ||
assert_eq!(Babe::epoch_index(), 0); | ||
assert_eq!(Babe::author_vrf_randomness(), Some(Some(vrf_randomness))); | ||
|
||
Babe::on_finalize(1); | ||
let header = System::finalize(); | ||
|
||
assert_eq!(SegmentIndex::get(), 0); | ||
assert_eq!(UnderConstruction::get(0), vec![vrf_randomness]); | ||
assert_eq!(Babe::randomness(), [0; 32]); | ||
assert_eq!(Babe::author_vrf_randomness(), None); | ||
assert_eq!(NextRandomness::get(), [0; 32]); | ||
|
||
assert_eq!(header.digest.logs.len(), 2); | ||
|
@@ -126,6 +128,99 @@ fn first_block_epoch_zero_start() { | |
}) | ||
} | ||
|
||
fn make_vrf_output( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ocd: maybe move to |
||
slot_number: u64, | ||
pair: &sp_consensus_babe::AuthorityPair | ||
) -> (VRFOutput, VRFProof, [u8; 32]) { | ||
let pair = sp_core::sr25519::Pair::from_ref(pair).as_ref(); | ||
let transcript = sp_consensus_babe::make_transcript(&Babe::randomness(), slot_number, 0); | ||
let vrf_inout = pair.vrf_sign(transcript); | ||
let vrf_randomness: sp_consensus_vrf::schnorrkel::Randomness = vrf_inout.0 | ||
.make_bytes::<[u8; 32]>(&sp_consensus_babe::BABE_VRF_INOUT_CONTEXT); | ||
let vrf_output = VRFOutput(vrf_inout.0.to_output()); | ||
let vrf_proof = VRFProof(vrf_inout.1); | ||
|
||
(vrf_output, vrf_proof, vrf_randomness) | ||
} | ||
|
||
#[test] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of the duplication in the tests below could be factored out, but won't block merge on this. |
||
fn author_vrf_output_for_primary() { | ||
let (pairs, mut ext) = new_test_ext_with_pairs(1); | ||
|
||
ext.execute_with(|| { | ||
let genesis_slot = 10; | ||
let (vrf_output, vrf_proof, vrf_randomness) = make_vrf_output(genesis_slot, &pairs[0]); | ||
let primary_pre_digest = make_pre_digest(0, genesis_slot, vrf_output, vrf_proof); | ||
|
||
System::initialize( | ||
&1, | ||
&Default::default(), | ||
&Default::default(), | ||
&primary_pre_digest, | ||
Default::default(), | ||
); | ||
assert_eq!(Babe::author_vrf_randomness(), None); | ||
|
||
Babe::do_initialize(1); | ||
assert_eq!(Babe::author_vrf_randomness(), Some(Some(vrf_randomness))); | ||
|
||
Babe::on_finalize(1); | ||
System::finalize(); | ||
assert_eq!(Babe::author_vrf_randomness(), None); | ||
}) | ||
} | ||
|
||
#[test] | ||
fn author_vrf_output_for_secondary_vrf() { | ||
let (pairs, mut ext) = new_test_ext_with_pairs(1); | ||
|
||
ext.execute_with(|| { | ||
let genesis_slot = 10; | ||
let (vrf_output, vrf_proof, vrf_randomness) = make_vrf_output(genesis_slot, &pairs[0]); | ||
let secondary_vrf_pre_digest = make_secondary_vrf_pre_digest(0, genesis_slot, vrf_output, vrf_proof); | ||
|
||
System::initialize( | ||
&1, | ||
&Default::default(), | ||
&Default::default(), | ||
&secondary_vrf_pre_digest, | ||
Default::default(), | ||
); | ||
assert_eq!(Babe::author_vrf_randomness(), None); | ||
|
||
Babe::do_initialize(1); | ||
assert_eq!(Babe::author_vrf_randomness(), Some(Some(vrf_randomness))); | ||
|
||
Babe::on_finalize(1); | ||
System::finalize(); | ||
assert_eq!(Babe::author_vrf_randomness(), None); | ||
}) | ||
} | ||
|
||
#[test] | ||
fn no_author_vrf_output_for_secondary_plain() { | ||
new_test_ext(1).execute_with(|| { | ||
let genesis_slot = 10; | ||
let secondary_plain_pre_digest = make_secondary_plain_pre_digest(0, genesis_slot); | ||
|
||
System::initialize( | ||
&1, | ||
&Default::default(), | ||
&Default::default(), | ||
&secondary_plain_pre_digest, | ||
Default::default(), | ||
); | ||
assert_eq!(Babe::author_vrf_randomness(), None); | ||
|
||
Babe::do_initialize(1); | ||
assert_eq!(Babe::author_vrf_randomness(), Some(None)); | ||
|
||
Babe::on_finalize(1); | ||
System::finalize(); | ||
assert_eq!(Babe::author_vrf_randomness(), None); | ||
}) | ||
} | ||
|
||
#[test] | ||
fn authority_index() { | ||
new_test_ext(4).execute_with(|| { | ||
|
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.