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 race condition in BTC sent confirmation popup #5333

Merged
merged 2 commits into from Mar 22, 2021
Merged

Fix race condition in BTC sent confirmation popup #5333

merged 2 commits into from Mar 22, 2021

Conversation

ghost
Copy link

@ghost ghost commented Mar 19, 2021

Fixes #5331, Fixes #5305

There is a race condition with the BTC display of the sent confirmation popup. The address field and senders amount were obtained from GUI input elements which are cleared after a transaction has been sent. So the fields could be empty when the confirmation popup is shown after the wallet calls back with success.

Solution is to move a variable sendersAmount from class scope to method scope and make it final. Likewise the withdrawToAddress must be defined in method scope so that the method does not access the GUI input field after it has been cleared.

The problem does not occur for displaying BSQ sent transactions, because the relevant parameters are already method-scoped and immutable.


Testing

The issue can be reproduced consistently if you introduce a delay of 1 second in the processing of the wallet OnSuccess callback.

// testing fix for #5331 [delay the async popup by 1 sec]
UserThread.runAfter(() -> {
    new TxDetails(transaction.getTxId().toString(), withdrawToAddress, formatter.formatCoinWithCode(sendersAmount))
            .dontShowAgainId(key)
            .show();
}, 1, TimeUnit.SECONDS);

It helps verify that the fix works correctly. (The delay was for testing/verification only, not included in PR).

@ripcurlx ripcurlx added this to the v1.6.0 milestone Mar 19, 2021
@ghost
Copy link
Author

ghost commented Mar 20, 2021

Tested & ready for review.

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.

ACK - Tested it on Regtest

Before
Bildschirmfoto 2021-03-22 um 11 50 15

After
Bildschirmfoto 2021-03-22 um 11 57 35

@ripcurlx ripcurlx merged commit 0d2db39 into bisq-network:master Mar 22, 2021
@ghost ghost mentioned this pull request Apr 2, 2021
@ghost ghost deleted the fix_withdrawal_txdetails_popup branch May 29, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant