Change `applyBlocks` to also accept `BlockSummary` #3172

HeinrichApfelmus · 2022-03-15T12:58:24Z

Issue number

Overview

Light-mode (Epic ADP-1422) aims to make synchronisation to the blockchain faster by trusting an off-chain source of aggregated blockchain data.

In this pull request, we change the applyBlocks function to work with both a list of Block and a BlockSummary. Specifically, we introduce a function applyBlockData which performs the following steps:

discoverFromBlockData: discover addresses and and transaction data (ChainEvents) contained in the sequence of blocks
applyBlockEventsToUTxO: update the wallet UTxO from the given ChainEvents.

Details

Property tests for discoverFromBlockData check that address discovery gives the same result for both a List of blocks and a Summary.
When processing a List of Block, we have to be a bit careful about performance, because we need to traverse ~ 32M transactions (as of Feb 2022). In this case, the transactions contained in BlockEvents are essentially a full copy of the transactions in Block. The best way to copy data is to copy a single reference to the data; for this purpose we introduce a Sublist type which avoids deconstructing and reconstructing the transaction list in the case where we do not filter transactions (All).
In order to preserve the current checkpointing mechanism, applyBlocks actually calls applyBlockData repeatedly instead of passing the full list of blocks. I will need to rethink the checkpointing mechanism in the future — the main issue is that in its current form, a consumer of BlockSummary cannot ask for a specific block height in order to set a suitable checkpoint there. In the future, the checkpointing logic will need to mediate between producer and consumer.

Comments

I expect a slight regression in benchmarks, due to overhead involved in calling applyBlockData repeatedly. However, the introduction of Sublist should prevent a serious deterioration.

* Construct `ChainEvents` from `NonEmpty Block`. * Test that `discoverFromBlockData` yields the same address discovery state on `List` and on `Summary`, at least for the mock type `WalletState`.

…to work with `BlockEvents` instead of `Block`.

`Sublist` = a sublist of a list, with an optimized case for the full list.

HeinrichApfelmus · 2022-03-15T19:16:58Z

bors try

iohk-bors · 2022-03-15T19:30:05Z

try

Build failed:

ci/hydra-build:required

lib/core/src/Cardano/Wallet.hs

paweljakubas · 2022-03-16T12:11:53Z

lib/core/src/Cardano/Wallet/Primitive/BlockSummary.hs

+-- The main purpose of this data type is optimization:
+-- When processing whole 'Block', we want to avoid copying
+-- and redecorating the entire list of transactions in that 'Block';
+-- instead, we want to copy a pointer to this list.


you mean the intent: when All then we process all content, then Some then only those pointed with ix (being map fst content), right?

Yes, though the focus is on the producer here: fromEntireBlock uses All and avoids redecorating. An external blockchain source will return Some with specific indices.

paweljakubas · 2022-03-16T12:16:15Z

lib/core/src/Cardano/Wallet/Primitive/BlockSummary.hs

+nullBlockEvents BlockEvents{transactions,delegations}
+    = null transactions && null delegations
+
+-- | Merge two 'Sublist' assuming that they are sublists of the /same/ list.


yeah, when sublists of the same list are considered then this is perfectly true. The question is can we outlaw calling this function with sublists of different lists? Maybe it would be worth adding some invariant avalilable for export here checking this? Possible?

I wondered the same thing, but I don't see how to do that in this context: We would need a run-time proof that the transaction data returned by an external blockchain data source indeed belongs to the same block; but that is out of reach and it's easier to postulate it using unsafeMkSublist.

In other contexts, one could enforce uniqueness by using an ST-like existential type parameter:

wholeList :: forall a. [a] -> exists s. Sublist s a filterSublist :: forall s a. (a -> Bool) -> Sublist s a -> Sublist s a mergeSublist :: forall s a. Sublist s a -> Sublist s a -> Sublist s a

In other words, the type parameter s would be a representing of the list that the sublist is a sublist of. But we'd be quickly walking into dependent types territory here, and this does not solve the issue in our context where the whole list s is only known on a remote machine.

As Sublist is a small gimmick used for optimization only, I opted to keep it simple and "unsafe" as it is.

paweljakubas · 2022-03-16T12:17:05Z

lib/core/src/Cardano/Wallet/Primitive/BlockSummary.hs

+mergeSublist :: Sublist a -> Sublist a -> Sublist a
+mergeSublist (All xs) _ = All xs -- result cannot be larger
+mergeSublist _ (All ys) = All ys
+mergeSublist (Some xs) (Some ys) = Some $ mergeOn fst const xs ys

 -- | Merge block events that belong to the same block.
 mergeSameBlock :: BlockEvents -> BlockEvents -> BlockEvents


ok, here it is apparent that sublist are from the same list

Yes. I'm not too happy about exporting mergeSublist in the first place, but the export is necessary for testing.

paweljakubas · 2022-03-16T12:18:56Z

lib/core/src/Cardano/Wallet/Primitive/Model.hs

+        , transactions = []
+        , delegations = [] 
+        }
+    fromFiltered (fblock:_) = fblock


we are sure fblock is NonEmpty, right?

Hm? fblock is a single block. As for the list, I think it's indeed true that the argument to fromFiltered will be nonempty when applied to the result of the call to applyBlockData, but that is not obvious 🤔 — that is why I have opted to cover the case of [] regardless.

paweljakubas · 2022-03-16T12:23:50Z

lib/core/src/Cardano/Wallet/Primitive/Model.hs

+    pure (filteredBlocks, (dw, Wallet u1 tip1 s1))
+
+-- | Strict variant of 'mapAccumL'.
+mapAccumL' :: (s -> a -> (o,s)) -> s -> [a] -> ([o],s) 


when I see something like this I always would like to make sure that even irrespective of bang patterns applied we have no thunk build up. I wonder if we should not start using nothunk - some reading for inspiration https://well-typed.com/blog/2020/09/nothunks/

Probably. In other parts of our code, nothunks is used, but not here yet. I do want to investigate the space usage of Wallet and WalletState more systematically at some point, but not right now — things are working out ok at the moment, so I figured that I could away with just making sure that I don't accidentally make things worse for the time being.

paweljakubas · 2022-03-16T12:27:04Z

lib/core/test/unit/Cardano/Wallet/Primitive/BlockSummarySpec.hs

+    describe "Sublist" $ do
+        it "merging is idempotent" $
+            property prop_idempotent
+        it "merging has whole list as neutral element" $


if we have I=All and A=Some then mergeSublist I A = I is I neutral element really?

Oops, you're right, the nomenclature "neutral element" is not correct. 🙈 The correct name is "absorbing element".

paweljakubas · 2022-03-16T12:29:55Z

lib/core/test/unit/Cardano/Wallet/Primitive/BlockSummarySpec.hs

    , (===)
    )

 import Data.Map.Strict as Map

 spec :: Spec
 spec = do
+    describe "Sublist" $ do


I would also distinct the following properties:

for any two sublists of the same list the result of merge is sublist that contains both sublists, is union sublist.

merging is commutative

Ok! There's not much that can go wrong in this code, but these laws are aesthetically pleasing. 😊

I was about to write that property 2 follows from property 1 because set union is commutative, but property 2 is slightly stronger due to the order of elements. 🤔

paweljakubas · 2022-03-16T12:31:28Z

lib/core/test/unit/Cardano/Wallet/Primitive/BlockSummarySpec.hs

 {-------------------------------------------------------------------------------
    Generators
 -------------------------------------------------------------------------------}
+genSublist :: [a] -> Gen (Sublist a)
+genSublist xs = unsafeMkSublist <$> sublistOf (zip [0..] xs)


sublistOf is from Quickcheck, right?

paweljakubas · 2022-03-16T12:32:41Z

lib/core/test/unit/Cardano/Wallet/Primitive/ModelSpec.hs

+            discoverState (Summary discover summary) s
+            === discoverState (List blocks) s
+  where
+    -- TODO: Test sequence of blocks with more than one block.


yeah, would be great to expand this property in this direction

Ooh, nice, that's an older comment. In another pull request, I had started setting up this ApplyBlocks type for generating lists of blocks; I'll plug that in now.

paweljakubas

LGTM, the step should not be invasive and impact negatively the current block processing. I like Sublist abstraction and intention behind it that utilizes lazyness, and look inside the list contents selectively (if I understood intent correctly). Left some pointers/questions.

HeinrichApfelmus · 2022-03-16T16:06:21Z

Thanks Pawel! 😊

Sublist does not use laziness, though, it relies on sharing of data structures, which is enabled by immutability / purity. The point is that fromEntireBlock can use

transactions = All transactions

which copies a pointer to the list of transactions. The previous code

transactions = zip [0..] transactions

deconstructs the list of transactions, decorates each list element with its index, and constructs a new list — this is way more expensive, and since the transaction processing is part of a tight loop, of relevance.

Co-authored-by: paweljakubas <pawel.jakubas@iohk.io>

piotr-iohk · 2022-03-18T08:06:31Z

Started benchmarks against this branch -> https://buildkite.com/input-output-hk/cardano-wallet-nightly/builds/1289

HeinrichApfelmus · 2022-03-21T11:05:34Z

Benchmarks look entirely neutral. 👍

HeinrichApfelmus · 2022-03-21T11:05:39Z

bors merge

3172: Change `applyBlocks` to also accept `BlockSummary` r=HeinrichApfelmus a=HeinrichApfelmus ### Issue number ADP-1422, ADP-1428 ### Overview [Light-mode][] (Epic ADP-1422) aims to make synchronisation to the blockchain faster by trusting an off-chain source of aggregated blockchain data. [light-mode]: https://input-output-hk.github.io/cardano-wallet/design/specs/light-mode In this pull request, we change the `applyBlocks` function to work with both a list of `Block` and a `BlockSummary`. Specifically, we introduce a function `applyBlockData` which performs the following steps: * `discoverFromBlockData`: discover addresses and and transaction data (`ChainEvents`) contained in the sequence of blocks * `applyBlockEventsToUTxO`: update the wallet UTxO from the given `ChainEvents`. ### Details * Property tests for `discoverFromBlockData` check that address discovery gives the same result for both a `List` of blocks and a `Summary`. * When processing a `List` of `Block`, we have to be a bit careful about performance, because we need to traverse ~ 32M transactions (as of Feb 2022). In this case, the `transactions` contained in `BlockEvents` are essentially a full copy of the `transactions` in `Block`. The best way to copy data is to copy a single reference to the data; for this purpose we introduce a `Sublist` type which avoids deconstructing and reconstructing the transaction list in the case where we do not filter transactions (`All`). * In order to preserve the current checkpointing mechanism, `applyBlocks` actually calls `applyBlockData` repeatedly instead of passing the full list of blocks. I will need to rethink the checkpointing mechanism in the future — the main issue is that in its current form, a consumer of `BlockSummary` cannot ask for a specific block height in order to set a suitable checkpoint there. In the future, the checkpointing logic will need to mediate between producer and consumer. ### Comments * I expect a slight regression in benchmarks, due to overhead involved in calling `applyBlockData` repeatedly. However, the introduction of `Sublist` should prevent a serious deterioration. Co-authored-by: Heinrich Apfelmus <heinrich.apfelmus@iohk.io>

iohk-bors · 2022-03-21T12:12:03Z

Build failed:

ci/hydra-build:required

HeinrichApfelmus · 2022-03-21T12:14:43Z

bors retry

iohk-bors · 2022-03-21T14:04:26Z

Build succeeded:

HeinrichApfelmus added 3 commits March 15, 2022 13:22

Add discoverFromBlockData

ddc0f48

* Construct `ChainEvents` from `NonEmpty Block`. * Test that `discoverFromBlockData` yields the same address discovery state on `List` and on `Summary`, at least for the mock type `WalletState`.

Adjust applyBlockEventsToUTxO

081e515

…to work with `BlockEvents` instead of `Block`.

Optimize: New type Sublist in BlockEvents

278b9a8

`Sublist` = a sublist of a list, with an optimized case for the full list.

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1422/applyBlockEvents branch from 1d892d1 to 278b9a8 Compare March 15, 2022 13:31

iohk-bors bot added a commit that referenced this pull request Mar 15, 2022

Try #3172:

6e0155b

HeinrichApfelmus requested a review from paweljakubas March 15, 2022 19:17

paweljakubas reviewed Mar 16, 2022

View reviewed changes

lib/core/src/Cardano/Wallet.hs Show resolved Hide resolved

paweljakubas reviewed Mar 16, 2022

View reviewed changes

paweljakubas approved these changes Mar 16, 2022

View reviewed changes

Apply suggestions from code review

bfd7474

Co-authored-by: paweljakubas <pawel.jakubas@iohk.io>

iohk-bors bot merged commit 46772e1 into master Mar 21, 2022

iohk-bors bot deleted the HeinrichApfelmus/ADP-1422/applyBlockEvents branch March 21, 2022 14:04

WilliamKingNoel-Bot pushed a commit that referenced this pull request Mar 21, 2022

docs: Merge #3172

d1dcad4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `applyBlocks` to also accept `BlockSummary` #3172

Change `applyBlocks` to also accept `BlockSummary` #3172

HeinrichApfelmus commented Mar 15, 2022

HeinrichApfelmus commented Mar 15, 2022

iohk-bors bot commented Mar 15, 2022

paweljakubas Mar 16, 2022 •

edited

Loading

HeinrichApfelmus Mar 16, 2022 •

edited

Loading

paweljakubas Mar 16, 2022

HeinrichApfelmus Mar 16, 2022

paweljakubas Mar 16, 2022

HeinrichApfelmus Mar 16, 2022

paweljakubas Mar 16, 2022

HeinrichApfelmus Mar 16, 2022

paweljakubas Mar 16, 2022

HeinrichApfelmus Mar 16, 2022

paweljakubas Mar 16, 2022 •

edited

Loading

HeinrichApfelmus Mar 16, 2022

paweljakubas Mar 16, 2022

HeinrichApfelmus Mar 16, 2022

HeinrichApfelmus Mar 16, 2022

paweljakubas Mar 16, 2022

HeinrichApfelmus Mar 16, 2022

paweljakubas Mar 16, 2022

HeinrichApfelmus Mar 16, 2022

paweljakubas left a comment

HeinrichApfelmus commented Mar 16, 2022

piotr-iohk commented Mar 18, 2022

HeinrichApfelmus commented Mar 21, 2022

HeinrichApfelmus commented Mar 21, 2022

iohk-bors bot commented Mar 21, 2022

HeinrichApfelmus commented Mar 21, 2022

iohk-bors bot commented Mar 21, 2022

Change applyBlocks to also accept BlockSummary #3172

Change applyBlocks to also accept BlockSummary #3172

Conversation

HeinrichApfelmus commented Mar 15, 2022

Issue number

Overview

Details

Comments

HeinrichApfelmus commented Mar 15, 2022

iohk-bors bot commented Mar 15, 2022

try

paweljakubas Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

HeinrichApfelmus Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas left a comment

Choose a reason for hiding this comment

HeinrichApfelmus commented Mar 16, 2022

piotr-iohk commented Mar 18, 2022

HeinrichApfelmus commented Mar 21, 2022

HeinrichApfelmus commented Mar 21, 2022

iohk-bors bot commented Mar 21, 2022

HeinrichApfelmus commented Mar 21, 2022

iohk-bors bot commented Mar 21, 2022

Change `applyBlocks` to also accept `BlockSummary` #3172

Change `applyBlocks` to also accept `BlockSummary` #3172

paweljakubas Mar 16, 2022 •

edited

Loading

HeinrichApfelmus Mar 16, 2022 •

edited

Loading

paweljakubas Mar 16, 2022 •

edited

Loading