substrate icon indicating copy to clipboard operation
substrate copied to clipboard

Independence of Slot-based algorithms from system Timestamp

Open davxy opened this issue 3 years ago • 4 comments

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: https://github.com/paritytech/polkadot/pull/5997 cumulus companion: https://github.com/paritytech/cumulus/pull/1617

davxy avatar Sep 08 '22 18:09 davxy

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.

rphmeier avatar Sep 12 '22 20:09 rphmeier

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: https://github.com/paritytech/substrate/blob/dd1f1b5f291aa2325eddf5f26037b84cfbbe0957/client/consensus/babe/src/verification.rs#L84-L87

And in AURA here: https://github.com/paritytech/substrate/blob/dd1f1b5f291aa2325eddf5f26037b84cfbbe0957/client/consensus/aura/src/import_queue.rs#L77-L80

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.

davxy avatar Sep 12 '22 20:09 davxy

Thanks for clarifying!

rphmeier avatar Sep 12 '22 20:09 rphmeier

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.

bkchr avatar Sep 12 '22 20:09 bkchr

@andresilva your opinion on this?

davxy avatar Sep 23 '22 07:09 davxy

bot merge

davxy avatar Sep 23 '22 17:09 davxy