Remove epoch from tipset structure
anorth opened this issue · 7 comments
Does it matter if F3 verifies this (beyond the validating that our peers are proposing the same epochs we are)?
I'm asking because it's convenient to be able to get this value without having to dig into the tipset itself, especially if we ever start committing to additional values in the commitment tree (https://www.notion.so/pl-strflt/F3-Commitments-Power-Table-d4dfc762a53a4c16a6afe2f9165b2c96).
Removing epochs is incompatible with #117, but I'm no longer sure #117 is well-founded – I've forgotten the reasoning.
The problem with including the epoch is partly one of validation. A node can't generally verify that the epoch proposed by a peer corresponds to the tipset ID. The node may not have ever seen that tipset. Now, if 2/3 of power proposes the same (tipset, epoch) pair then that is enough social validation to assume it's correct. But a correct algorithm requires that we treat (TipsetA, EpochN) and (TipsetA, EpochN+1) as different proposals. The current code does not: it just uses the tipset ID as the unique key for a proposed value.
If we really need (#117) or want the epoch in these values, then we need to refactor to use hash((tipset, epoch)) as the map keys throughout. This isn't unreasonable, but could be avoided if we removed the epoch altogether.
However, the easy way may be foiled by power table/commitment CID that is still missing from the code (pending #5). This will mean each protocol value is at least a tuple of (tipset, commitment) and we must similarly not assume the commitment follows from the tipset in values received from peers.
Yeah, I agree. We need to agree on the entire value (tipset, epoch, commitment, etc...), not just the tipset key. But from what I can tell we already key on the TipSet struct which includes the epoch.
we already key on the TipSet struct which includes the epoch.
Ah yes, I now remember deciding that was cheaper than hashing. And that will still be true when we add another []byte to the structure, I think.
Ok great, so we're still to decide if #117 is the right fix, but maybe we want to leave Epoch here anyway as convenience.
We resolved to remove epoch, so this issue is good as written.
We made the tipset entirely opaque in #149, so there's nothing to do for this issue. We'll no doubt add another tipset structure later on but it can be drawn directly from the FIP.
We ended up switching to a structured tipset and kept the epoch. We don't validate that the epoch matches the tipset, but we do include the epoch when computing the longest common prefix.