a582b4141f gui: send, left alignment for "bytes" and "change" label (furszy)
210ef1e980 qt: remove confusing "Dust" label from coincontrol / sendcoins dialog (Sebastian Falbesoner)
Pull request description:
In contrast to to all other labels on the coin selection dialog, the displayed dust information has nothing to do with the selected coins. All that this label shows is whether at least one of the _outputs_ qualify as dust, but the outputs are set in a different dialog. (Even worse, the dust check is currently simply wrong because it only looks at an output's nValue and just assumes a P2PKH script size.)
As the label clearly doesn't help the user and is, quite the contrary, rather increasing confusion/misguidance, it seems sensible to remove it. The label from the sendcoins dialog is also removed with the same rationale. Additionally, the "bytes" and "change" labels are aligned to the left (second commit).
Closes https://github.com/bitcoin-core/gui/issues/699.
ACKs for top commit:
furszy:
ACK a582b41
hebasto:
Looks good. ACK a582b4141f.
Tree-SHA512: ebc00b68bdeab69f6ab643e4b89301a7e3d04a8a4027b50813314ddddb1387bc97a83313851e375dfbce97751c234686c82af7f4e55fa5ef29f4fed4e8fc11d9
In contrast to to all other labels on the coin selection dialog, the
displayed dust information has nothing to do with the selected coins.
All that this label shows is whether at least one of the _outputs_
qualify as dust, but the outputs are set in a different dialog.
(Even worse, the dust check is currently simply wrong because it only
looks at an output's nValue and just assumes a P2PKH script size.)
As the label clearly doesn't help the user and is, quite the contrary,
rather increasing confusion/misguidance, it seems sensible to remove it.
Also, remove the label from the sendcoins dialog with the same rationale.
The previous behavior for getAvailableBalance when coin control
has selected coins was to return the sum of them. Instead, we
are currently returning the wallet's available total balance minus
the selected coins total amount.
This turns into a GUI-only issue for the "use available balance"
button when the user manually select coins in the send screen.
Reason:
We missed to update the GetAvailableBalance function to include
the coin control selected coins on #25685.
Context:
Since #25685 we skip the selected coins inside `AvailableCoins`,
the reason is that there is no need to traverse the wallet's
txes map just to get coins that can directly be fetched by
their id.
9a1d73fdff Fix segfault when shutdown during wallet open (John Moffett)
Pull request description:
Fixes #689
## Summary
If you open a wallet and send a shutdown signal during that process, you'll get a segfault when the wallet finishes opening. That's because the `WalletController` object gets deleted manually in bitcoin.cpp during shutdown, but copies of the pointer (and pointers to child objects) are dangling in various places and are accessed in queued events after the deletion.
## Details
The issue in #689 is caused by the following sequence of events:
1. Wallet open modal dialog is shown and worker thread does the actual work.
2. Every 200ms, the main event loop checks to see if a shutdown has been requested, but only if a modal is not being shown.
3. Request a shutdown while the modal window is shown.
4. The wallet open process completes, the modal window is dismissed, and various `finish` signals are sent.
5. During handling of one of the `finish` signals, `qApp->processEvents()` is [called](e9262ea32a/src/qt/sendcoinsdialog.cpp (L603)), which causes the main event loop to detect the shutdown (now that the modal window has been dismissed). The `WalletController` and all the `WalletModel`s are [deleted](65de8eeeca/src/qt/bitcoin.cpp (L394-L401)).
6. Control returns to the `finish` method, which eventually tries to send a [signal](e9262ea32a/src/qt/sendcoinsdialog.cpp (L167)) from a wallet model, but it's been deleted already (and the signal is sent from a now-[dangling](d8bdee0fc8/src/qt/walletview.cpp (L65)) pointer).
The simplest fix for that is to change the `qApp->processEvents()` into a `QueuedConnection` call. (The `qApp->processEvents() was a [workaround](https://github.com/bitcoin/bitcoin/pull/593#issuecomment-3050699) to get the GUI to scroll to the last item in a list that just got added, and this is just a safer way of doing that).
However, once that segfault is fixed, another segfault occurs due to some queued wallet events happening after the wallet controller object is deleted here:
65de8eeeca/src/qt/bitcoin.cpp (L394-L401)
Since `m_wallet_controller` is a copy of that pointer in `bitcoingui.cpp`, it's now dangling and `if(null)` checks won't work correctly. For instance, this line:
65de8eeeca/src/qt/bitcoingui.cpp (L413)
sets up a `QueuedConnection` to `setCurrentWallet`, but by the time control reaches that method (one event cycle after shutdown deleted `m_wallet_controller` in `bitcoin.cpp`), the underlying objects have been destroyed (but the pointers are still dangling).
Ideally, we'd use a `QPointer` or `std::shared_ptr / std::weak_ptr`s for these, but the changes would be more involved.
This is a minimal fix for the issues. Just set `m_wallet_controller` to `nullptr` in `bitcoingui.cpp`, check its value in a couple places, and avoid a use of `qApp->processEvents`.
ACKs for top commit:
hebasto:
ACK 9a1d73fdff, I have reviewed the code and it looks OK.
furszy:
ACK 9a1d73fdff
Tree-SHA512: a1b94676eb2fcb7606e68fab443b1565b4122aab93c35382b561842a049f4b43fecc459535370d67a64d6ebc4bcec0ebcda981fff633ebd41bdba6f7093ea540
52f4d567d6 refactor: remove <util/system.h> include from wallet.h (furszy)
6c9b342c30 refactor: wallet, remove global 'ArgsManager' access (furszy)
d8f5fc4462 wallet: set '-walletnotify' script instead of access global args manager (furszy)
3477a28dd3 wallet: set keypool_size instead of access global args manager (furszy)
Pull request description:
Structurally, the wallet class shouldn't access the global `ArgsManager` class, its internal behavior shouldn't be coupled to a global command line args parsing object.
So this PR migrates the only two places where we depend on it: (1) the keypool size, and (2) the "-walletnotify" script. And cleans up the, now unneeded, wallet `ArgsManager` ref member.
Extra note:
In the process of removing the args ref member, discovered and fixed files that were invalidly depending on the wallet header including `util/system.h`.
ACKs for top commit:
achow101:
ACK 52f4d567d6
TheCharlatan:
Re-ACK 52f4d567d6
hebasto:
re-ACK 52f4d567d6
Tree-SHA512: 0cffd99b4dd4864bf618aa45aeaabbef2b6441d27b6dbb03489c4e013330877682ff17b418d07aa25fbe1040bdf2c67d7559bdeb84128c5437bf0e6247719016
Since we no longer store a ref to the global `ArgsManager`
inside the wallet, we can move the util/system.h
include to the cpp.
This dependency removal opened a can of worms, as few
other places were, invalidly, depending on the wallet's
header including it.
fdb8dc8a5a gui: Show watchonly balance only for Legacy wallets (Andrew Chow)
Pull request description:
Descriptor wallets do not have a watchonly balance as wallets are designated watchonly or not. Thus we should not be displaying the empty watchonly balance for descriptor wallets.
The result is that instead of the send page showing "Watch-only balance: 0.00000000 BTC" for watchonly descriptor wallets, we see the actual balance as "Balance: 10.00000000 BTC"
ACKs for top commit:
johnny9:
tACK fdb8dc8a5a
furszy:
ACK fdb8dc8a
hebasto:
ACK fdb8dc8a5a
Tree-SHA512: e5c0703a62d25c881c8dadfb9cffd482791f3d437a4ec5ae0088ce1a2069c2455ad6d3ec6c95a4404a3b55fbd727f92694529c35052236951553ca90c4ed31b5
7b7cd11244 clang-tidy, qt: Force checks for headers in `src/qt` (Hennadii Stepanov)
69eacf2c5e clang-tidy, qt: Fix `modernize-use-default-member-init` in headers (Hennadii Stepanov)
Pull request description:
This PR split from bitcoin/bitcoin#26705 and contains only changes in `src/qt`.
Effectively, it fixes the clang-tidy's `modernize-use-default-member-init` errors, and forces clang-tidy checks for all headers in the `src/qt` directory.
ACKs for top commit:
jarolrod:
ACK 7b7cd11244
Tree-SHA512: 79525bb0f31ae7cad88c781e55091a21467c0485ddc1ed03ad62e051480fda3b3710619ea11af480437edba3c6e038f7c40edc6b373e3a37408c006d11b34686
If you open a wallet and send a shutdown signal during
that process, the GUI will segfault due to some queued
wallet events happening after the wallet controller
is deleted. This is a minimal fix for those issues.
no need to waste resources calculating the wallet available coins if
they are not going to be used.
The 'm_allow_other_inputs=true` default value change is to correct
an ugly misleading behavior:
The tx creation process was having a workaround patch to automatically
fall back to select coins from the wallet if `m_allow_other_inputs=false`
(previous default value) and no manual inputs were selected.
This could be seen in master in flows like `sendtoaddress`, `sendmany`
and even the GUI, where the `m_allow_other_inputs` value isn't customized
and the wallet still selects and adds coins to the tx internally.
4584d300a4 GUI: remove now unneeded 'm_balances' field from overviewpage (furszy)
050e8b1391 GUI: 'getAvailableBalance', use cached balance if the user did not select UTXO manually (furszy)
96e3264a82 GUI: use cached balance in overviewpage and sendcoinsdialog (furszy)
321335bf02 GUI: add getter for WalletModel::m_cached_balances field (furszy)
e62958dc81 GUI: sendCoinsDialog, remove duplicate wallet().getBalances() call (furszy)
Pull request description:
As per the title says, we are recalculating the entire wallet balance on different situations calling to `wallet().getBalances()`, when should instead make use of the wallet model cached balance.
This has the benefits of (1) not spending resources calculating a balance that we already have cached, and (2) avoid blocking the main thread for a long time, in case of big wallets, walking through the entire wallet's tx map more than what it's really needed.
Changes:
1) Fix: `SendCoinsDialog` was calling `wallet().getBalances()` twice during `setModel`.
2) Use the cached balance if the user did not select any UTXO manually inside the wallet model `getAvailableBalance` call.
-----------------------
As an extra note, this work born in [#25005](https://github.com/bitcoin/bitcoin/pull/25005) but grew out of scope of it.
ACKs for top commit:
jarolrod:
ACK 4584d300a4
hebasto:
re-ACK 4584d300a4, only suggested changes and commit message formatting since my [recent](https://github.com/bitcoin-core/gui/pull/598#pullrequestreview-1071268192) review.
Tree-SHA512: 6633ce7f9a82a3e46e75aa7295df46c80a4cd4a9f3305427af203c9bc8670573fa8a1927f14a279260c488cc975a08d238faba2e9751588086fea1dcf8ea2b28
Descriptor wallets do not have a watchonly balance as wallets are
designated watchonly or not. Thus we should not be displaying the empty
watchonly balance for descriptor wallets.
Plus, calculate the cached balance right when the wallet model, so the wallet widgets don't need to redo the same balance calculation multiple times when they are waiting for the model balance polling timer.
----------------------------------------------------------------------
test wise: `WalletTests` now need to trigger the walletModel balance changed manually. So the model updates its internal state and can be used by the widgets.
This is because the test does not start the balance polling timer, in the same way as does not initialize several parts of the GUI workflow. All the objects (wallet, models, views, etc) that are used on this test are manually created instead of using the `WalletController` class flow.
Rationale is that this unit test is focused on verifying the GUI widgets/views behavior only: update the presented information, etc. when they receive different signals and/or function calls from outside (in other words, focus is on the signal slots/receiver side). It's not about whether the wallet balance polling timer is functioning as expected or not (which we definitely create a new test case for it in a follow-up work).
Inside setModel, we call 'wallet().getBalances()', to set the view balance,
right before calling 'updateDisplayUnit' which calls 'wallet().getBalances()'
internally and re-sets the view balance again.
1f653dc262 qt, wallet, refactor: Make `WalletModel::sendCoins()` return `void` (Hennadii Stepanov)
Pull request description:
Currently, the `WalletModel::sendCoins()` function always returns the same value.
Also dead and noop (calling `processSendCoinsReturn(OK)`) code has been removed.
The other `return` statements have been removed from the `WalletModel::sendCoins()` function in bitcoin/bitcoin#17154 and bitcoin/bitcoin#17165.
ACKs for top commit:
kristapsk:
cr ACK 1f653dc262
furszy:
Code review ACK 1f653dc2
shaavan:
Code Review ACK 1f653dc262
w0xlt:
Code Review ACK 1f653dc262
Tree-SHA512: 2b59495a7fc10b4de30fcc63fc3af92d50406e16031112eb72494736dce193ac1fbac0802623496cf81edcd16766e1647d9c4f3a607b3eb84cc50e273b999c04
0e5dedbc9e qt/wallettests: sort includes (William Casarin)
0554251d66 qt: Skip displayUnitChanged signal if unit is not actually changed (Hennadii Stepanov)
ffbc2fe459 qt, refactor: Remove default cases for scoped enum (Hennadii Stepanov)
152d5bad50 qt, refactor: Remove BitcoinUnits::valid function (Hennadii Stepanov)
aa23960fdf qt, refactor: Make BitcoinUnits::Unit a scoped enum (Hennadii Stepanov)
75832fdc37 qt: Use QVariant instead of int for BitcoinUnit in QSettings (Hennadii Stepanov)
Pull request description:
This is a rebased version of #60
Since Qt 5.5 there are [means](https://doc.qt.io/qt-5/qobject.html#Q_ENUM) to register an enum type with the meta-object system (such enum still lacks an ability to interact with [QSettings::setValue()](https://doc.qt.io/qt-5/qsettings.html#setValue) and [QSettings::value()](https://doc.qt.io/qt-5/qsettings.html#value) without defined stream operators).
In order to reduce global namespace polluting and to force strong type checking, this PR makes BitcoinUnits::Unit a scoped enum (typedef BitcoinUnits::Unit BitcoinUnit;).
No behavior change.
ACKs for top commit:
jonatack:
ACK 0e5dedbc9e, review and debug build of each commit after rebase on current master, lightly tested running the GUI, changing units a few times, and verifying persistence after restarting
promag:
Code review ACK 0e5dedbc9e
Tree-SHA512: 39ec0d7e4f0b9b25be287888121a8db6b282339674e37ec3a3554da63a9e22d6fe079e8310ca289b2a0356a19b3c7e55afa17d09dd34e0f222177f603bb053a3
Before this change the send confirmation dialog would keep the Send option disabled. The Create Unsigned choice would actually send. This is potentially confusing.
With this change the Create Unsigned button will not attempt to sign and always produce a PSBT. The Send button will attempt to sign, and only return a PSBT if more signatures are needed.
When using an external signer, the Create Unsigned option only appears when PSBT controls are enabled in the wallet settings.
This commit maintains the pre-existing behavior of filling the PSBT (without signing) even when not using an external signer.
Closes #551
Co-authored-by: Jon Atack <jon@atack.com>
-BEGIN VERIFY SCRIPT-
sed -i 's/ShowModalDialogAndDeleteOnClose/ShowModalDialogAsynchronously/' -- $(git grep -l -e "ShowModalDialogAndDeleteOnClose")
-END VERIFY SCRIPT-
It is important to highlight that a modal dialog is showed
asynchronously as there are cases when the synchronous QDialog::exec()
is required.
Warning: Replacing fs::system_complete calls with fs::absolute calls
in this commit may cause minor changes in behaviour because fs::absolute
no longer strips trailing slashes; however these changes are believed to
be safe.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
e5b6aef612 Move CBlockFileInfo::ToString method where class is declared (Russell Yanofsky)
f7086fd8ff Add src/wallet/* code to wallet:: namespace (Russell Yanofsky)
90fc8b089d Add src/node/* code to node:: namespace (Russell Yanofsky)
Pull request description:
There are no code changes, this is just adding `namespace` and `using` declarations and `node::` or `wallet::` qualifiers in some places.
Motivations for this change are:
- To make it easier to see when node and wallet code is being accessed places where it shouldn't be. For example if GUI code is accessing node and wallet internals or if wallet and node code are referencing each other.
- To make source code organization clearer ([#15732](https://github.com/bitcoin/bitcoin/issues/15732)), being able to know that `wallet::` code is in `src/wallet/`, `node::` code is in `src/node/`, `init::` code is in `src/init/`, `util::` code is in `src/util/`, etc.
Reviewing with `git log -p -n1 -U0 --word-diff-regex=.` can be helpful to verify this is only updating declarations, not changing code.
ACKs for top commit:
achow101:
ACK e5b6aef612
MarcoFalke:
Concept ACK e5b6aef612🍨
Tree-SHA512: 3797745c90246794e2d55a2ee6e8b0ad5c811e4e03a242d3fdfeb68032f8787f0d48ed4097f6b7730f540220c0af99ef423cd9dbe7f76b2ec12e769a757a2c8d
742918c8ef qt: hide Create Unsigned button behind an expert mode option (Andrew Chow)
5c3b800acd qt: Add Create Unsigned button to SendConfirmationDialog (Andrew Chow)
Pull request description:
Instead of having different buttons or changing button behavior for making a PSBT, just have SendConfirmationDialog return whether the user wants a PSBT or a broadcasted transaction. Since this dialog is used by both the bumpFeeAction and the SendCoinsDialog, changes to both to support the different behavior is needed. They will check the return value of the SendConfirmationDialog for whether a PSBT needs to be created instead of checking whether private keys are disabled.
Strings used in this dialog are being slightly modified to work with both private keys enabled and disabled wallets.
Moved from https://github.com/bitcoin/bitcoin/pull/18789
ACKs for top commit:
jarolrod:
ACK 742918c
ryanofsky:
Code review ACK 742918c8ef. Just suggested changes since last review. Looks great!
hebasto:
ACK 742918c8ef, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: dd29f4364c7b4f15befe8fe63257b26187918786b005e0f8336183270b1a162680b93f6ced60f0285c6e607c084cc0d24950fc68a8f9c056521ede614041be66
Instead of having different buttons or changing button behavior for
making a PSBT, just have SendConfirmationDialog return whether the user
wants a PSBT or a broadcasted transaction. Since this dialog is used
by both the bumpFeeAction and the SendCoinsDialog, changes to both
to support the different behavior is needed. They will check
the return value of the SendConfirmationDialog for whether a PSBT
needs to be created instead of checking whether private keys are
disabled.
Strings used in this dialog are being slightly modified to work with
both private keys enabled and disabled wallets.
451ca244db qt, refactor: Drop intermediate BitcoinApplication::shutdownResult slot (Hennadii Stepanov)
f3a17bbe5f qt: Do not exit and re-enter main event loop during shutdown (Hennadii Stepanov)
b4e0d2c431 qt, refactor: Allocate SendConfirmationDialog instances on heap (Hennadii Stepanov)
332dea2852 qt, refactor: Keep HelpMessageDialog in the main event loop (Hennadii Stepanov)
c8bae37a7a qt, refactor: Keep PSBTOperationsDialog in the main event loop (Hennadii Stepanov)
7fa91e8312 qt, refactor: Keep AskPassphraseDialog in the main event loop (Hennadii Stepanov)
6f6fde30e7 qt, refactor: Keep EditAddressDialog in the main event loop (Hennadii Stepanov)
59f7ba4fd7 qt, refactor: Keep CoinControlDialog in the main event loop (Hennadii Stepanov)
7830cd0b35 qt, refactor: Keep OptionsDialog in the main event loop (Hennadii Stepanov)
13f618818d qt: Add GUIUtil::ShowModalDialogAndDeleteOnClose (Hennadii Stepanov)
Pull request description:
On master (1ef34ee25e) during shutdown `QApplication` exits the main event loop, then re-enter again.
This PR streamlines shutdown process by removing the need to interrupt the main event loop, that is required for #59.
Also, blocking [`QDialog::exec()`](https://doc.qt.io/qt-5/qdialog.html#exec) calls are replaced with safer [`QDialog::show()`](https://doc.qt.io/qt-5/qwidget.html#show), except for `SendConfirmationDialog` as that change is not trivial (marked as TODO).
The [`QDialog::open()`](https://doc.qt.io/qt-5/qdialog.html#open) was not used because the actual modality mode (application modal or window modal) of a dialog depends on whether it has a parent.
This PR does not change behavior, and all touched dialogs are still application modal.
As a follow up, a design research could suggest to make some dialogs window modal.
NOTE for reviewers: quitting app while a dialog is open (e.g., via systray icon menu) must work fine.
ACKs for top commit:
laanwj:
Code review and lighly tested ACK 451ca244db
promag:
ACK 451ca244db, just changed signal to `quitRequested`.
Tree-SHA512: ef01ab6ed803b202e776019a4e1f592e816f7bc786e00574b25a0bf16be2374ddf9db21f0a26da08700df7ef0ab9e879550df46dcfe3b6d940f5ed02ca5f8447
This ensures that all gui translator comments end in a full stop.
If a comment does not end in a full stop, a translator may think
that the rest of the comment is being cut off.
While here, add a colon to the word "see" for any comments
touched which point to look at a link.
Move fillPSBT input-output argument before output-only arguments. This is a
temporary workaround which can go away with improvements to libmultiprocess
code generator. Currently code generator figures out order of input-output
parameters by looking at input list, but it would make more sense for it to
take order from output list, so input-only parameters still have to be first
but there is more flexibility for the other parameters.
