Reject legacy snapshots #1774
Reject legacy snapshots #1774
Conversation
jasagredo
requested review from
amesgen,
bladyjoker,
dnadales,
fraser-iohk,
geo2a and
nfrisby
as code owners
jasagredo
force-pushed
the
js/tracing-forkers
branch
from
c279b8e to
6f0481a
Compare
jasagredo
force-pushed
the
js/reject-legacy
branch
from
0d5773e to
1a84a00
Compare
jasagredo
force-pushed
the
js/tracing-forkers
branch
from
6f0481a to
0be9949
Compare
jasagredo
force-pushed
the
js/reject-legacy
branch
from
1a84a00 to
764ea8f
Compare
jasagredo
force-pushed
the
js/tracing-forkers
branch
from
0be9949 to
b45f224
Compare
jasagredo
force-pushed
the
js/reject-legacy
branch
from
764ea8f to
125e68f
Compare
jasagredo
force-pushed
the
js/tracing-forkers
branch
from
b45f224 to
7af3e4d
Compare
jasagredo
force-pushed
the
js/reject-legacy
branch
from
125e68f to
b5127a5
Compare
| --> | ||
| ### Breaking | ||
|
|
||
| - Legacy snapshots will be rejected and deleted, instead of crashing consensus. |
There was a problem hiding this comment.
From the code, I see that we check if the snapshot is a file and, if so, infer that it is in the legacy format and throw and exception. Is the deletion already done when we handle this exception? Where can I find this code, if it exists?
There was a problem hiding this comment.
Ah I see, it's not in Consensus, the exception need to be handled in the node. In that case, I think this changelog entry needs to be reformulated:
| - Legacy snapshots will be rejected and deleted, instead of crashing consensus. | |
| - Legacy snapshots will be rejected instead of crashing Consensus, and an error will be signaled upstream, so that `cardano-node` could choose what to do with the incompatible snapshot. |
There was a problem hiding this comment.
The logic is here:
https://github.com/IntersectMBO/ouroboros-consensus/blob/main/ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/API.hs#L596-L601
We delete the snapshot if we get an InitFailureRead as we do here.
There was a problem hiding this comment.
Right. I think I like the approach of emitting a warning and suggesting the convert the snapshot, as you've described in #1708, but if you think that deleting is fine, then let's delete.
geo2a
left a comment
geo2a
left a comment
There was a problem hiding this comment.
Approved with a suggesting to reformulate the changelog entry.
Closes #1708