Fix Input resolution for inputs only present in db #1685
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KtorZ
commented
May 25, 2020
| ) | ||
| => (Context t -> [Natural] -> IO ApiByronWallet) | ||
| -> SpecWith (Context t) | ||
| scenario_TRANS_REG_1670 fixture = it title $ \ctx -> do |
There was a problem hiding this comment.
This fails before the fix.
There was a problem hiding this comment.
Since the tx input resolving is moved into the DBLayer, this test could probably be more easily done at the unit level.
KtorZ
commented
May 25, 2020
| -- to assemble coin selection information for outgoing payments. | ||
| -- To reliably provide this information, it should be looked up when | ||
| -- applying blocks, but that is future work (issue #573). | ||
| assemble |
There was a problem hiding this comment.
no longer needed, the db now returns [TransactionInfo] directly.
There was a problem hiding this comment.
Is it still noted somewhere that coin selection information can't (yet) be provided for outgoing payments?
There was a problem hiding this comment.
I've rewritten the comment and linked to the github issue in the relevant place in the db layer 👍 , good point
rvl
approved these changes
May 26, 2020
| ) | ||
| => (Context t -> [Natural] -> IO ApiByronWallet) | ||
| -> SpecWith (Context t) | ||
| scenario_TRANS_REG_1670 fixture = it title $ \ctx -> do |
There was a problem hiding this comment.
Since the tx input resolving is moved into the DBLayer, this test could probably be more easily done at the unit level.
| -- to assemble coin selection information for outgoing payments. | ||
| -- To reliably provide this information, it should be looked up when | ||
| -- applying blocks, but that is future work (issue #573). | ||
| assemble |
There was a problem hiding this comment.
Is it still noted somewhere that coin selection information can't (yet) be provided for outgoing payments?
KtorZ
added
the
RESOLVING ISSUE
|
bors r+ |
|
bors r- |
Canceled |
|
Whoops. I failed to push because there was a change upstream I didn't see. Rebasing and pushing. |
KtorZ
force-pushed
the
KtorZ/1670/input-resolution-fix
branch
from
ad7f8f9 to
67adc07
Compare
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
May 26, 2020
1685: Fix Input resolution for inputs only present in db r=KtorZ a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #1670 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - 1502136 📍 **add regression test illustrating the issue** Input that should be resolved aren't. In order to resolve inputs, we are actually wrongly using the transactions available from the history that was fetched from the database, instead of using the entire database! So, giving different query parameters restricting the set of outputs has an influence on something it shouldn't. - 8ec0123 📍 **resolve transaction inputs from db when fetching the history** Before, this was done in the application layer, from the available history ('assemble' in 'Cardano.Wallet'). This requires exactly two extra lookups. One in the TxOut table, and one in the Checkpoint table so the performance impact should be quite small. It simplifies the application logic quite a bit and doesn't much impact the database logic, so it's almost a win-win. Plus, it fixes the issue where resolvable inputs would appear as non resolved - 64bf4db 📍 **fix DB model to also return full 'TransactionInfo' when fetching the tx history** - 21d75d2 📍 **Fix db benchmark build** Benchmarks may produce higher timings after this change, but it's OK since the answers are more correct now, and the work has just shifted from the wallet layer into the db layer. - fffe476 📍 **re-add lost comment about input resolution for incoming/outgoing transactions** - 67adc07 📍 **rename 'txs' -> 'pending' to better match context** # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>
Build failed |
Input that should be resolved aren't. In order to resolve inputs, we are actually wrongly using the transactions available from the history that was fetched from the database, instead of using the entire database! So, giving different query parameters restricting the set of outputs has an influence on something it shouldn't.
Before, this was done in the application layer, from the available history ('assemble' in 'Cardano.Wallet'). This requires exactly two extra lookups. One in the TxOut table, and one in the Checkpoint table so the performance impact should be quite small. It simplifies the application logic quite a bit and doesn't much impact the database logic, so it's almost a win-win. Plus, it fixes the issue where resolvable inputs would appear as non resolved
Benchmarks may produce higher timings after this change, but it's OK since the answers are more correct now, and the work has just shifted from the wallet layer into the db layer.
KtorZ
force-pushed
the
KtorZ/1670/input-resolution-fix
branch
from
67adc07 to
7c085f9
Compare
|
bors r+ |
Build succeeded |
iohk-bors bot
added a commit
that referenced
this pull request
May 27, 2020
1696: sql: Fix input resolution when number of inputs is large r=KtorZ a=rvl ### Issue Number #1670 / #1685 ### Overview The "too many SQL variables" problem strikes again in the nightly benchmark. One way to fix it is to do a SQL join or subquery. Another way to fix it (this PR) is to make the select query in chunks. ### Comments Tested with: cabal run cardano-wallet-core:bench:db -- --match prefix "TxHistory (Read)/1000" or stack bench cardano-wallet-core:bench:db --ba '--match prefix "TxHistory (Read)/1000"' Co-authored-by: Rodney Lorrimar <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Number
#1670
Overview
1502136
📍 add regression test illustrating the issue
Input that should be resolved aren't. In order to resolve inputs, we are actually wrongly using
the transactions available from the history that was fetched from the database, instead of using
the entire database! So, giving different query parameters restricting the set of outputs has an
influence on something it shouldn't.
8ec0123
📍 resolve transaction inputs from db when fetching the history
Before, this was done in the application layer, from the available history ('assemble' in 'Cardano.Wallet'). This requires exactly two extra lookups. One in the TxOut table, and one in the Checkpoint table so the performance impact should be quite small. It simplifies the application logic quite a bit and doesn't much impact the database logic, so it's almost a win-win. Plus, it fixes the issue where resolvable inputs would appear as non resolved
64bf4db
📍 fix DB model to also return full 'TransactionInfo' when fetching the tx history
21d75d2
📍 Fix db benchmark build
Benchmarks may produce higher timings after this change, but it's OK
since the answers are more correct now, and the work has just shifted
from the wallet layer into the db layer.
fffe476
📍 re-add lost comment about input resolution for incoming/outgoing transactions
67adc07
📍 rename 'txs' -> 'pending' to better match context
Comments