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.

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

Fix calculation & display of Locked Funds #5813

Merged
merged 1 commit into from Nov 16, 2021
Merged

Fix calculation & display of Locked Funds #5813

merged 1 commit into from Nov 16, 2021

Conversation

ghost
Copy link

@ghost ghost commented Nov 8, 2021

Fixes #5703, #5574

Scenario:

If for any reason the deposit transaction is rejected by bitcoin nodes, Bisq thinks those funds are locked in a trade. Bisq does not receive any notification about rejected transactions -- once they are broadcast via BitcoinJ they are assumed to be valid even though this may not be the case. This can happen when for example if the user's wallet is out of sync and the deposit transaction is attempting to use already spent funds. The only indication that something is wrong is that the deposit transaction never confirms (and if they look it up on a blockchain explorer the txId will not be found). At this point Bisq will erroneously show the deposit amount in "Locked Funds" on the main screen.

This can be fixed by considering funds to be locked once the deposit transaction is confirmed on the blockchain.


Code change:

We change one line in the method isFundsLockedIn() replacing isDepositPublished with isDepositConfirmed.

public boolean isFundsLockedIn() {

In order to determine the possible impacts of this, here is a usage analysis of the altered method 'isFundsLockedIn'.
It is used in the following 3 locations:

Funds -> Locked Funds Tab:
TradeManager.java +769 method getTradesStreamWithFundsLockedIn
-> LockedView.java +224
-> updates the amounts shown in Funds -> Locked Funds (active trades only)

Main Screen Locked Balance:
TradeManager.java +769 method getTradesStreamWithFundsLockedIn
ClosedTradableManager.java +156 method getTradesStreamWithFundsLockedIn
FailedTradesManager.java +117 method getTradesStreamWithFundsLockedIn
-> Balances.java +123
-> sets the locked balance amount shown at the top right of the main screen

Locked Funds Popup:
ClosedTradableManager.java +156 method getTradesStreamWithFundsLockedIn
FailedTradesManager.java +117 method getTradesStreamWithFundsLockedIn
Each of the above are used in the following two use cases:
-> TradeManager.java +772 method getSetOfFailedOrClosedTradeIdsFromLockedInFunds
-> BisqSetup.java method checkForLockedUpFunds pops up a user directed warning message "you have locked up funds from a failed trade"

@chimp1984
Copy link
Contributor

chimp1984 commented Nov 9, 2021

NACK

This will not fix it as the call checks if the state.ordinal is >= DEPOSIT_PUBLISHED so DEPOSIT_CONFIRMED is included in that as well.

You probably need to check the tx from the wallet to see if its really confirmed/available

   public boolean isDepositPublished() {
        return getTradePhase().ordinal() >= Phase.DEPOSIT_PUBLISHED.ordinal();
    }


 public enum Phase implements TradePhase {
        INIT,
        TAKER_FEE_PUBLISHED,
        DEPOSIT_PUBLISHED,
        DEPOSIT_CONFIRMED,
        FIAT_SENT,
        FIAT_RECEIVED,
        PAYOUT_PUBLISHED,
        WITHDRAWN;

@ripcurlx
Copy link
Contributor

@jmacxx Did you have time already to look at @chimp1984 's remark?

@ghost
Copy link
Author

ghost commented Nov 12, 2021

I did test this fix while developing it. I was going to return and re-do the tests before commenting again. They do take a few hours to run and I still have a PR to finish work on before I can get to this.

@ghost
Copy link
Author

ghost commented Nov 13, 2021

The easiest consistent way to reproduce the locked up funds issue described in #5703, is by not signing one of the deposit transaction inputs causing it to be rejected by bitcoind. Achieve that in test commenting out TradeWalletService L645-L646.

You end up with a trade that looks ok, but its deposit tx never gets a confirm.

At that point you'll decide to do an SPV resync, reconciling BitcoinJ state with the blockchain. Upon restarting Bisq you'll encounter the following:

  • Locked funds amount showing the trade deposit value.
  • A popup which says the deposit tx has missing inputs.
  • Red failed icons in the pending trades screen.
  • Nothing shown in Funds -> Locked funds tab.
Screenshots - click to expand

failed_trade_popup_missing_deposit

failed_trade_locked_funds

failed_trade_details

failed_trade_move_to

Moving that trade to Failed Trades shows the dialog which says there are no locked funds, contradicting what is shown in the top right of the screen at Locked Funds display.

I re-did my testing to obtain these screenshots.


After the patch is applied, the Locked Funds no longer displays the fake locked amount.

confirmed deposit == locked funds.

image

@chimp1984
Copy link
Contributor

Ah sorry my earlier comment was wrong. As we use the negation its a difference if the state is published or confirmed for that case.
Could you check what state the trade is in? Probably in pusblished state but not confirmed state...

@ghost
Copy link
Author

ghost commented Nov 15, 2021

Yes published, not confirmed.

Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@chimp1984
Copy link
Contributor

Yes published, not confirmed.

Ok. From my understanding it should be ok then.

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK - based on #5813 (comment)

@pazza83
Copy link

pazza83 commented Nov 21, 2021

Hi @jmacxx will this fix users with historic locked funds showing? Assuming they will have to do an spv resync?

@ghost
Copy link
Author

ghost commented Nov 21, 2021

Yes.

@ghost ghost deleted the fix_locked_funds_display branch May 29, 2022 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Locked balance amount in menu bar is not consistent with "Locked Funds" tab
3 participants