Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Independence of Slot-based algorithms from system Timestamp #12224

Merged
merged 18 commits into from
Sep 23, 2022
Merged

Independence of Slot-based algorithms from system Timestamp #12224

merged 18 commits into from
Sep 23, 2022

Conversation

davxy
Copy link
Member

@davxy davxy commented Sep 8, 2022

Removal from SlotInfo:

  • This struct field is just redundant.
  • If required it can be easily computed as slot_info.slot * slot_info.duration.

(Edited)

Slot based algorithms such as Babe, Aura and (in the future) Sassafras should not explicitly depend on Timestamp.

This PR relaxes this requirement by removing direct timestamp inclusion from Slots crate.

If the slot value is computed using the system timestamp then this should be a user choice part of the node construction (i.e. within the bin).


Note that this also simplifies the creation of slot values for future for other consensus algorithms using a different criteria.

For example I wish to at least try (or keep the door open) to implement 2 different slot providers for Sassafras:

  • one using system time (as the one used by babe right now)
  • one using the median algorithm described by the babe paper (thus that relies on information that abstracts the physical system)

polkadot companion: paritytech/polkadot#5997
cumulus companion: paritytech/cumulus#1617

@davxy davxy requested review from bkchr and a team September 8, 2022 18:13
@davxy davxy self-assigned this Sep 8, 2022
@davxy davxy 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Sep 8, 2022
@davxy davxy requested review from skunert and koute September 9, 2022 06:22
client/consensus/slots/src/lib.rs Outdated Show resolved Hide resolved
@davxy davxy requested a review from bkchr September 12, 2022 07:31
@davxy davxy changed the title Remove redundant timestamp field from SlotInfo Independence of Slot-based algorithms from system Timestamp Sep 12, 2022
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Some nitpicks, otherwise looking good! Ty

client/consensus/slots/src/lib.rs Show resolved Hide resolved
client/consensus/babe/src/tests.rs Show resolved Hide resolved
client/consensus/babe/src/tests.rs Show resolved Hide resolved
client/consensus/aura/src/lib.rs Show resolved Hide resolved
primitives/timestamp/src/lib.rs Outdated Show resolved Hide resolved
@rphmeier
Copy link
Contributor

The dependency on timestamp was used to perform the check that the Timestamp set by the block author is in alignment with the slot selected by the author. e.g. to ensure that authors don't set timestamps in the future, which are not coordinated with the slot.

Does this keep that property? It'd be nice to have a test if so.

@davxy
Copy link
Member Author

davxy commented Sep 12, 2022

Does this keep that property? It'd be nice to have a test if so.

@rphmeier the property is enforced by exactly the same means as before.

In practice the check is performed in Babe here:

if pre_digest.slot() > slot_now {
header.digest_mut().push(seal);
return Ok(CheckedHeader::Deferred(header, pre_digest.slot()))
}

And in AURA here:

if slot > slot_now {
header.digest_mut().push(seal);
Ok(CheckedHeader::Deferred(header, slot))
} else {

The check consists in ensuring that the slot used by the block author is less than slot_now.
The way slot_now is computed is opaque as before (in "practice" as a function of timestamp).

I'll check it a test exists already. Otherwise I'm going to add one.

@rphmeier
Copy link
Contributor

Thanks for clarifying!

@bkchr
Copy link
Member

bkchr commented Sep 12, 2022

We also have these checks inside the runtime: https://github.com/paritytech/substrate/blob/master/frame/babe/src/lib.rs#L866-L869

To ensure that timestamp and slot are matching.

@davxy davxy requested a review from a team September 16, 2022 13:38
@davxy
Copy link
Member Author

davxy commented Sep 23, 2022

@andresilva your opinion on this?

@davxy
Copy link
Member Author

davxy commented Sep 23, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit ffcd523 into paritytech:master Sep 23, 2022
@davxy davxy deleted the davxy/remove-timestamp-from-slotinfo branch September 23, 2022 18:21
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…ch#12224)

* Remove timestamp from SlotInfo

* Expose as millis instead of secs

* Nits

* Fix test after field removal

* Yet another test fix

* On the fly timestamp computation

* Removed slot timestamp from logs

* Removed reference to timestamp from slots subsystem

* Slot based algorithm tests do not require timstamp inherent anymore

* Remove junk files

* Further tests cleanup

* Trigger pipeline

* Apply code suggestions

* Trigger pipeline

Co-authored-by: André Silva <andrerfosilva@gmail.com>
@zqhxuyuan zqhxuyuan mentioned this pull request Mar 13, 2023
12 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants