-
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
Different behavior regarding resolved inputs when listing transactions with and without query parameters #1670
Comments
Going quickly through the code, one of the reason might be: -- This relies on DB.readTxHistory returning all necessary transactions
-- to assemble coin selection information for outgoing payments.
assemble So, we the history returned is used both as the result, but also as an index for resolving transaction inputs. Therefore, if a transaction input refers to a known transaction that is discarded by the range parameters, then it won't be present in the return history and will fail the lookup. Making Another alternative could be to list transactions as already resolved from the database with an inner join; haven't quite thought about it but it seems doable from the top of my head and should not induce that much overhead. cc @piotr-iohk : if this intuition is correct ☝️ , we should be able to reproduce this by making two transactions, the second one using at least one output of the first one, and then, by listing all transactions and listing all transaction with a range only including the second transaction. |
Here is an example case from my test wallet
As you can see the first transaction( If I fetch the transactions without
|
Indeed I can reproduce: I have 3 outgoing txs on my wallet: TC0: Listing all transactions. => The amount is shown for every input for each tx.
TC1: List txs with --start 2020-05-18T14:15:36Z ( start before tx id1) => Amount for tx id1 is missing.
TC2: List txs with --start 2020-05-18T14:20:00Z (before tx id2) => Amount for tx id2 is missing.
TC3: List txs with --start 2020-05-18T14:30:00Z (before tx id3) => Amount for tx id3 is missing.
Conclusion: amount is not displayed for the first outgoing transaction that is shown after Can be observed for wallet on byron testnet: |
Similar issue, but for incoming transactions: #573. |
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]>
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]>
Looking at my previous example #1670 (comment) the original issue is no longer the case. I.e. the amount is being shown when one uses I noticed however that there is some discrepancy on transaction input amounts before and after the fix. I mean transactions on the wallet that is restored on recent master are different from tx on the wallet restored on revision before the fix (0318359) Looking at the transaction from my previous example Before the fix: After the fix: |
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]>
1697: actually identify output by their transaction AND output index... 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 --> The transaction id doesn't uniquely identify an output! So, if a transaction had several output, they'd all be conflated under the same key... 🤦 I wonder why the state machine didn't catch that though.. because the database model was doing it correctly by using the full `TxIn` as a key. # 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: Piotr Stachyra <[email protected]>
1697: actually identify output by their transaction AND output index... 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 --> The transaction id doesn't uniquely identify an output! So, if a transaction had several output, they'd all be conflated under the same key... 🤦 I wonder why the state machine didn't catch that though.. because the database model was doing it correctly by using the full `TxIn` as a key. # 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: Piotr Stachyra <[email protected]>
@piotr-iohk , should be fixed by #1697 now 👌 |
lgtm. |
Context
Reported by an exchange on Slack.
Steps to Reproduce
see #1685
Expected behavior
Responses are coherent with and without
start
/end
Actual behavior
Responses are different.
Resolution
QA
TRANS_REG_1670
The text was updated successfully, but these errors were encountered: