-
Notifications
You must be signed in to change notification settings - Fork 217
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
Add restore benchmark #157
Conversation
f8ed851
to
237fb73
Compare
-> IO () | ||
-- ^ Consume the entire available chain, applying block to the given | ||
-- wallet, and stop when finished. This function is intended for | ||
-- benchmarking. |
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.
This is very slippery slope that has caused us a lot of pain in the previous implementations. Let's not introduce code that is test-specific in our codebase. If something is hard to test, I'd rather review our source implementation than introduce tests work-around..
In the end, we do want to test our production code, and not some testing variation of it.
Here's what I'd suggest:
- Get rid of
watchWallet
&processWallet
altogether. - Introduce a new
restoreWallet :: WalletId -> (SlotId, SlotId) -> ExceptT RestoreWalletError IO ()
which would restore a wallet from a starting slot up to another slot. - We can pretty much re-use the existing
applyBlocks
to do so, and simply, add an extra round of getting the "next" slot from the network layer.
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.
Thanks for the lecture, I agree with not introducing test specific code, and that implementing tests improves the API - however -
- The problem with the previous implementation was a complete absence of benchmarks, not test-specific code.
- There are no call sites of
watchWallet
yet and at the moment there is no clear spec of how this function orrestoreWallet
will be used. I suggest fixing it not now but soon when the caller ofrestoreWallet
is written. - In the interests of small short-lived PRs, this change should be done in a separate PR.
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.
-
The problem was, unfortunately, also test-specific code. We had entire modules that were almost only present for testing. And functions with arbitrarily complex signatures to allow passing around custom data and short-circuit surrounding logic :( ...
-
& 3. True, hence why I think we could remove it. We probably added it too early for "testing" whereas in the end, there was still much work to do before that. However, your suggestion is sensible. I'll get it merge as it is and issue a follow-up PR 👍
GotChunk blocks -> do | ||
action start' blocks | ||
go start' | ||
Sleep -> pure () |
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.
The more we make changes to these interfaces, the more it makes me believe that we are doing something wrong and we should be defining functions in the network layer interface instead. I believe, everything we need could be achieved with a simple change in the network interface:
nextBlocks :: SlotId -> ExceptT e0 m Result
data Result
= NextBlocks !([Blocks], SlotId) -- ^ Have a result, a batch of next blocks and, the next Id to start from
| NoBlocks -- ^ There is no blocks available now, so wait.
This way, the chain producer implementation is charge of letting others know about what is the next slot id and, we can get rid of listen
, getNextBlocks
and drain
in favor of an approach that is also probably more resilient to the upcoming changes in the network protocol with Shelley.
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.
Sure - where do you envision the "listen" process sitting in relation to wallet layer and network layer?
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.
I'll make a proposal in another PR, it will be simpler to discuss around that 👍
rebased on top of master + removed unused dependencies in dot cabal. |
Relates to issue #100
Overview
Adds a benchmark for wallet restore, which require cardano-http-bridge already synced to mainnet and testnet.
Comments
This measures time but memory usage can be measured with
+RTS -h
.There is not yet the special
IsOurs
scheme which will cause many transactions to applied to the wallet.