-
Notifications
You must be signed in to change notification settings - Fork 32
Minimize exposure to exact immutability criterion #1619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
b0f3982
to
cb5ff23
Compare
d791dfb
to
247c489
Compare
0ab3170
to
de3bf4a
Compare
...ros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/Background.hs
Outdated
Show resolved
Hide resolved
...ros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/Background.hs
Outdated
Show resolved
Hide resolved
-- We watch the chain for changes. Whenever the chain is longer than @k@, then | ||
-- the headers older than @k@ are copied from the VolatileDB to the ImmutableDB | ||
-- (using 'copyToImmutableDB'). Once that is complete, | ||
-- We watch the 'cdbChain' for changes. Whenever the chain is longer than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the previous comment, it was clear when blocks were copied from the immutable to the volatile DB. However, I find the current condition a bit hard to understand. Isn't getCurrentChain
always a suffix of "the chain"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
-- We watch the 'cdbChain' for changes. Whenever it is longer than
-- 'getCurrentChain' (the volatile suffix of 'cdbChain'), then new blocks (a
-- prefix of 'cdbChain') became immutable, and are copied from the VolatileDB to
-- the ImmutableDB (using 'copyToImmutableDB').
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now:
-- Wait until the current chain ('cdbChain') is longer than its volatile suffix
-- ('getCurrentChain'). When this occurs, it indicates that new blocks have
-- become immutable. These newly immutable blocks are then copied are copied
-- from the VolatileDB to the ImmutableDB (using 'copyToImmutableDB').
...ros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/Background.hs
Outdated
Show resolved
Hide resolved
...ros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/Background.hs
Show resolved
Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V2.hs
Outdated
Show resolved
Hide resolved
...ros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/Background.hs
Show resolved
Hide resolved
2988210
to
13b1c27
Compare
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl.hs
Show resolved
Hide resolved
247c489
to
08bda65
Compare
13b1c27
to
63069cf
Compare
08bda65
to
dec284f
Compare
This is preparing for Peras, which will introduce a refined immutability criterion. Due to this change, we defer the immutability criterion to the implementation of `getCurrentChain`, reducing the impact of Peras.
This is a preparatory change for Peras, which uses a different immutability criterion (based on weight of chains, instead of (just) length). The only remaining use of the `SecurityParam` is for the snapshot policy 🙃
This requires some knot tying as we initialize the LedgerDB before the ChainDB is fully initialized. This is preparing for Peras, which will introduce a refined immutability criterion. Due to this change, the LedgerDB will automatically use the same criterion as the ChainDB (via `getCurrentChain`).
In case of data loss in the VolatileDB and a node restart, this condition can be violated until we received `k` headers again. Additionally, Ouroboros Peras will use a different immutability criterion where it is perfectly fine to have less than `k` headers on our chain (as long as they have sufficient weight).
63069cf
to
572b241
Compare
-- (using 'copyToImmutableDB'). Once that is complete, | ||
-- Wait until the current chain ('cdbChain') is longer than its volatile suffix | ||
-- ('getCurrentChain'). When this occurs, it indicates that new blocks have | ||
-- become immutable. These newly immutable blocks are then copied are copied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove "are copied"
This PR tries to isolate as much code as possible from the exact criterion for whether a block is immutable. Instead of directly using the Praos criterion ("a block is immutable iff it is buried under at least
k
blocks"), we instead rely ongetCurrentChain
to return the volatile suffix of our chain, with its anchor being the most recent immutable block.This way, the upcoming implementation of Ouroboros Peras (#1594) will not require touching this code, despite modifying the criterion for immutability to be "a block is buried under at least
k
weight".Concretely, this PR adapts the following places:
Forker
s and rolling back.For now, without Peras being implemented, the actual logic doesn't change at all; we still always use Praos-based immutability.
Note that there are still millions of comments that implicitly consider "immutable" and "buried under
k
blocks" to be the same thing; it seems acceptable to defer adapting those.Based on top of #1513 to avoid conflicts
Necessary for tweag/cardano-peras#71, preparation for #1594