[CBR-150] Implement rollback #3245
Conversation
edsko
requested review from
adinapoli-iohk,
denisshevchenko and
parsonsmatt
as code owners
edsko
force-pushed
the
feature/cbr-150-implement-rollback
branch
2 times, most recently
from
7022c65 to
1807318
Compare
|
Oops, sorry @erikd , left a debugging change to a core file (actually just whitespace), which is why you got roped in as a reviewer. Will fix. |
edsko
force-pushed
the
feature/cbr-150-implement-rollback
branch
3 times, most recently
from
ed8046a to
e9358f0
Compare
| txs' | ||
| let raw = mkRawResolvedBlock block resolvedTxInputs | ||
| checkpoint <- mkCheckpoint prev slot raw | ||
| if siSlot slot == localSlotIndexMaxBound pc |
There was a problem hiding this comment.
[minor] I find this slightly more confusing, since it's not obvious what localSlotIndexMaxBound is and why we should care here.
There was a problem hiding this comment.
Reintroduced
isEpochBoundary :: ProtocolConstants -> SlotId -> Bool
isEpochBoundary pc slot = siSlot slot == localSlotIndexMaxBound pcIt's removal was an unintended consequence of the refactoring.
| flatten (b, Nothing) = [Right (rawResolvedBlock b)] | ||
| flatten (b, Just ebb) = [Right (rawResolvedBlock b), Left ebb] | ||
|
|
||
| -- The Cardano verifier has a curious pecularity that we are not allowed to |
There was a problem hiding this comment.
Should clarify whether this is a property of the Cardano verifier, or whether it's just the ESREmptyEpoch case. And then discuss what we should do in that case.
There was a problem hiding this comment.
I'll leave that in your capable hands :)
There was a problem hiding this comment.
LGTM, excellent work @edsko! I have just some minor style & API remarks, and would be curious to hear what you think 😉
| , applyBlocks | ||
| , switchToFork | ||
| -- *** Testing | ||
| , observableRollbackUseInTestsOnly |
There was a problem hiding this comment.
Cannot get more explicit than this! 😬
| -> [ResolvedBlock] -- ^ Blocks in the new fork | ||
| -> IO () | ||
| switchToFork pw@PassiveWallet{..} n bs = do | ||
| blockssByAccount <- mapM (prefilterBlock' pw) bs |
There was a problem hiding this comment.
Something we might need to revisit later (jotting down out loud notes here): another good testament for CBR-341, as prefilterBlock' is calling Keystore.toList every time, and that might be expensive if the number of blocks to resolve is big.
There was a problem hiding this comment.
Added a comment to CBR-341 that this toList should be removed completely
| data Checkpoint = Checkpoint { | ||
| _checkpointUtxo :: InDb Core.Utxo | ||
| , _checkpointUtxoBalance :: InDb Core.Coin | ||
| , _checkpointExpected :: InDb Core.Utxo |
There was a problem hiding this comment.
Are we tracking in a ticket anywhere that we need to tackle expected balance separately from this PR?
There was a problem hiding this comment.
| -- Use with caution! This loses all the indices, potentially losing all the | ||
| -- benefits that 'IxSet' provides. | ||
| toMap :: HasPrimKey a => IxSet a -> Map (PrimKey a) a | ||
| toMap = Map.mapKeysMonotonic (primKey . unwrapOrdByPrimKey) |
There was a problem hiding this comment.
Isn't this falling into the same anti-pattern we wanted to avoid with toList? In my current branch I am carefully trying to avoid to introduce an Ixset.toList function, but this one, I guess, could be used in an evil manner to write a toList implementation, couldn't it?
If we think this is unavoidable, I would be inclined to take the plunge and also export a toList function with the same caveats, as it would make my code less horrid in some places. Happy to bounce off ideas here.
There was a problem hiding this comment.
In this particular case this is precisely the format I needed, and the code is somewhat non-trivial, so I figured it belonged in the main IxSet wrapper. We can reconsider toList if you really think it's needed?
| liftOldestFirst' :: Functor m | ||
| => (f a -> m (f a)) | ||
| -> OldestFirst f a -> m (OldestFirst f a) | ||
| liftOldestFirst' f = fmap OldestFirst . f . getOldestFirst |
There was a problem hiding this comment.
Wouldn't liftOldestFirstM be a slightly better name? Not very picky about this but some folks are really adamant that the prime naming should be used only for strict variants 😉
There was a problem hiding this comment.
I hadn't heard of that convention, I certainly don't use it, but fair enough :) Renamed to liftOldestFirstF and co. (My first version was in fact called liftOldestFirstM but then I realized it just needed a Functor constraint, not a Monad constraint, and I wasn't sure what name to use anymore :).
| module Wallet.Inductive.History ( | ||
| History -- opaque | ||
| -- * "Difference history" (like difference lists) | ||
| , HistoryD -- opaque |
There was a problem hiding this comment.
Pure 🚴 shedding -- what about DHistory as in, literally, DList? 😁
(Jokes aside -- this is testing code, so it doesn't really matter).
There was a problem hiding this comment.
To be honest, this is a relic of the old version of the History. I'll get rid of this type completely.
edsko
force-pushed
the
feature/cbr-150-implement-rollback
branch
2 times, most recently
from
45e3be4 to
6a32c88
Compare
This is the full implemention of rollback, along with various fixes to the test infrastructure. * Wrote a hand-written test to test specifically for dependent pending transactions, because the generator doesn't always hit that case (and fixed the bug that Ryan intentionally left in because he rightly wanted a test first that exposes it first). * Fixed a bug in the test infrastructure: the interpreter requires state that depends on the blockchain (hash of the previous block, the stake distrubtion, etc). It is therefore important that we correctly track this state and rollback the interpreter state when there is a rollback in the blockchain. * Improved the test infrastructure to get better information when this happens. * Don't update stakes for pending txs The interpreter context maintains a set of stakes, and this set of stakes was bieng updated on every transaction that we translated (in `pushTx`). This is incorrect as it means we're updating stakes even for transactions originating from "new pending" events, and it messes also with rollback. Instead, we now update the stakes only when we create a block. * Removed the EBB from 'RawResolvedBlock', and instead just changed the type of interpretating a block. This means we don't have to deal with these changes upstream. It also has the additional benefit that we are forced to be more explicit when we ignore the EBB (for instance, do we pass the EBB as an explicit block in `interpretT`? Right now, we don't.) NOTE: With the new structure it was more convenient to pair the EBB with the _last_ block in the epoch, rather than the first. * Added `ccMagic :: ProtocolMagic` to `CardanoContext` and got rid of `withProtocolMagic`; this makes this consistent with everything else. * Renamed 'icNextSlot' to 'icUnsafeNextSlot' and isolated all access to it in 'getNextSlot'. This makes the flow of data a lot clearer. * Relatedly, eemoved 'icEpoch' from 'IntCtxt'. Having both 'icNextSlot' and 'icEpoch' was adding to the confusion. * Moved the EBB creation to its own function. * Made it clear that `translateFirstSlot` and `translateNextSlot` cannot throw errors. * Take advantage of 'fromRawResolvedBlock' in the stake computation. * Rework the interpreter state The interpreter state now mirrors the concepts we use elsewhere: we have a "global" database of transaction metadata, which never gets rolled back, and then separately a list of checkpoints, which we append to each time we apply a block. This makes the whole flow of data in the interpreter much clearer, and makes clients such as `interpretT` significantly cleaner. * Strip off final EBB, if any Apparently the Cardano validator does not like a chain that terminates on an EBB, so if there is a final EBB without any subsequent regular blocks we just strip it off. * Generalized the spans-epochs test slightly so that we test with different number of epochs plus or minus a few blocks to test for this.
We currently throw this away, since we don't have any validation directly on the EBB.
edsko
force-pushed
the
feature/cbr-150-implement-rollback
branch
from
6a32c88 to
3b6317a
Compare
Description
Implement rollback
Linked issue
https://iohk.myjetbrains.com/youtrack/issue/CBR-150
https://iohk.myjetbrains.com/youtrack/issue/CBR-151
Type of change
Developer checklist
Testing checklist
QA Steps
Screenshots (if available)