Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Balance temporarily increases when spending rewards #1955

Closed
Anviking opened this issue Jul 26, 2020 · 6 comments
Closed

Balance temporarily increases when spending rewards #1955

Anviking opened this issue Jul 26, 2020 · 6 comments
Assignees
@KtorZ
Labels
Bug PRIORITY:MEDIUM A bug that needs to be addressed after ongoing stories and tasks. SEVERITY:MEDIUM Visible impact on a core functionality or some important performance degradation.

Comments

@Anviking
Copy link
Member

Anviking commented Jul 26, 2020 •
edited by KtorZ
Loading

Context

  • When spending rewards, we spend everything
  • Restoring a ITN wallet is an easy way to get a wallet with only rewards
  • Think Alan created a thread about this in #adrestia-dev some time ago
Information -
Version
Platform
Installation

Steps to Reproduce

  1. Have a wallet with a noticeable amount of rewards
  2. Send a outgoing tx

Expected behavior

  1. While the balance is pending, the balance should either stay the same or decrease

Actual behavior

  1. The wallet balance doubles when spending ada (for wallet with 99% rewards)
  2. Balance goes back to normal when the tx is confirmed

Resolution

QA

  • Added a property test showing that for any wallet, for any non empty set of pending transaction: the total balance with pending transaction is strictly smaller than the total balance without any pending. This simple assertion was actually failing without a fix here because the withdrawals would be counted twice when some pending transaction had withdrawals (raw and as part of pending change output).

https://github.com/input-output-hk/cardano-wallet/blob/905277f370093ab43c9246ee6d7c32df7114fe1d/lib/core/test/unit/Cardano/Wallet/Primitive/ModelSpec.hs#L233-L255

  • Other integration and unit test still pass after the fix without any change.
@Anviking Anviking mentioned this issue Jul 26, 2020
Merged
@KtorZ KtorZ self-assigned this Jul 27, 2020
KtorZ added a commit that referenced this issue Jul 27, 2020
@KtorZ
add test showing oddities on total balance + rewards when there are p…
41d1f76 
…ending withdrawals + fix it

  This was trickier than I originally thought because crafting a generator of pending transactions which is somewhat meaningful and can have withdrawals turned out to be a bit complex. The property originally failed (without the fix) and clearly showed the bug described in #1955. The fix is relatively straightforward and is about adding the reward amount only if there are no pending withdrawal
KtorZ added a commit that referenced this issue Jul 27, 2020
@KtorZ
add test showing oddities on total balance + rewards when there are p…
aa9fdf9 
…ending withdrawals + fix it

  This was trickier than I originally thought because crafting a generator of pending transactions which is somewhat meaningful and can have withdrawals turned out to be a bit complex. The property originally failed (without the fix) and clearly showed the bug described in #1955. The fix is relatively straightforward and is about adding the reward amount only if there are no pending withdrawal
iohk-bors bot added a commit that referenced this issue Jul 27, 2020
@iohk-bors @KtorZ
Merge #1954
a5d7c24 
1954: Fix reported amount on incoming transactions carrying withdrawals r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#1952 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- d1c5618
  📍 **modify scenario spending rewards to send money to _another_ wallet and assess right amount**
    We didn't quite catch a bug here because we were only looking at one side of the coin by sending the transaction to ourselves (so, we couldn't really assess the amount on an _incoming_ transaction, from a different perspective. That is now done properly such that 1 lovelace is send to another wallet (and, which should be the amount reported by this wallet). The outgoing amount however should contain any output sent to _another_ wallet, as well as fees.

- 6932d1d
  📍 **revise 'amount' calculation in the primitive model**
    Such that we only count withdrawals in outgoing transactions. In principle, withdrawals should be necessarily ours on a transaction.. but it's not excluded that a transaction may contain multiple withdrawals from different wallets (especially in the context of multisig). Beside, withdrawals should be completely out of the picture for _incoming_ transactions.

- aa9fdf9
  📍 **add test showing oddities on total balance + rewards when there are pending withdrawals + fix it**
    This was trickier than I originally thought because crafting a generator of pending transactions which is somewhat meaningful and can have withdrawals turned out to be a bit complex. The property originally failed (without the fix) and clearly showed the bug described in #1955. The fix is relatively straightforward and is about adding the reward amount only if there are no pending withdrawal

- 5867d38
  📍 **fix SlotNo generator in database arbitrary instances**
    This one was wrongly changed to browse the entire Word32 space instead of staying within small bounds as originally. There are many things in tests and other generators derived from the slot number value, and too big values can really do bad things here.

- 8d1eadb
  📍 **prevent users from creating two withdrawals simultaneously on the same wallet**
    Rewards ought to be spent fully, therefore, making two concurrent withdrawals has no sense since only one transaction will eventually be validated.


# 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]>
KtorZ added a commit that referenced this issue Jul 27, 2020
@KtorZ
add test showing oddities on total balance + rewards when there are p…
62b6358 
…ending withdrawals + fix it

  This was trickier than I originally thought because crafting a generator of pending transactions which is somewhat meaningful and can have withdrawals turned out to be a bit complex. The property originally failed (without the fix) and clearly showed the bug described in #1955. The fix is relatively straightforward and is about adding the reward amount only if there are no pending withdrawal
iohk-bors bot added a commit that referenced this issue Jul 27, 2020
@iohk-bors @KtorZ
Merge #1954
f9f4182 
1954: Fix reported amount on incoming transactions carrying withdrawals r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#1952 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- d1c5618
  📍 **modify scenario spending rewards to send money to _another_ wallet and assess right amount**
    We didn't quite catch a bug here because we were only looking at one side of the coin by sending the transaction to ourselves (so, we couldn't really assess the amount on an _incoming_ transaction, from a different perspective. That is now done properly such that 1 lovelace is send to another wallet (and, which should be the amount reported by this wallet). The outgoing amount however should contain any output sent to _another_ wallet, as well as fees.

- 6932d1d
  📍 **revise 'amount' calculation in the primitive model**
    Such that we only count withdrawals in outgoing transactions. In principle, withdrawals should be necessarily ours on a transaction.. but it's not excluded that a transaction may contain multiple withdrawals from different wallets (especially in the context of multisig). Beside, withdrawals should be completely out of the picture for _incoming_ transactions.

- aa9fdf9
  📍 **add test showing oddities on total balance + rewards when there are pending withdrawals + fix it**
    This was trickier than I originally thought because crafting a generator of pending transactions which is somewhat meaningful and can have withdrawals turned out to be a bit complex. The property originally failed (without the fix) and clearly showed the bug described in #1955. The fix is relatively straightforward and is about adding the reward amount only if there are no pending withdrawal

- 5867d38
  📍 **fix SlotNo generator in database arbitrary instances**
    This one was wrongly changed to browse the entire Word32 space instead of staying within small bounds as originally. There are many things in tests and other generators derived from the slot number value, and too big values can really do bad things here.

- 8d1eadb
  📍 **prevent users from creating two withdrawals simultaneously on the same wallet**
    Rewards ought to be spent fully, therefore, making two concurrent withdrawals has no sense since only one transaction will eventually be validated.


# 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]>
KtorZ added a commit that referenced this issue Jul 27, 2020
@KtorZ
add test showing oddities on total balance + rewards when there are p…
6584dd3 
…ending withdrawals + fix it

  This was trickier than I originally thought because crafting a generator of pending transactions which is somewhat meaningful and can have withdrawals turned out to be a bit complex. The property originally failed (without the fix) and clearly showed the bug described in #1955. The fix is relatively straightforward and is about adding the reward amount only if there are no pending withdrawal
iohk-bors bot added a commit that referenced this issue Jul 27, 2020
@iohk-bors @KtorZ
Merge #1954
905277f 
1954: Fix reported amount on incoming transactions carrying withdrawals r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#1952 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- d1c5618
  📍 **modify scenario spending rewards to send money to _another_ wallet and assess right amount**
    We didn't quite catch a bug here because we were only looking at one side of the coin by sending the transaction to ourselves (so, we couldn't really assess the amount on an _incoming_ transaction, from a different perspective. That is now done properly such that 1 lovelace is send to another wallet (and, which should be the amount reported by this wallet). The outgoing amount however should contain any output sent to _another_ wallet, as well as fees.

- 6932d1d
  📍 **revise 'amount' calculation in the primitive model**
    Such that we only count withdrawals in outgoing transactions. In principle, withdrawals should be necessarily ours on a transaction.. but it's not excluded that a transaction may contain multiple withdrawals from different wallets (especially in the context of multisig). Beside, withdrawals should be completely out of the picture for _incoming_ transactions.

- aa9fdf9
  📍 **add test showing oddities on total balance + rewards when there are pending withdrawals + fix it**
    This was trickier than I originally thought because crafting a generator of pending transactions which is somewhat meaningful and can have withdrawals turned out to be a bit complex. The property originally failed (without the fix) and clearly showed the bug described in #1955. The fix is relatively straightforward and is about adding the reward amount only if there are no pending withdrawal

- 5867d38
  📍 **fix SlotNo generator in database arbitrary instances**
    This one was wrongly changed to browse the entire Word32 space instead of staying within small bounds as originally. There are many things in tests and other generators derived from the slot number value, and too big values can really do bad things here.

- 8d1eadb
  📍 **prevent users from creating two withdrawals simultaneously on the same wallet**
    Rewards ought to be spent fully, therefore, making two concurrent withdrawals has no sense since only one transaction will eventually be validated.


# 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]>
@piotr-iohk
Copy link
Contributor

I have tried to test it also on MC4 with earned rewards.

  • while transaction is pending there is no increase in total amount
  • for a very brief period upon transaction transition into in_ledger I think I saw doubled total amount
  • then it was all OK when it was already in_ledger

Is it possible?

@Anviking
Copy link
Member Author

Anviking commented Jul 28, 2020 •
edited
Loading

That sounds like:

I'm wondering if there could still be strange behaviour from race-conditions from the fact that the reward-balance and wallet utxo are not guaranteed to be in-sync.

So I think that is currently possible for short periods of time.

@KtorZ
Copy link
Member

KtorZ commented Jul 28, 2020

Is it possible?

It is possible indeed. The reward balance update and the block processing happens concurrently in two separate workers. They're invoked roughly at the same time but possibly with a visible delay. As a result there could be a point in time where there's no more pending withdrawal, but the balance hasn't been updated to 0 yet.

We could "force" our cached balance to zero after processing a withdrawal (within the same database transaction) to prevent this.

@piotr-iohk
Copy link
Contributor

We could "force" our cached balance to zero after processing a withdrawal (within the same database transaction) to prevent this.

Shall we return it ticket to "in progress"?

@KtorZ
Copy link
Member

KtorZ commented Jul 28, 2020

Oui.

@KtorZ KtorZ added Bug SEVERITY:MEDIUM Visible impact on a core functionality or some important performance degradation. PRIORITY:MEDIUM A bug that needs to be addressed after ongoing stories and tasks. and removed BUG:MINOR labels Aug 12, 2020
@KtorZ KtorZ mentioned this issue Aug 25, 2020
Closed
@KtorZ
Copy link
Member

KtorZ commented Dec 10, 2020

Moved to; https://jira.iohk.io/browse/ADP-611

@KtorZ KtorZ closed this as completed Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@KtorZ KtorZ

Labels
Bug PRIORITY:MEDIUM A bug that needs to be addressed after ongoing stories and tasks. SEVERITY:MEDIUM Visible impact on a core functionality or some important performance degradation.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Anviking @KtorZ @piotr-iohk