Engine switching with committee rotation #531
Conversation
…asier understandable at which slot certain conditions apply
…d thus prevented weight/ratifier propagation
…didates that were decided before the target slot instead of after
…aringThreshold=2*maxCommittableAge
…re committing in attestation manager. This is because at the time the attestation was created, the committee might have been different from the current one (due to rotation at epoch boundary).
… instead of always relying on accountsData of committee before forking point (important when fork persists over epoch boundary with rotating committee)
…s and weight propagation
…ed throughout the slot where blocks are issued. If that is not the case, some blocks might already commit to a more recent commitment (which became committable because of issuing). This, however, makes the issuance non-deterministic and assertions of e.g. attestations impossible
jonastheis
force-pushed
the
feat/engine-switching-committee-rotation-test
branch
from
aab424b to
b31645c
Compare
| if exists { | ||
| validatorAccounts, err := committee.Accounts() | ||
| if err == nil { | ||
| validatorAccountsData, err := c.engine.Ledger.PastAccounts(validatorAccounts.IDs(), c.lastCommonSlotBeforeFork) |
There was a problem hiding this comment.
So if you have a committee for the new epoch you take those accounts with the data at the last common slot before the fork. If instead you don't have a committee for the new epoch you are going to keep using the previous (or initial) validatorAccountsData; you are therefore assuming that the committee should be re-used and it was not rotated.
If I reed the following code correctly, in either case you would need accurate account data at lastCommonSlotBeforeFork ONLY in terms of signing keys: to verify attestations' signatures.
pkg/protocol/sybilprotection/sybilprotectionv1/sybilprotection.go
Outdated
Show resolved
Hide resolved
| // Determine the rewards root. | ||
| { | ||
| targetRewardsEpoch := currentEpoch | ||
| if slot == currentEpochEndSlot { |
There was a problem hiding this comment.
Why would we calculate the rewards for the next epoch? I don't get this.
There was a problem hiding this comment.
Was asking myself the same question. I just refactored that code to be easier understandable what is happening at which time because before it was basically impossible to read
| } | ||
|
|
||
| if err := p.rollbackCommitteesCandidates(targetEpoch, pruningRange[0]-1); err != nil { | ||
| if err := p.rollbackCommitteesCandidates(targetEpoch, startPruneRange); err != nil { |
There was a problem hiding this comment.
You removed the original -1 from the startPruneRange. Was it intended?
There was a problem hiding this comment.
I fixed a bug in the condition as it was candidacySlot < targetSlot which lead to the wrong slots being deleted.
And yes, it was intended because I changed it to be inclusive candidacySlot >= deletionStartSlot. So we delete everything starting and including deletionStartSlot
…ing-committee-rotation-test
This PR addresses several problems around engine switching with committee rotation across epoch boundaries:
AttestationManager: Use the committee of the slot of the attestation in contrast to the slot that we're committing. This is because at the time the attestation was created, the committee might have been different from the current one (due to rotation at epoch boundary).CommitmentVerifier: UpdatevalidatorAccountsDataif fork happened across epoch boundaries. We try to use the latest accounts data (atlastCommonSlotBeforeFork) for the current epoch. This is necessary because the committee might have rotated at the epoch boundary and different validators might be part of it. In case anything goes wrong we keep using previously known accounts data (initially set to the accounts data of the validators for the epoch of the last common commitment before the fork). It could be, that we don't know about a validator when we verify the attestations: we then simply ignore this attestation (just like with public key changes) and weight of the chain will build up slower.Fixes #210, #266, #255