bb6441b7a4 qt: Pre-splitoff translations update (Wladimir J. van der Laan)
Pull request description:
0.21 split-off should be near now. Let's do one final translations update just before the split-off.
(Hopefully it won't take too long, but might want to keep this open to be the last thing merged)
ACKs for top commit:
hebasto:
ACK bb6441b7a4
MarcoFalke:
ACK bb6441b7a4 (checked that only changes are translation changes in `src/qt`)
Tree-SHA512: 3273246923d3020e1f7ae46cbb59f1ed45a35acb5e1582b55486c5723f5aa1e5809fe2fd87b1ac34d308eef2902e621d0ace97181a044262b2c8f002bf50daac
ac64cec4ce gui: create wallet: add advanced section (Sjors Provoost)
c99d6f644a gui: create wallet: name placeholder (Sjors Provoost)
5bff82540b [gui] create wallet: smarter checkbox toggling (Sjors Provoost)
Pull request description:
Previously only users who needed a second wallet had to use to the create wallet dialog. With the merge of https://github.com/bitcoin/bitcoin/pull/15454 now all new users have to. I don't think it was user-friendly enough for that.
<img width="403" alt="Schermafbeelding 2020-09-18 om 09 41 44" src="https://user-images.githubusercontent.com/10217/93574129-52ef9680-f998-11ea-9a6f-31144f66d3bf.png">
This PR makes a few simple improvements so that new users don't have to think too much:
<img width="369" alt="Schermafbeelding 2020-10-15 om 16 45 22" src="https://user-images.githubusercontent.com/10217/96145959-0c914700-0f06-11eb-9526-cf447d841d7a.png">
It's lightly inspired by #77. It would be better if those changes made it into the upcoming release, but this PR is a good start imo.
* wallet encryption is no longer checked by default, because such a change in the default needs a separate discussion (fwiw, I suspect it increases the number of users losing access to coins)
* watch-only and descriptor wallet stuff is moved to advanced, so new users know they can safely ignore these check boxes
* bonus: when you click on "disable private keys" it disables encrypt wallet and checks blank wallet
* label changes: see screenshot
* tooltip changes: see code diff
Note that a blank wallet name isn't allowed in the dialog; I haven't addressed that.
_Update 2020-10-30_, dropped the new strings for now:
<img width="450" alt="Schermafbeelding 2020-10-30 om 11 26 55" src="https://user-images.githubusercontent.com/10217/97694591-1b99fc80-1aa3-11eb-8b85-e19f1ad5add4.png">
ACKs for top commit:
fjahr:
Tested ACK ac64cec4ce
jonatack:
re-ACK ac64cec4ce, per `git diff d393708 ac64cec` only change since my last review is improving the placeholder from "MyWallet" to "Wallet" and dropping the last commit. Tested creating a dozen wallets in signet with different combinations of options and then verifying/comparing their characteristics in the console with getwalletinfo. My remaining caveats are (1) the need for less user surprise by either (a) improving the user info or (b) with less auto-(un)selecting as mentioned in https://github.com/bitcoin-core/gui/pull/96#issuecomment-727017409 and (2) I prefer the "Encrypt private keys" and "Watch-only" wording and descriptions below over the current ones; hopefully these can be addressed in a follow-up.
hebasto:
re-ACK ac64cec4ce
ryanofsky:
Code review ACK ac64cec4ce. Only changes since last review are tweaking placeholder text and dropping "allow nameless" commit
Tree-SHA512: a25f84eb66ee4f99af441d73e33928df9d9cf592177398ef48f0037f5913699e47a162cf1301c83b34501546d43ff4ae12607fd078c5c03b92f573bf7604a9f2
241434200e refactor: qt: Use vQueueNotifications.clear() (João Barbosa)
989e579d07 qt: Make transaction notification queue wallet specific (João Barbosa)
7b3b2303f4 move-only: Define TransactionNotification before TransactionTablePriv (João Barbosa)
Pull request description:
Currently `vQueueNotifications` holds transactions of any wallet, but the queue is dispatched on a given wallet and it assumes notifications are of that wallet.
This means that some transactions can be missed if multiple wallets are loaded.
Fix this by having a queue for each wallet.
ACKs for top commit:
jonasschnelli:
utACK 241434200e
hebasto:
ACK 241434200e, I have reviewed the code and it looks OK, I agree it can be merged.
ryanofsky:
Code review ACK 241434200e. Only change is dropping one commit
Tree-SHA512: 61beac5a16ed659e3a25ad145dbceafcef963aaf8f9838355298949ec2324e2bd760f59353cd251d30cf0334d8dc1642a1f3821d8a9eec092533b581f6ce86db
bbb42a6896 RPC: createwallet: Nicer error message if descriptor wallet requested and sqlite support not compiled in (Luke Dashjr)
6608fec332 GUI: Create Wallet: Nicely disable descriptor wallet checkbox if sqlite support not compiled in (Luke Dashjr)
7b54d768e1 Make sqlite support optional (compile-time) (Luke Dashjr)
Pull request description:
As a new requirement, sqlite support should be optional. This PR aims to be only minimum/blocker changes for 0.21.
Potential follow-up PRs after this:
* Make BDB support optional
* Nicer error messages when user tries to load an unsupported wallet
* Don't compile descriptor wallet code if sqlite disabled
ACKs for top commit:
jonasschnelli:
Tested ACK bbb42a6896
achow101:
ACK bbb42a6896
Sjors:
re-utACK bbb42a6896
hebasto:
ACK bbb42a6896, tested on Linux Mint 20 (x86_64, Qt 5.12.8).
Tree-SHA512: 500209dd1971310fab8ae51543343ce0ba91f088ccccff6109b4cc27547cd5532289dca6cb7dac2a7d7c59cdf3c8f5aacc31e9b0f912e38cea52ec26b97100bd
20c9e03554 gui: Call setWalletActionsEnabled(true) only for the first wallet (Hennadii Stepanov)
Pull request description:
On master (a78742830a) there is a bug:
- open an encrypted wallet; please note that the "Encrypt Wallet..." menu item is disabled that is expected:
- then open any other wallet; note that the "Encrypt Wallet..." menu item gets enabled that is wrong:
This PR fixes this bug.
ACKs for top commit:
jonasschnelli:
Tested ACK 20c9e03554 - I could reproduce the issue on master and have verify that this PR fixes it.
achow101:
ACK 20c9e03554
Tree-SHA512: 2c9ab94bde8c4f413b0a95c05bf3a1a29f5910e0f99d6639a11dd77758c78af25b060b3fecd78117066ef15b113feb79870bc1347cc04289da915c00623e5787
0d9d2a1f7c Only update the updateSmartFeeLabel once in sync (Jonas Schnelli)
Pull request description:
Calling `updateSmartFeeLabel` and therefore `estimateSmartFee` is pointless during IBD.
GUI freezes appear because `estimateSmartFee` competes with `processBlock` for the `m_cs_fee_estimator` lock leading to multiple seconds of blocking the GUI thread in `updateSmartFeeLabel`.
ACKs for top commit:
ryanofsky:
Code review ACK 0d9d2a1f7c. Clever fix. Didn't test but I remember I could reproduce the startup issue easily before by putting a sleep in estimateSmartFee.
promag:
Code review ACK 0d9d2a1f7c.
hebasto:
ACK 0d9d2a1f7c, tested on Linux Mint 20 (x86_64) with `QT_FATAL_WARNINGS=1` and `-debug=qt`.
Tree-SHA512: 85ec2266f06ddd7b523e24d2a462f10ed965d5b4d479005263056f81b7fe49996e1568dafb84658af406e9202ed3bfa846d59c10bb951e0f97cee230e30fafd5
72a1d5c6f3 validation: Remove review-only comments + assertions (Carl Dong)
3756853b15 docs: Move FindFilesToPrune{,Manual} doxygen comment (Carl Dong)
485899a93c style: Make FindFilesToPrune{,Manual} match style guide (Carl Dong)
3f5b5f3f6d validation: Move FindFilesToPrune{,Manual} to BlockManager (Carl Dong)
f8d4975ab3 validation: Move PruneOneBlockFile to BlockManager (Carl Dong)
74f73c783d validation: Pass in chainman to UnloadBlockIndex (Carl Dong)
4668ded6d6 validation: Move ~CMainCleanup logic to ~BlockManager (Carl Dong)
Pull request description:
This PR paves the way for de-globalizing `g_chainman` entirely by removing the usage of `g_chainman` in the following functions/methods:
- `~CMainCleanup`
- `CChainState::FlushStateToDisk`
- `UnloadBlockIndex`
The remaining direct uses of `g_chainman` are as follows:
1. In initialization codepaths:
- `AppTests`
- `AppInitMain`
- `TestingSetup::TestingSetup`
2. `::ChainstateActive`
3. `LookupBlockIndex`
- Note: `LookupBlockIndex` is used extensively throughout the codebase and require a much larger set of changes, therefore I've left it out of this initial PR
ACKs for top commit:
MarcoFalke:
re-ACK 72a1d5c6f3👚
jnewbery:
utACK 72a1d5c6f3
Tree-SHA512: 944a4fa8405eecf39706ff944375d6824373aaeea849d11473f08181eff26b12f70043a8348a5b08e6e9021b243b481842fbdfbc7c3140ca795fce3688b7f5c3
This removes a source of complexity and indirection that makes it harder to
understand path checking code. Path checks will be simplified in upcoming
commits.
There is no change in behavior in this commit other than a slightly more
descriptive error message in `loadwallet` if the default "" wallet can't be
found. (The error message is improved more in upcoming commit "wallet: Remove
path checking code from loadwallet RPC".)
f1ee37319a wallet: Reload previously loaded wallets on GUI startup (Andrew Chow)
Pull request description:
Enable the GUI to also use the load_on_startup feature. Wallets loaded in the GUI always have load_on_startup=true. When they are unloaded from the GUI, load_on_startup=false.
To facilitate this change, UpdateWalletSetting is moved into the wallet module and called from within LoadWallet, RemoveWallet, and Createwallet. This change does not actually touch the GUI code but rather the wallet functions that are shared between the GUI and RPC.
ACKs for top commit:
jonasschnelli:
Tested ACK f1ee37319a - works as expected. Wallets loaded via bitcoin-cli (in `-server` mode) or through the RPC console won't be loaded on startup but wallets loaded via the GUI menu will.
kristapsk:
ACK f1ee37319a, I have tested the code.
Tree-SHA512: f5b44aa763cf761d919015c5fbc0600b72434aa71e3b57007fd7530a29c3da1a9a0c98c4f22cb6cdffba61150a31170056a7d4737625e7b76f6958f3d584da8c
Enable the GUI to also use the load_on_startup feature.
Wallets loaded in the GUI always have load_on_startup=true.
When they are unloaded from the GUI, load_on_startup=false.
To facilitate this change, UpdateWalletSetting is moved into the wallet
module and called from within LoadWallet, RemoveWallet, and
Createwallet. This change does not actually touch the GUI code but
rather the wallet functions that are shared between the GUI and RPC.
24bf17602c gui refactor: Inline SplashScreen::ConnectWallet (Russell Yanofsky)
e4f4350471 refactor: Move wallet methods out of chain.h and node.h (Russell Yanofsky)
b266b3e0bf refactor: Create interfaces earlier during initialization (Russell Yanofsky)
Pull request description:
Add WalletClient interface so node interface is cleaner and don't need wallet-specific methods.
The new NodeContext::wallet_client pointer will also be needed to eliminate global wallet variables like ::vpwallets in #19101, because createWallet(), loadWallet(), getWallets(), etc methods called by the GUI need a way to get a reference to the list of open wallets if it is no longer a global variable.
ACKs for top commit:
promag:
Code review ACK 24bf17602c.
MarcoFalke:
ACK 24bf17602c🐚
Tree-SHA512: a70d3776cd6723093db8912028c50075ec5fa0a48b961cb1a945f922658f5363754f8380dbb8378ed128c8c858913024f8264740905b8121a35c0d63bfaed7cf
ca185cf5a1 doc: Document differences in bitcoind and bitcoin-qt locale handling (practicalswift)
Pull request description:
Document differences in `bitcoind` and `bitcoin-qt` locale handling.
Since this seems to be the root cause to the locale dependency issues we've seen over the years I thought it was worth documenting :)
Note that 1.) `QLocale` (used by Qt), 2.) C locale (used by locale-sensitive C standard library functions/POSIX functions and some parts of the C++ standard library such as `std::to_string`) and 3.) C++ locale (used by the C++ input/output library) are three separate things. This comment is about the perhaps surprising interference with the C locale (2) that takes place as part of the Qt initialization.
ACKs for top commit:
hebasto:
re-ACK ca185cf5a1
Tree-SHA512: e51c32f3072c506b0029a001d8b108125e1acb4f2b6a48a6be721ddadda9da0ae77a9b39ff33f9d9eebabe2244c1db09e8502e3e7012d7a5d40d98e96da0dc44
4ec49f8d1e qt: Leverage the default "Create new receiving address" button (Hennadii Stepanov)
4227a8e1f3 qt: Make "Create new receiving address" default unconditionally (Hennadii Stepanov)
Pull request description:
Fix#24
The first commit:
- visual improvement with no behavior change
The second commit:
- removes a bunch of LOCs
- slightly change behavior and makes it standard
With this PR:
ACKs for top commit:
Saibato:
Concept tACK 4227a8e1f34ec49f8d1e
promag:
Tested ACK 4ec49f8d1e on macos.
Tree-SHA512: 3403d5ee96ec139491c7e23b24a24d9239fe55c58d99cbd4cd13bc877f76f992ed011c09e2af35b2a63be1a2371b95f6ac719325396dcc8333cf3eb7fa2e3d2c
b6dcc6d741 gui: Clarify block height label (Hennadii Stepanov)
Pull request description:
Prefer "block height" instead of "number of blocks".
This was done while testing https://github.com/bitcoin/bitcoin/pull/16981.
ACKs for top commit:
michaelfolkson:
ACK b6dcc6d741. I don't think there are any other obvious examples in the GUI where "block height" should replace "number of blocks" except for translations.
MarcoFalke:
cr ACK b6dcc6d741
Tree-SHA512: ec3b48c1af5d613ed657ad51f2caddea774376736ecc02343d54518986e35ec37f1745b059814b5be92b5e5c2bb2970d17159b24c6e88b9316803d4de5327c31
Add WalletClient interface so node interface is cleaner and don't need
wallet-specific methods.
The new NodeContext::wallet_client pointer will also be needed to eliminate
global wallet variables like ::vpwallets, because createWallet(), loadWallet(),
getWallets(), etc methods called by the GUI need a way to get a reference to
the list of open wallets if it is no longer a global variable.
Also tweaks splash screen registration for load wallet events to be delayed
until after wallet client is created.
