Stake Table Catchup in Hotshot #2623
Conversation
|
Unable to build without Cargo.lock file. This means that after this change 3rd party projects may have For the first failing build see: https://github.com/EspressoSystems/espresso-sequencer/actions/runs/13426683855 To reproduce locally run This PR can still be merged. |
bfish713
requested review from
philippecamacho,
ImJeremyHe and
sveitser
as code owners
bfish713
requested review from
jbearer,
tbro and
imabdulbasit
as code owners
There was a problem hiding this comment.
just leaving some initial comments about the swap in the Membership object internally in hotshot. I think overall it makes sense, but I'm really concerned about some of the uses of next_epoch and prev_epoch and would feel much better if we explicitly passed the full EpochMembershipCoordinator to query for a specific epoch in those places (though in some other places, like where we e.g. check to see if we're the leader for the next epoch I think next_epoch makes complete sense)
I haven't yet looked into the actual catchup logic, I'll try to do that tomorrow
| let we_are_leader = task_state | ||
| .membership | ||
| .read() | ||
| let mem = task_state |
There was a problem hiding this comment.
nit: it might be nice if we had a pattern of calling this epoch_membership or something to emphasize that it's the object for one specific epoch
| let prev_epoch = qc.data.epoch; | ||
| let mem = if prev_epoch < self.membership.epoch() { | ||
| &self.membership.prev_epoch().await.ok()? | ||
| } else { | ||
| &self.membership |
There was a problem hiding this comment.
this seems suspicious, is there not a better way to deal with this? e.g. I feel like we might rather just pass in the EpochMembershipCoordinator so we can specifically get the EpochMembership for prev_epoch
There was a problem hiding this comment.
I don't really know what a realistic attack might be but you can imagine e.g. someone puts in a fake epoch into qc.data.epoch but tricks you into only validating against prev_epoch
There was a problem hiding this comment.
Yeah agree with this point, I had some debugging issues with other places with this pattern as well. I think we should maybe just eplicitly checking that the epoch is for the next or prev epoch rather than < or > the one in the membership would work?
Closes #<ISSUE_NUMBER>
This PR:
Recreates: EspressoSystems/HotShot#4109
This PR does not:
Key places to review: