babe: make secondary slot randomness available on-chain #7053

octol · 2020-09-08T22:45:00Z

Make secondary slot randomness available on-chain: #7046

~~TODO~~

Make sure that the implementation satisfies the requirements in the issue
Tests

rphmeier · 2020-09-15T00:30:28Z

frame/babe/src/lib.rs

-		Initialized::put(maybe_randomness);
+		let maybe_primary_randomness = maybe_randomness.filter(|_| is_primary);
+
+		Initialized::put(maybe_primary_randomness);


I would find this cleaner to read just as a single if is_primary { Initializer:put(maybe_randomness) }

Note that this is not exactly the same as what the existing version does, which puts a None when there is no randomness

You could do

Initialized::put(if is_primary { maybe_randomness } else { None });

The base value of Initialized is None, so it logically doesn't make a difference. But yeah, the second option is more exact

I tried the suggested approach of if is_primary { Initialized::put(maybe_randomness) } but got assertion failures here

substrate/frame/babe/src/lib.rs

Line 464 in e65a60c

debug_assert!(Self::initialized().is_some());

in a lot of the frame-babe tests

rphmeier · 2020-09-15T00:30:54Z

frame/babe/src/lib.rs

+			digest
+				.vrf_output()
+				.and_then(|vrf_output| {
+					// place the VRF output into the `Initialized` storage item


This comment is now outdated. Seems like it should move to below.

rphmeier

looks fine except for nits. thanks!

andresilva

lgtm. just minor nits.

andresilva · 2020-09-16T15:07:50Z

frame/babe/src/lib.rs

+		// once we've decided which epoch this block is in.
+		Initialized::put(if is_primary { maybe_randomness } else { None });
+
+		// Place the both primary and secondary VRF output into the


Suggested change

// Place the both primary and secondary VRF output into the

// Place either the primary or secondary VRF output into the

andresilva · 2020-09-16T15:10:15Z

frame/babe/src/lib.rs

+		/// Temporary value (cleared at block finalization) which is `Some` if we
+		/// have generated VRF randomness.


Suggested change

/// Temporary value (cleared at block finalization) which is `Some` if we

/// have generated VRF randomness.

/// Temporary value (cleared at block finalization) that includes the VRF randomness generated at this block.

/// This field should always be populated during block processing unless secondary plain slots are enabled

/// (which don't contain a VRF proof).

andresilva · 2020-09-16T15:12:56Z

frame/babe/src/lib.rs

+						.map(|inout| {
+							inout.make_bytes(&sp_consensus_babe::BABE_VRF_INOUT_CONTEXT)
+						})
+			})


Suggested change

})

})

andresilva · 2020-09-16T15:21:22Z

frame/babe/src/lib.rs

@@ -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>;


I know that this is not related to this PR, but we're essentially declaring this as Option<Option<Randomness>> (same for Initialized). Do we really need the double option wrapping or was it just an oversight? Both keys are supposed to be cleared before block execution finishes.

Well for Initialized the outer Option seems to have a specific meaning? See for example

substrate/frame/babe/src/lib.rs

Line 464 in e65a60c

debug_assert!(Self::initialized().is_some());

However for AuthorVrfRandomness yes probably the outer Option can be removed

Yeah right the semantics for Initialized require both options since the outer signals that we have done the initialization procedure (regardless of whether there was a primary VRF to extract or not). For AuthorVrfRandomness I think we could do with removing it and just use MaybeRandomness.

andresilva · 2020-09-16T15:22:31Z

frame/babe/src/tests.rs

@@ -126,6 +128,99 @@ fn first_block_epoch_zero_start() {
 	})
 }

+fn make_vrf_output(


ocd: maybe move to mock.rs next to the other utility functions?

andresilva · 2020-09-16T15:24:00Z

frame/babe/src/tests.rs

+	(vrf_output, vrf_proof, vrf_randomness)
+}
+
+#[test]


Most of the duplication in the tests below could be factored out, but won't block merge on this.

…y-slot-randomness-available

andresilva

made some final changes for minor nits. lgtm 👍

andresilva · 2020-09-18T15:39:47Z

bot merge

ghost · 2020-09-18T15:39:50Z

Waiting for commit status.

andresilva · 2020-09-18T15:47:45Z

bot merge cancel

ghost · 2020-09-18T15:47:47Z

Merge cancelled.

andresilva · 2020-09-18T16:13:58Z

The PR has been reviewed and looks good but should not be merged until it is audited.

andresilva · 2020-10-14T16:37:00Z

@octol The audit is done and no issues were found. Can you merge master to fix the conflicts (I believe it's only about the increased spec_version)?

Conflicts: bin/node/runtime/src/lib.rs

andresilva · 2020-10-15T08:32:08Z

bot merge

ghost · 2020-10-15T08:32:11Z

Trying merge.

babe: make secondary slot randomness available on-chain

7762a5e

octol added A3-in_progress Pull request is in progress. No review needed at this stage. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Sep 8, 2020

octol added 2 commits September 9, 2020 09:03

babe: extract out vrf_output function

7ee26e5

babe: add missing comment

792423f

This was referenced Sep 9, 2020

Sassafras Consensus #4600

Closed

Generalize pallet-babe to be independent of BABE primitives #5655

Closed

octol added 2 commits September 9, 2020 13:28

babe: fix incorrectly storing primary randomness

1bdf010

babe: add test for onchain author vrf

00e5aa8

octol requested review from rphmeier and andresilva September 14, 2020 11:12

octol marked this pull request as ready for review September 14, 2020 11:12

github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 14, 2020

paritytech-ci added the D2-breaksapi label Sep 14, 2020

rphmeier reviewed Sep 15, 2020

View reviewed changes

rphmeier approved these changes Sep 15, 2020

View reviewed changes

rphmeier added A6-mustntgrumble and removed A0-please_review Pull request needs code review. labels Sep 15, 2020

octol added 3 commits September 16, 2020 00:24

babe: fix reviewer nits

19d1d2e

Merge branch 'master' into jon/make-secondary-slot-randomness-available

6ad70aa

runtime: bump spec_version

b351f28

andresilva suggested changes Sep 16, 2020

View reviewed changes

octol added 4 commits September 17, 2020 21:07

babe: remove outer Option for AuthorVrfRandomness

350fadf

babe: fix reviewer nits on doc strings

79edaa9

babe: move make_vrf_output to mock.rs

914f20d

Merge remote-tracking branch 'upstream/master' into jon/make-secondar…

7063bb5

…y-slot-randomness-available

andresilva added the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Sep 18, 2020

andresilva added 3 commits September 18, 2020 16:32

babe: cleanup docs

de1c4d9

babe: kill ephemeral entry instead of take

417f449

babe: use type alias for maybe randomness

ec68c40

andresilva approved these changes Sep 18, 2020

View reviewed changes

github-actions bot added the A7-needspolkadotpr label Sep 18, 2020

Merge branch 'master' into jon/make-secondary-slot-randomness-available

8c7f07d

andresilva removed A6-mustntgrumble labels Sep 18, 2020

github-actions bot added the A7-needspolkadotpr label Sep 18, 2020

gnunicorn added the A3-needsresolving label Oct 1, 2020

andresilva 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 Oct 14, 2020

Merge branch 'master' into jon/make-secondary-slot-randomness-available

577c215

Conflicts: bin/node/runtime/src/lib.rs

andresilva removed the A3-needsresolving label Oct 14, 2020

github-actions bot removed the A7-needspolkadotpr label Oct 14, 2020

ghost merged commit d86a98c into master Oct 15, 2020

ghost deleted the jon/make-secondary-slot-randomness-available branch October 15, 2020 08:32

andresilva mentioned this pull request Oct 22, 2020

BABE should make secondary slot randomness available on-chain #7046

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

babe: make secondary slot randomness available on-chain #7053

babe: make secondary slot randomness available on-chain #7053

octol commented Sep 8, 2020 •

edited

Loading

rphmeier Sep 15, 2020

octol Sep 15, 2020

octol Sep 15, 2020

rphmeier Sep 15, 2020

octol Sep 15, 2020

rphmeier Sep 15, 2020

rphmeier left a comment

andresilva left a comment

andresilva Sep 16, 2020

andresilva Sep 16, 2020

andresilva Sep 16, 2020

andresilva Sep 16, 2020

octol Sep 16, 2020

andresilva Sep 16, 2020 •

edited

Loading

andresilva Sep 16, 2020

andresilva Sep 16, 2020

andresilva left a comment •

edited

Loading

andresilva commented Sep 18, 2020

ghost commented Sep 18, 2020

andresilva commented Sep 18, 2020

ghost commented Sep 18, 2020

andresilva commented Sep 18, 2020

andresilva commented Oct 14, 2020

andresilva commented Oct 15, 2020

ghost commented Oct 15, 2020

	// Place the both primary and secondary VRF output into the
	// Place either the primary or secondary VRF output into the

		/// Temporary value (cleared at block finalization) which is `Some` if we
		/// have generated VRF randomness.

-		/// Temporary value (cleared at block finalization) which is `Some` if we
-		/// have generated VRF randomness.
+		/// Temporary value (cleared at block finalization) that includes the VRF randomness generated at this block.
+		/// This field should always be populated during block processing unless secondary plain slots are enabled
+		/// (which don't contain a VRF proof).

babe: make secondary slot randomness available on-chain #7053

babe: make secondary slot randomness available on-chain #7053

Conversation

octol commented Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier left a comment

Choose a reason for hiding this comment

andresilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresilva Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresilva left a comment • edited Loading

Choose a reason for hiding this comment

andresilva commented Sep 18, 2020

ghost commented Sep 18, 2020

andresilva commented Sep 18, 2020

ghost commented Sep 18, 2020

andresilva commented Sep 18, 2020

andresilva commented Oct 14, 2020

andresilva commented Oct 15, 2020

ghost commented Oct 15, 2020

octol commented Sep 8, 2020 •

edited

Loading

andresilva Sep 16, 2020 •

edited

Loading

andresilva left a comment •

edited

Loading