Introduce new concept of "slot portion for proposing" #8280

bkchr · 2021-03-05T19:03:37Z

Currently when building a block we actually give the proposer all of the
time in the slot, while this is wrong. The slot is actually split in at
least two phases proposing and propagation or in the polkadot case into
three phases validating pov's, proposing and propagation. As we don't
want to bring that much polkadot concepts into Substrate, we only
support splitting the slot into proposing and propagation. The portion
can now be passed as parameter to AuRa and BABE to configure this value.
However, this slot portion for propagation doesn't mean that the
proposer can not go over this limit. When we miss slots we still apply
the lenience factor to increase the proposing time, so that we have
enough time to build a heavy block.

Besides all what was said above, this is especially required for
parachains. Parachains have a much more constraint proposing window.
Currently the slot duration is at minimum 12 seconds, but we only have
around 500ms for proposing. So, this slot portion for proposing is
really required to make it working without hacks.

polkadot companion: paritytech/polkadot#2578

Currently when building a block we actually give the proposer all of the time in the slot, while this is wrong. The slot is actually split in at least two phases proposing and propagation or in the polkadot case into three phases validating pov's, proposing and propagation. As we don't want to bring that much polkadot concepts into Substrate, we only support splitting the slot into proposing and propagation. The portion can now be passed as parameter to AuRa and BABE to configure this value. However, this slot portion for propagation doesn't mean that the proposer can not go over this limit. When we miss slots we still apply the lenience factor to increase the proposing time, so that we have enough time to build a heavy block. Besides all what was said above, this is especially required for parachains. Parachains have a much more constraint proposing window. Currently the slot duration is at minimum 12 seconds, but we only have around 500ms for proposing. So, this slot portion for proposing is really required to make it working without hacks.

octol

Looking good!

octol · 2021-03-08T08:21:56Z

client/consensus/slots/src/lib.rs

 		let proposing = awaiting_proposer.and_then(move |proposer| proposer.propose(
 			slot_info.inherent_data,
 			sp_runtime::generic::Digest {
 				logs,
 			},
-			slot_remaining_duration,
+			proposing_remaining_duration.mul_f32(0.98),


Is there a risk that the 2% ends up being too small when the remaining duration becomes very small? As opposed to use a % of the proposing time or slot time which should be a pretty stable duration?

That is a good question. For parachains this will currently be 500ms for proposing, so leaving us with 10ms buffer. I would say that this is enough buffer, especially as the select is biased to the proposing, meaning this will be polled first.
Do you have some better idea?

I think we should use this for now and adapt if someone complains.

WDYT @andresilva ?

Currently the margin was 0% so this is better :P

For the default case, but with a lenience factor applied, the timeout was always > than the remaining time of the slot. (Before if there was a runtime upgrade that maybe took 7 seconds and a proposing time of 8 seconds after applying the lenience factor, we would have build the block and ended the proposing. Now, it could theoretically happen that we still add one or 2 transactions 😬 as we propagate the real proposing time to the block builder.)

paritytech/substrate#8280

andresilva · 2021-03-09T15:04:27Z

client/consensus/slots/src/lib.rs

 		let proposing = awaiting_proposer.and_then(move |proposer| proposer.propose(
 			slot_info.inherent_data,
 			sp_runtime::generic::Digest {
 				logs,
 			},
-			slot_remaining_duration,
+			proposing_remaining_duration.mul_f32(0.98),


Currently the margin was 0% so this is better :P

andresilva · 2021-03-09T15:07:18Z

client/consensus/slots/src/lib.rs

@@ -613,7 +621,7 @@ pub fn slot_lenience_linear(parent_slot: Slot, slot_info: &SlotInfo) -> Option<D
 		None
 	} else {
 		let slot_lenience = std::cmp::min(skipped_slots, BACKOFF_CAP);
-		Some(Duration::from_millis(slot_lenience * slot_info.duration))
+		Some(slot_info.duration.mul_f64(slot_lenience as f64))


Suggested change

Some(slot_info.duration.mul_f64(slot_lenience as f64))

Some(slot_lenience * slot_info.duration)

I first had this, but they don't implement Mull<f64> for Duration :D

slot_lenience should not be f64 here, we are just multiplying by the number of skipped slots.

Ahh now I remember. slot_lenience is an u64 and they don't support multiplication with that. BUT, they support multiplication with f64.

But yeah, I think the maximum value is 20. So, I can just cast it to an u32.

When I was reviewing the PR I noticed that above for the exponential case we just multiply, and I found it strange that here it was different :P

bkchr · 2021-03-09T19:14:50Z

bot merge

ghost · 2021-03-09T19:14:53Z

Trying merge.

* Substrate companion #8280 paritytech/substrate#8280 * "Update Substrate" Co-authored-by: parity-processbot <>

* Introduce new concept of "slot portion for proposing" Currently when building a block we actually give the proposer all of the time in the slot, while this is wrong. The slot is actually split in at least two phases proposing and propagation or in the polkadot case into three phases validating pov's, proposing and propagation. As we don't want to bring that much polkadot concepts into Substrate, we only support splitting the slot into proposing and propagation. The portion can now be passed as parameter to AuRa and BABE to configure this value. However, this slot portion for propagation doesn't mean that the proposer can not go over this limit. When we miss slots we still apply the lenience factor to increase the proposing time, so that we have enough time to build a heavy block. Besides all what was said above, this is especially required for parachains. Parachains have a much more constraint proposing window. Currently the slot duration is at minimum 12 seconds, but we only have around 500ms for proposing. So, this slot portion for proposing is really required to make it working without hacks. * Offgit feedback * Cast cast cast

bkchr 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. labels Mar 5, 2021

bkchr requested review from andresilva and octol March 5, 2021 19:03

github-actions bot added the A7-needspolkadotpr label Mar 5, 2021

octol approved these changes Mar 8, 2021

View reviewed changes

bkchr added 2 commits March 8, 2021 20:54

Offgit feedback

5904f9f

Merge remote-tracking branch 'origin/master' into bkchr-slot-portion

ed557c9

bkchr added a commit to paritytech/polkadot that referenced this pull request Mar 8, 2021

Substrate companion #8280

2d2cd98

paritytech/substrate#8280

bkchr mentioned this pull request Mar 8, 2021

Substrate companion #8280 paritytech/polkadot#2578

Merged

andresilva approved these changes Mar 9, 2021

View reviewed changes

Cast cast cast

57b4dce

ghost merged commit 8fca15b into master Mar 9, 2021

ghost deleted the bkchr-slot-portion branch March 9, 2021 19:14

ghost pushed a commit to paritytech/polkadot that referenced this pull request Mar 9, 2021

Substrate companion #8280 (#2578)

a6c637a

* Substrate companion #8280 paritytech/substrate#8280 * "Update Substrate" Co-authored-by: parity-processbot <>

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce new concept of "slot portion for proposing" #8280

Introduce new concept of "slot portion for proposing" #8280

bkchr commented Mar 5, 2021 •

edited

octol left a comment

octol Mar 8, 2021

bkchr Mar 8, 2021

andresilva Mar 9, 2021

bkchr Mar 9, 2021

andresilva Mar 9, 2021

andresilva Mar 9, 2021

bkchr Mar 9, 2021

andresilva Mar 9, 2021

bkchr Mar 9, 2021

andresilva Mar 9, 2021 •

edited

bkchr commented Mar 9, 2021

ghost commented Mar 9, 2021

	Some(slot_info.duration.mul_f64(slot_lenience as f64))
	Some(slot_lenience * slot_info.duration)

Introduce new concept of "slot portion for proposing" #8280

Introduce new concept of "slot portion for proposing" #8280

Conversation

bkchr commented Mar 5, 2021 • edited

octol 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

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 Mar 9, 2021 • edited

Choose a reason for hiding this comment

bkchr commented Mar 9, 2021

ghost commented Mar 9, 2021

bkchr commented Mar 5, 2021 •

edited

andresilva Mar 9, 2021 •

edited