feat: implement slot arithmetic #39

rrruko · 2024-12-18T21:09:38Z

this is a draft PR for slot arithmetic to help align on implementation as per last week's meeting.

this implements slot-to-time, time-to-slot, slot-to-epoch, and epoch boundary functions. these functions depend on the current state of the node. in particular, the start and end times, the slot lengths, and the epoch lengths of all known eras are required. this information is captured by an EraHistory type roughly corresponding to the Interpeter type used in Ouroboros.Consensus.HardFork.History.Qry.

just like in Qry.hs, these functions require that the latest era in the era history argument ends at the epoch of the safe zone (see Ouroboros.Consensus.HardFork.Combinator.State.Infra), so care must be taken by the caller to ensure that an up-to-date era history is supplied. the end of the era history expresses that we can't definitively predict times beyond that point.

to elaborate on the details, as far as i understand:

an era transition is considered to have been announced if the block confirming the transition is 'stable', i.e. it is at least k blocks behind the tip.
at any given time, if an era transition has not been announced, it is guaranteed not to happen in the next 3k/f slots (this is the "safe zone").
because era transitions only happen at epoch boundaries, an era transition is guaranteed not to happen until the next epoch after the end of the safe zone.
this means that we can safely use the current era's parameters until the start of the epoch after the end of the safe zone.

the expectations for this deliverable have been kept pretty loose so far, so i'm open to any feedback.

KtorZ

Nice start, thanks for the early PR!

KtorZ · 2024-12-20T10:13:38Z

crates/amaru/src/time.rs

Let's put that in another crate actually 🫡

KtorZ · 2024-12-20T10:14:22Z

crates/amaru/src/time.rs

+    pub bound_time: u64, // Milliseconds
+    pub bound_slot: u64,
+    pub bound_epoch: u64,


Suggested change

pub bound_time: u64, // Milliseconds

pub bound_slot: u64,

pub bound_epoch: u64,

pub time: u64, // Milliseconds

pub slot: u64,

pub epoch: u64,

KtorZ · 2024-12-20T10:20:34Z

crates/amaru/src/time.rs

+    pub start_slot: u64,
+    pub end_slot: u64,


Suggested change

pub start_slot: u64,

pub end_slot: u64,

pub start: Slot,

pub end: Slot,

And define somewhere type Slot = u64, that better convey the intent IMO, even though it's semantically the same.

KtorZ · 2024-12-20T10:21:49Z

crates/amaru/src/time.rs

+#[derive(PartialEq, Eq)]
+pub struct EraParams {
+    pub epoch_size: u64,
+    pub slot_length: u64, // Milliseconds
+}


Given that epoch_size and slot_length can't be null, it might be worth making those fields private and defining a smart constructor enforcing those invariants.

KtorZ · 2024-12-20T10:22:55Z

crates/amaru/src/time.rs

+#[derive(PartialEq, Eq)]
+pub struct Summary {
+    pub start: Bound,
+    pub end: Bound,


If i recall correctly, eras don't necessarily have an upper-bound (the last one in particular). At least, it used to be like that but I recall it was changed around Babbage. Not sure how anymore. To be confirmed 😅 ...

KtorZ · 2024-12-20T10:24:14Z

crates/amaru/src/time.rs

+// The last era in the provided EraHistory must end at the time horizon for accurate results. The
+// horizon is the end of the epoch containing the end of the current era's safe zone relative to
+// the current tip. Returns number of milliseconds elapsed since the system start time.
+pub fn slot_to_relative_time(eras: &EraHistory, slot: u64) -> Result<u64, TimeHorizonError> {


Seems like all functions in this module are defined on &EraHistory, so perhaps consider making them methods of that type?

KtorZ · 2024-12-20T10:25:16Z

crates/amaru/src/time.rs

+#[derive(PartialEq, Eq)]
+pub struct EraHistory {
+    pub eras: Vec<Summary>,
+}


Note: we'll eventually have to persist this on disk; so defining CBOR (minicbor) encoding and decoding for it would be useful. JSON (via serde) is also arguably useful, though priority is on CBOR.

KtorZ · 2024-12-20T10:26:22Z

crates/amaru/src/time.rs

+        match t1 {
+            Ok(t) => {
+                assert_eq!(t, 172800000);
+            }
+            _ => {
+                panic!("expected no error");
+            }
+        }


Suggested change

match t1 {

Ok(t) => {

assert_eq!(t, 172800000);

}

_ => {

panic!("expected no error");

}

}

assert_eq!(t1, Ok(172800000));

Both more concise and will provide a better error message showing the actual unexpected value.

KtorZ · 2024-12-20T10:27:49Z

crates/amaru/src/time.rs

+        let bounds1 = epoch_bounds(&eras, 10);
+        match bounds1 {
+            Err(TimeHorizonError::PastTimeHorizon) => {
+            }
+            _ => {
+                panic!("expected error");
+            }
+        }


Suggested change

let bounds1 = epoch_bounds(&eras, 10);

match bounds1 {

Err(TimeHorizonError::PastTimeHorizon) => {

}

_ => {

panic!("expected error");

}

}

assert_eq!(epoch_bounds(&eras, 10), Err(TimeHorizonError::PastTimeHorizon))

KtorZ · 2024-12-20T10:28:14Z

crates/amaru/src/time.rs

+                panic!("expected error");
+            }
+        }
+    }


Same for all those 😅, pardon my laziness to suggest changes.

Quantumplation · 2024-12-20T19:01:09Z

crates/time/src/lib.rs

+}
+
+// The start is inclusive and the end is exclusive. In a valid EraHistory, the
+// end of each era will equal the start of the next one.


Is this another constraint it makes sense to validate / enforce in an EraHistory constructor @KtorZ?

i.e. just pass in a Vec<(Bound, Params)>, and then use the start of the next one as the end of the previous one, with the last one taking a None?

abailly

It's one of those parts of the system where I think we need test vectors to ensure computations are identical across implementations. That's something I could contribute if you think that's useful (but not next week :) )

abailly · 2024-12-21T11:11:24Z

crates/time/src/lib.rs

+
+#[derive(Clone, PartialEq, Eq)]
+pub struct Bound {
+    pub time: u64, // Milliseconds


suggestion: either rename to time_ms or have a type Millis = u64 to remove the comment

abailly · 2024-12-21T11:12:46Z

crates/time/src/lib.rs

+pub struct Bound {
+    pub time: u64, // Milliseconds
+    pub slot: Slot,
+    pub epoch: u64,


I think I would also have a type Epoch = u64 but maybe overkill?

abailly · 2024-12-21T11:13:31Z

crates/time/src/lib.rs

+#[derive(Clone, PartialEq, Eq)]
+pub struct EraParams {
+    epoch_size: u64,
+    slot_length: u64, // Milliseconds


Same remark than above

abailly · 2024-12-21T11:13:58Z

crates/time/src/lib.rs

+
+#[derive(Clone, PartialEq, Eq)]
+pub struct EraParams {
+    epoch_size: u64,


I would make the unit explicit, either through the type or name

abailly · 2024-12-21T11:20:38Z

crates/time/src/lib.rs

+        let t0 = eras.slot_to_relative_time(172801);
+        assert_eq!(t0, Err(TimeHorizonError::PastTimeHorizon));
+        let t1 = eras.slot_to_relative_time(172800);
+        assert_eq!(t1, Ok(172800000));


I am not a big fan of testing several things in a single test: When the test fails, it's hard to know quickly what's failed, you would have to look at the source code of the test to know exactly what broke, which is annoying.

I would suggest to:

not prefix test functions with test_, we know they are tests because there's an annotation just above

have more explicit test names (eg. slot_to_time_fails_given_its_past_upper_bound)

have each function test a single "thing" (possibly using parameterised tests to avoid duplication)

abailly · 2024-12-21T11:21:21Z

crates/time/src/lib.rs

+        let params = EraParams::new(86400, 1000).unwrap();
+        let eras = EraHistory {
+            eras: vec![
+                Summary {


You could remove duplication by lifting this definition to be a constant within the test module perhaps?

abailly · 2024-12-21T11:22:08Z

crates/time/src/lib.rs

+    }
+
+    #[test]
+    fn test_encode_era_history() {


One would typically want to have unit tests for encoding/decoding and a roundtrip property which is quite powerful to ensure whatever is encoded can be decoded and find edge cases.

rrruko added 4 commits December 18, 2024 05:19

implement slot/epoch/time functions

d09ae2a

comments

2c5032d

relative time to slot

b0a6acd

remove unused record fields

c55e2f7

KtorZ reviewed Dec 20, 2024

View reviewed changes

rrruko added 2 commits December 20, 2024 05:40

address pr feedback

a12fed3

encoding

53f1b3d

Quantumplation reviewed Dec 20, 2024

View reviewed changes

abailly reviewed Dec 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement slot arithmetic #39

feat: implement slot arithmetic #39

rrruko commented Dec 18, 2024

KtorZ left a comment

KtorZ Dec 20, 2024

KtorZ Dec 20, 2024

KtorZ Dec 20, 2024

KtorZ Dec 20, 2024

KtorZ Dec 20, 2024

KtorZ Dec 20, 2024

KtorZ Dec 20, 2024

KtorZ Dec 20, 2024

KtorZ Dec 20, 2024

KtorZ Dec 20, 2024

KtorZ Dec 20, 2024

Quantumplation Dec 20, 2024

abailly left a comment

abailly Dec 21, 2024

abailly Dec 21, 2024

abailly Dec 21, 2024

abailly Dec 21, 2024

abailly Dec 21, 2024

abailly Dec 21, 2024

abailly Dec 21, 2024

feat: implement slot arithmetic #39

Are you sure you want to change the base?

feat: implement slot arithmetic #39

Conversation

rrruko commented Dec 18, 2024

KtorZ 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abailly 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