Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add epoch transition to spec #63

Merged
merged 5 commits into from
Feb 6, 2024
Merged

Add epoch transition to spec #63

merged 5 commits into from
Feb 6, 2024

Conversation

zeegomo
Copy link
Contributor

@zeegomo zeegomo commented Feb 5, 2024

No description provided.

@@ -36,6 +38,7 @@ def s(self):

# An absolute unique indentifier of a slot, counting incrementally from 0
@dataclass
@functools.total_ordering
Copy link
Contributor

Choose a reason for hiding this comment

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

cool, didn't know about this

Comment on lines -256 to +287
if new_chain == self.local_chain:
# we have not re-org'd therefore we can simply update our ledger state
# if this block extend our local chain
if self.local_chain.tip() == block:
self.ledger_state.apply(block)
else:
# we have re-org'd, therefore we must roll back out ledger state and
# re-apply blocks from the new chain
ledger_state = self.genesis_state.copy()
for block in new_chain.blocks:
ledger_state.apply(block)

self.ledger_state = ledger_state
self.local_chain = new_chain
new_state = self.ledger_state[block.parent].copy()
new_state.apply(block)
self.ledger_state[block.id()] = new_state
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that's simpler

@@ -27,24 +28,39 @@ class TimeConfig:
class Config:
k: int
active_slot_coeff: float # 'f', the rate of occupied slots
# The numerator multiplier in the formula used for calculating the number of slots in an epoch.
# An epoch is exactly int(floor(epoch_length_multiplier * k / f slots)) long.
epoch_length_multiplier: int
Copy link
Contributor

@davidrusu davidrusu Feb 6, 2024

Choose a reason for hiding this comment

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

epoch length is derived from the stabilization periods of the different snapshots taken within an epoch.

Thus, the target is (4 + 3 + 3) * k / f
where 4 is for stabilizing the stake freezing snapshot, 3 is the buffer between frozen stake and nonce snapshot, and the last 3 is for stabilizing the nonce snapshot.

how about we decompose this multiple into those three periods?

    epoch_period_stake_distribution: int
    epoch_period_nonce_buffer: int
    epoch_period_nonce: int

   @property
    def base_period_length(self) -> int:
        return int(floor(self.k / self.active_slot_coeff))

   @property
    def epoch_length(self) -> int:
        return (self.epoch_period_stake_distribution_stabilization
            + self.epoch_period_nonce_buffer
            + self.epoch_period_nonce_stabilization) * self.base_period_length

Then we can use these period configs when creating snapshots

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, and makes it impossible to configure values that do not make sense (e.g. seting epoch_length_multiplier to say 2 when the nonce snapshot is taken after 7k/f)

Comment on lines -170 to +187
commitments=self.commitments.copy(),
nullifiers=self.nullifiers.copy(),
commitments=deepcopy(self.commitments),
nullifiers=deepcopy(self.nullifiers),
Copy link
Contributor

Choose a reason for hiding this comment

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

these commitments/nullifiers are never changed no? I didn't think there was a risk to a shallow copy.

But better to be paranoid 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We add the nullifier of the new leader proof, so yes that changes

Comment on lines 321 to 326
def get_last_valid_state(self, chain: Chain, slot: Slot) -> LedgerState:
for block in reversed(chain.blocks):
if block.slot < slot:
return self.ledger_state[block.id()]

@dataclass
class EpochState:
# for details of snapshot schedule please see:
# https://github.com/IntersectMBO/ouroboros-consensus/blob/fe245ac1d8dbfb563ede2fdb6585055e12ce9738/docs/website/contents/for-developers/Glossary.md#epoch-structure

# The stake distribution snapshot is taken at the beginning of the previous epoch
stake_distribution_snapshot: LedgerState
return self.genesis_state
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "valid" mean here? how about just state_at_slot(...)?

Comment on lines 337 to 340
nonce_slot = Slot(
int(floor(7 * self.config.k / self.config.active_slot_coeff))
+ stake_snapshot_slot.absolute_slot
)
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think putting the epoch periods in the config would make this code clearer

Copy link
Contributor

@davidrusu davidrusu left a comment

Choose a reason for hiding this comment

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

looks good

@zeegomo zeegomo merged commit c1e12d6 into master Feb 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants