Algod: state-proof key deletion safety#4601
Conversation
Codecov Report
@@ Coverage Diff @@
## feature/stateproofs-recoverability #4601 +/- ##
======================================================================
- Coverage 54.53% 54.03% -0.51%
======================================================================
Files 408 408
Lines 52650 52658 +8
======================================================================
- Hits 28714 28453 -261
- Misses 21540 21791 +251
- Partials 2396 2414 +18
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
algonathan
changed the base branch from
master
to
feature/stateproofs-recoverability
id-ms
force-pushed
the
stateprf-keydelete
branch
from
98719a4 to
3d0b984
Compare
id-ms
force-pushed
the
stateprf-keydelete
branch
from
3d0b984 to
3ee4000
Compare
id-ms
force-pushed
the
stateprf-keydelete
branch
from
3ee4000 to
0e8a113
Compare
id-ms
force-pushed
the
stateprf-keydelete
branch
from
15387cd to
9e5ec61
Compare
id-ms
force-pushed
the
stateprf-keydelete
branch
from
9468178 to
a8b7543
Compare
id-ms
force-pushed
the
stateprf-keydelete
branch
2 times, most recently
from
d99b8e3 to
65d8b21
Compare
| return | ||
| } | ||
|
|
||
| oldestRoundToRemove := stateProofNextRound.SubSaturate(basics.Round(proto.StateProofInterval)) |
There was a problem hiding this comment.
If I understand correctly, for every round (?) we're going to be going over all of the participation records, extracting the state proof keys from them and removing stale keys? It might be worth it to hold the previous state proof next round to decide if we have to do that, even if we put aside the question of changing state proof intervals.
There was a problem hiding this comment.
You have a point. I've addressed that. take a second look
| func (spw *Worker) deleteOldBuilders(currentHdr *bookkeeping.BlockHeader) { | ||
| oldestRoundToRemove := GetOldestExpectedStateProof(currentHdr) | ||
| func (spw *Worker) deleteStaleKeys(latestRoundToKeep basics.Round) { | ||
| keys := spw.accts.StateProofKeys(latestRoundToKeep) |
There was a problem hiding this comment.
Won't we miss out on purging stale data from accounts that can't sign latestRoundToKeep but still have old keys in their DB?
It seems to me we should simply iterate over all accounts and invoke spw.accts.DeleteStateProofKey(participationID, latestRoundToKeep) for all of them, no?
| spw.log.Errorf("deleteOldKeys: could not calculate keylifetime for account %v on round %s: %v", key.ParticipationID, roundToRemove, err) | ||
| continue | ||
| } | ||
| err = spw.accts.DeleteStateProofKey(key.ParticipationID, basics.Round(roundToRemove)) |
There was a problem hiding this comment.
Why do we need to use roundToRemove? Why not latestRoundToKeep instead?
There was a problem hiding this comment.
if we delete roundToRemove we might delete a key that should be used for a later state proof.
almog-t
left a comment
almog-t
left a comment
There was a problem hiding this comment.
Let's iron out the kinks in the deletion part, otherwise looks good.
id-ms
force-pushed
the
stateprf-keydelete
branch
from
16943a3 to
62be9ac
Compare
id-ms
force-pushed
the
stateprf-keydelete
branch
from
62be9ac to
c5f89f1
Compare
4 participants
Summary
Due to StateProof keys' lifetime, the signer might delete the key while not all signatures are present in the DB.
This critical error might occur when the DB fails to record a signature and a new signature arrives from the same key.