Cardano-wallet: Change `applyBlocks` to return a list of checkpoints

Created on 21 Aug 2019  路  10Comments  路  Source: input-output-hk/cardano-wallet

Context

We want to save checkpoints individually rather than in chunks.

Decision

Change Cardano.Wallet.Primitive.Model.applyBlocks to return a list of wallet checkpoints, one for each block that it has been given.

applyBlocks
    :: forall s t. (DefineTx t)
    => [Block (Tx t)]
    -> Wallet s t
    -> [(Map (Hash "Tx") (Tx t, TxMeta), Wallet s t)]

Since TxMeta contains a SlotId already, it's not necessary to split the transactions by block.

In Cardano.Wallet, adjust restoreBlocks accordingly. It should run DB.putCheckpoint once per checkpoint. DB.putTransactions would be invoked as before.

Acceptance Criteria

  1. Function applyBlocks must return one checkpoint per block applied.
  2. A checkpoint must be created for each block that is applied.

PR

| Number | Base |
| --- | --- |
| #668 | master |
| #669 | master |
| #674 | master |
| #684 | master |

QA

  • [ ] build a test to verify that exactly one checkpoint is created for every block, regardless of the number of blocks returned by nextBlocks. This test could be built with a mocked-out DBLayer, and a mocked-out NetworkLayer.

All 10 comments

@rvl Because of your response to my suggestion of removing applyBlocks in favour for applyBlock (here), was the description here intentional:

applyBlocks
    :: forall s t. (DefineTx t)
    => [Block (Tx t)]
    -> Wallet s t
    -> [(Map (Hash "Tx") (Tx t, TxMeta), Wallet s t)]

? because it is also different from your original suggestion of returning a single Map.

If applyBlocks were to depend on the blockchain params, it would need new blockchain params for each block applied. By only exposing applyBlock to the wallet layer, this would become more transparent. I know this is not how it is or will be, but still I thought there's some general neatness to it 鈥攕tate advances one block at a time.

@Anviking Yes I adjusted applyBlocks return type so that it can be implemented more simply in terms of applyBlock. @jonathanknowles tells me this is a scan (more or less).

I guess the blockchain params you're talking about is slot duration (for TxMeta time). But let's cross that bridge when we come to it, and keep it simple for now.

@jonathanknowles, can you put few words in QA section? thx :)

@jonathanknowles, can you put few words in QA section? thx :)

Done. I think that testing of this can be accomplished through mocks, what do you think?

As discussed, two options for testing this:

  • with mocks, as mentioned in QA section,
  • include as part of QSM tests

To be sorted on Monday.

cc: @jonathanknowles, @rvl

As discussed, putting back to in progress as there is:

  • #684
  • additional work required to make the change more test friendly (i.e. making restoreBlock to be a pure function)

@piotr-iohk, putting back in QA. #684 has been merged and "fixed" the nightly restoration as we wanted: https://buildkite.com/input-output-hk/cardano-wallet-nightly/builds/202

The nightly is still failing though, but because of the recent move from CI to Buildkite.. Seems like we have a problem clearing our cache.

The nightly Buildkite pipeline was failing in a completely unrelated step which I recently added as part of the stack build caching work. I have fixed that in #702.

@KtorZ thanks. Is: additional work required to make the change more test friendly (i.e. making restoreBlock to be a pure function) still a plan for this task @rvl, @jonathanknowles ?

@piotr-iohk I am on it, hopefully I'll push that today although that's part of a bigger whole. I think this particular can be closed.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

jonathanknowles picture jonathanknowles  路  7Comments

Anviking picture Anviking  路  3Comments

gufmar picture gufmar  路  9Comments

paweljakubas picture paweljakubas  路  6Comments

got3nks picture got3nks  路  9Comments