If the user has unchecked "Allow incoming connections" in
`Settings->Options...->Network` then `fListen=false` is saved in
`~/.config/Bitcoin/Bitcoin-Qt.conf`. This flips `-listen` to `false`
during startup, but leaves `-listenonion` to `true`.
This flipping of `-listen` is done in `OptionsModel::Init()` after
`InitParameterInteraction()` has been executed which would have flipped
`-listenonion`, should it have seen `-listen` being `false`
(this is a difference between `bitcoind` and `bitcoin-qt`).
Fixes: https://github.com/bitcoin-core/gui/issues/567
4d2b503d6c gui: improve "Addresses Rate-Limited" translator comments and tooltip in peers tab (Jon Atack)
81ef1f7ef1 gui: improve "Addresses Processed" translator comments and tooltip in peers tab (Jon Atack)
77f24aac52 gui: improve "Address Relay" translator comments and tooltip in peers tab (Jon Atack)
Pull request description:
Per translator feedback in this thread: https://github.com/bitcoin-core/gui/pull/526#discussion_r809237830
*"The lack of string context in Transifex is a real problem for this project, as proper context (dev notes and/or screenshots) are essential to achieve quality translations."*
This pull adds developer notes for transifex translators via `extracomment` tags, and it improves the existing ones and their tooltips with more context, clarity and completeness for the following peer tab fields as a follow-up to bitcoin-core/gui#526:
- address relay
- addresses processed
- addressed rate-limited
It looks like only six lines of diff, but they are loooong lines.
If this is the right direction, the same can be done for other fields in follow-ups.
ACKs for top commit:
jarolrod:
re-ACK [4d2b503](4d2b503d6c)
hebasto:
ACK 4d2b503d6c, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: a185f46a66375a5fd6854640745b7d1d00740cf7be58db03256f44d71acc351e1770de137cb3bc9c1f0ea3cabd7cfa1cb1ccb87ec0df222680924ca3dab6c8bf
2efdfb88aa gui: restore Send for external signer (Sjors Provoost)
4b5a6cd149 refactor: helper function signWithExternalSigner() (Sjors Provoost)
026b5b4523 move-only: helper function to present PSBT (Sjors Provoost)
Pull request description:
Fixes#551
For the simplest use case of a wallet with one external signer and "PSBT Controls" disabled in settings (the default), the send dialog will behave the same as when using a wallet with private keys. I.e. there's no "Create Unsigned" button.
When PSBT controls are turned on, you can now actually make a PSBT with signing it; before this PR that button would trigger a sign event and would broadcast the transaction.
In case a multisig, the Send button will sign on the device, and then fall back to presenting a PSBT (same behavior as before #441).
This PR starts with two refactoring commits to move some stuff into a helper function for improved readability in general, and to make the main commit easier to review.
ACKs for top commit:
jonatack:
utACK 2efdfb88aa diff review since my last review, code re-review, rebased to current master, verified clean debug build of each commit
luke-jr:
utACK 2efdfb88aa
Tree-SHA512: e8731a0ef9e87564b2676c7b022b742d9621bba964c19dba9fd9f6961eb608737a9e1a22c0a3c8b2f2f6d583bba067606ee8392422e82082deefb20ea7b88c7c
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>
b7dbc83f23 qt: Remove network detection based on address in BIP21 (laanwj)
Pull request description:
This is removes some ugly and brittle code that switches the global network to testnet based on a provided address. I think in practice it's very unlikely for testnet BIP21 payment URIs to be used, and if so it's for testing so it's easy enough to manually copy it. Or to specify `-testnet` explicitly.
There is already no such case for `-regtest` or `-signet`.
After this change it will only accept addresses for the explicitly selected network. Others will result in a "wrong network" popup.
There is also a possibility for refactor after this as the initialization order of `PaymentServer::ipcParseCommandLine` isn't important anymore (well, it still has to be before `PaymentServer::ipcSendCommandLine`, maybe even merged with it), but I have not done so here.
ACKs for top commit:
jonatack:
ACK b7dbc83f23
achow101:
ACK b7dbc83f23
Tree-SHA512: ebc77c0e5c98f53fe254bcd0f94c9d1c06937b794346e95b14158860d5c607515e71d73b782d2726674dce86d6d4001085d473c6d1bc11c5e6c25ff3fb3cfa22
This is some very ugly and brittle code that switches the global network
based on a provided address, remove it. I think in practice it's very
unlikely for testnet BIP21 payment URIs to be used, and if so it's for
testing so it's easy enough to manually copy it. Or to specify
`-testnet` explicitly.
There is already no case for `-regtest` or `-signet`.
Fix GUI startup crash reported by Rspigler in
https://github.com/bitcoin/bitcoin/issues/24457 that happens if
settings.json contains an integer value for any of the configuration
options which GUI settings can currently clash with (-dbcache, -par,
-spendzeroconfchange, -signer, -upnp, -natpmp, -listen, -server, -proxy,
-proxy, -onion, -onion, -lang, and -prune).
Fix is a one-line change in ArgsManager::GetArg.
Just add tests. No changes to application behavior. Tests will be
updated in the next commit changing & improving current behavior.
Include a Qt test for GUI startup crash reported by Rspigler in
https://github.com/bitcoin/bitcoin/issues/24457 caused by GetArg
behavior that happens if settings.json contains an integer value for any
of the configuration options which GUI settings can currently clash with
(-dbcache, -par, -spendzeroconfchange, -signer, -upnp, -natpmp, -listen,
-server, -proxy, -proxy, -onion, -onion, -lang, and -prune).
6f2593dc23 gui, refactor: use std::chrono for formatDurationStr() helper (Jon Atack)
Pull request description:
Updates `formatDurationStr()` to use the `chrono` standard lib. No change in behavior.
ACKs for top commit:
RandyMcMillan:
tACK 6f2593dc23
shaavan:
ACK 6f2593dc23
w0xlt:
tACK 6f2593d on Ubuntu 21.10 Qt 5.15.2
promag:
Code review ACK 6f2593dc23.
Tree-SHA512: 61e9afdb1db779150df338e6af08727c34f69639add465c2f7003ff775d97dce3e78e78d325bc6dea5bc13f0fce9ef1c3506d13f1661a5e083e52bba8a32ba44
0eea83a85e scripted-diff: rename `proxyType` to `Proxy` (Vasil Dimov)
e53a8505db net: respect -onlynet= when making outbound connections (Vasil Dimov)
Pull request description:
Do not make outbound connections to hosts which belong to a network
which is restricted by `-onlynet`.
This applies to hosts that are automatically chosen to connect to and to
anchors.
This does not apply to hosts given to `-connect`, `-addnode`,
`addnode` RPC, dns seeds, `-seednode`.
Fixes https://github.com/bitcoin/bitcoin/issues/13378
Fixes https://github.com/bitcoin/bitcoin/issues/22647
Supersedes https://github.com/bitcoin/bitcoin/pull/22651
ACKs for top commit:
naumenkogs:
utACK 0eea83a85e
prayank23:
reACK 0eea83a85e
jonatack:
ACK 0eea83a85e code review, rebased to master, debug built, and did some manual testing with various config options on signet
Tree-SHA512: 37d68b449dd6d2715843fc84d85f48fa2508be40ea105a7f4a28443b318d0b6bd39e3b2ca2a6186f2913836adf08d91038a8b142928e1282130f39ac81aa741b
48742693ac Replace "can not" with "cannot" in docs, user messages, and tests (Jon Atack)
e670edd434 User-facing content fixups from transifex translator feedback (Jon Atack)
Pull request description:
Closes#24366.
ACKs for top commit:
laanwj:
Code review re-ACK 48742693ac
hebasto:
re-ACK 48742693ac, only suggested change since my previous [review](https://github.com/bitcoin/bitcoin/pull/24367#pullrequestreview-885938219).
Tree-SHA512: 4dcdcb417251a413e65fab6070515e13a1267c8e0dbcf521386b842511391f24c84a0c2168fe13458c977682034466509bf2a3453719d4d94d3c568fd9f4adb4
e7fc50681e qt: Override BitcoinApplication::event() to handle QEvent::Quit (Hennadii Stepanov)
Pull request description:
bitcoin-core/gui#336 introduced a regression when termination requests from a platform are not handled properly.
This PR fixes this regression. On macOS shutdown after clicking "Quit" in Dock icon menu, and during logout works again.
Fixesbitcoin-core/gui#545.
ACKs for top commit:
RandyMcMillan:
tACK e7fc50681e
Sjors:
tACK e7fc50681e (rebased on master) indeed fixes the crash described in #545
promag:
Tested ACK e7fc50681e on macOS 10.15 with Qt 5.15.2.
Tree-SHA512: 236a483dc0828f22999469e133b8ac9f0b6267ec2a27004c3ebaa967689ddb972ea1fa90c1dd41f3bff3d17bf571a707babcef53bd79fd711fda98cfbf120131
f730bd7d58 scripted-diff: Rename ShowModalDialogAndDeleteOnClose (Hennadii Stepanov)
5d7666b151 qt: Revert 7fa91e8312 partially (Hennadii Stepanov)
89c277a6fc qt: Delay shutdown while a modal dialog is active (Hennadii Stepanov)
8c0eb80f41 qt: Disable tray icon menu when a modal dialog is active (Hennadii Stepanov)
92427354dd qt, refactor: Use local QAction instances for the tray icon menu (Hennadii Stepanov)
58e16035c1 qt, refactor: Drop BitcoinGUI::{send,receive}CoinsMenuAction members (Hennadii Stepanov)
fd667e73cd qt: Make show_hide_action dependent on the main window actual state (Hennadii Stepanov)
ee151d0327 qt: Drop BitcoinGUI::toggleHideAction member (Hennadii Stepanov)
78189daac8 qt, refactor: Fill up trayIconMenu before connections (Hennadii Stepanov)
66afa286e5 qt, refactor: Replace BitcoinGUI::trayIconActivated with a lambda (Hennadii Stepanov)
c3ca8364b2 qt, refactor: Replace BitcoinGUI::macosDockIconActivated with a lambda (Hennadii Stepanov)
Pull request description:
As pointed in bitcoin/bitcoin#23790 a regression in wallet unlock was introduced in bitcoin-core/gui#336 when a synchronous `AskPassphraseDialog` has been replaced with an asynchronous one.
This PR reverts a call back to a synchronous mode.
To make synchronous dialogs behave nice during shutdown some additional changes were made.
Please note that disabling the tray icon menu when a modal dialog is active is useful itself as on master (4ad59042b3) it is possible to switch to the "Receive" tab while the GUI is waiting for a password for the "Send" tab:
This is confusing and must be avoided.
Fixesbitcoin/bitcoin#23790.
ACKs for top commit:
prayank23:
tACK f730bd7d58
Tree-SHA512: 2b68275754190e4a9831b96e882d3c5b005e03909aeb6f2c5846da07199bb3efbb74ce87a9d25bb139f643c43d377a2051b221d553281fa5aefdd3181a58077f
-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.
The AskPassphraseDialog modal dialog must be synchronous here as
expected in the WalletModel::requestUnlock() function.
Fixed an introduced regression.
aeb18b665c Bugfix: GUI: Check validity when QValidatedLineEdit::setText is called (Luke Dashjr)
b1a544be10 Bugfix: GUI: Re-check validity after QValidatedLineEdit::setCheckValidator (Luke Dashjr)
2385b508d5 Bugfix: GUI: Only apply invalid style to QValidatedLineEdit, not its tooltip (Luke Dashjr)
Pull request description:
1. Use a CSS selector to avoid changing the background colour of the tooltip.
2. Re-check validity of input when we first set the validator (probably a no-op in practice).
3. Check validity of input when it is set programmatically via `setText` (eg, via the address book). Probably no-op in practice UNTIL merging https://github.com/bitcoin/bitcoin/pull/15987 or any other PR that adds a warning for valid addresses.
Moved from https://github.com/bitcoin/bitcoin/pull/18133 (just concept ACKs)
ACKs for top commit:
Sjors:
tACK aeb18b665c
hebasto:
ACK aeb18b665c, tested on Linux Mint 20.3 (Qt 5.12.8).
Tree-SHA512: b6fa8ee4dec76e1c759095721240e6fa5071a02993cb28406e96a0fa2e819f5dddc03d2e7c9073354d7865c2b09eb263afaf853ecba42e9fc4f50ef4ae20bf0f
f7a19ef774 qt,refactor: Use std::chrono in TrafficGraphWidget class (Shashwat)
Pull request description:
The PR is a follow-up to #517
- It addresses the change suggested in [this](https://github.com/bitcoin-core/gui/pull/517#pullrequestreview-850260826) comment.
- This PR changes the type of `msecsPerSample` from **int** to **std::chrono::minutes** and makes other relevant subsequent changes that were limited to the **trafficgraphwidget** file.
ACKs for top commit:
RandyMcMillan:
tACK f7a19ef774
hebasto:
ACK f7a19ef774
promag:
Code review ACK f7a19ef774.
Tree-SHA512: 5094ba894f3051fc99148cb8f408fc6f9d6571188673dcb7bf24366cdfb3eaf6d4e41083685d578ad2a9fbe31cc491a5f3fa9b7c9ab6eb90e4dc1356f89ae18a
04255073bb qt: Update source translations (laanwj)
cf79c56e65 init: Remove confusing '(possible integer overflow?)' from error message (laanwj)
d570a63894 qt: Update transifex resource blob to 23.0 (laanwj)
Pull request description:
- Update translations for 0.23 string freeze
- Update transifex resource blob to 23.0
This is necessary before a 23.0 resource can be created on Transifex.
ACKs for top commit:
hebasto:
ACK 04255073bb
Tree-SHA512: ff886e92721f070e3c135cfec229c41848a67c02355b88f2a5a507241b545f4209167d83b561420c2a82f49a5994170b01dcfb95bfc3fe6b9c832abcc6547b14
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>
9fbd1bb7fa gui: use available space to display "Last Transaction" in peer details (Jon Atack)
6cd132d380 gui: add "Addresses Rate-Limited" (m_addr_rate_limited) to peer details (Jon Atack)
19623d3182 gui: add "Addresses Processed" (m_addr_processed) to peer details (Jon Atack)
a465a66ef2 gui: add "Address Relay" (m_addr_relay_enabled) to peer details (Jon Atack)
Pull request description:
This pull adds the following address fields in rpc getpeerinfo and cli -netinfo to the gui peers details:
- Address Relay (Yes/No)
- Addresses Processed (integer)
- Addresses Rate-Limited (integer)
and uses the additional horizontal space to display "Last Transaction" (instead of "Last Tx").
ACKs for top commit:
hebasto:
ACK 9fbd1bb7fa, tested on Ubuntu 21.10 (Qt 5.15.2).
w0xlt:
reACK 9fbd1bb
Tree-SHA512: 76d414b82f432b7baf2cadcf2f52412a3af8ad78a93755bb82c65df5353dda4d2e2522428a36c8bb95316bf84b17f2485636c33ce5ae11566469671b5384d845
fac8165443 Remove unused checkFinalTx (MarcoFalke)
fa272eab44 wallet: Avoid dropping confirmed coins (MarcoFalke)
888841ea8d interfaces: Remove unused is_final (MarcoFalke)
dddd05e7a3 qt: Treat unconfirmed txs as unconfirmed (MarcoFalke)
Pull request description:
The wallet has several issues:
## Unconfirmed txs in the GUI
The GUI clumsily attempts to guess if unconfirmed txs are locked until a future time. This is currently based on the locktime only, not nSequence, thus wrong. Fix this by removing the clumsy code and treat all unconfirmed txs as unconfirmed. The GUI already prints whether a tx is in the mempool, in which case the user knows that the tx wasn't locked until a future time. If the tx is not in the mempool, it might be better to report the exact reject reason from the mempool instead of using incorrect heuristics.
## Confirmed txs in the wallet
The wallet drops coins that it incorrectly assumes to be locked until a future time, even if they are already confirmed in the chain. This is because the wallet is using the wrong time (adjusted network time) instead of MTP, due to the `-1` default argument of `CheckFinalTx`.
The issues are fixed in separate commits and there is even a test.
ACKs for top commit:
achow101:
ACK fac8165443
prayank23:
reACK fac8165443
glozow:
code review ACK fac8165443, I understand now how this fixes both issues.
Tree-SHA512: 210afb855f4c6d903fee49eba6b1a9735d699cf0168b669eabb38178e53b3a522258b7cc669f52489c6cd3e38bf358afde12eef3ba2e2f2ffaeb06b8f652ccd0
7f122a4188 fuzz: non-addrman fuzz tests: override-able check ratio (Vasil Dimov)
3bd83e273d fuzz: addrman fuzz tests: override-able check ratio (Vasil Dimov)
46b0fe7829 test: non-addrman unit tests: override-able check ratio (Vasil Dimov)
81e4d54d3a test: addrman unit tests: override-able check ratio (Vasil Dimov)
6dff6214be bench: put addrman check ratio in a variable (Vasil Dimov)
6f7c7567c5 fuzz: parse the command line arguments in fuzz tests (Vasil Dimov)
92a0f7e58d test: parse the command line arguments in unit tests (Vasil Dimov)
Pull request description:
Previously command line arguments passed to unit and fuzz tests would be ignored by the tests themselves. They would be used by the boost test framework (e.g. `--run_test="addrman_tests/*"`) or by the fuzzer (e.g. `-runs=1`). However both provide ways to pass down the extra arguments to the test itself. Use that, parse the arguments and make them available to the tests via `gArgs`.
This makes the tests more flexible as they can be run with any bitcoind config option specified on the command line.
When creating `AddrMan` objects in tests, use `-checkaddrman=` (if provided) instead of hardcoding the check ratio in many different places. See https://github.com/bitcoin/bitcoin/pull/20233#issuecomment-889813074 for further motivation for this.
ACKs for top commit:
mzumsande:
re-ACK 7f122a4188
josibake:
reACK 7f122a4188
Tree-SHA512: 3a05e61e4d70a0569bb67594bcce3aad8fdef63cdcc54e2823a3bc9f18679571985004412b6c332a210f67849bab32d8467b4115fbff8f5fac9834982e60dcf3
Retrieve the command line arguments from boost and pass them to
`BasicTestingSetup` so that we gain extra flexibility of passing any
config options on the test command line, e.g.:
```
test_bitcoin -- -printtoconsole=1 -checkaddrman=5
```
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
Name has been confusing since it was introduced, and it was pointed in
recent review club as https://bitcoincore.reviews/10102 that it was
particularly unclear how interfaces::WalletClient was different from
interfaces::Wallet.
-BEGIN VERIFY SCRIPT-
ren() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
ren WalletClient WalletLoader
ren walletClient walletLoader
ren wallet_client wallet_loader
ren "wallet clients release the wallet" "wallet pointer owners release the wallet"
ren "wallet client" "wallet loader"
ren "Wallet client" "Wallet loader"
-END VERIFY SCRIPT-
71d33380ed qt: prevent negative values of progressPerHour (HiLivin)
Pull request description:
Added a similar guard to _progressPerHour_ as is placed at _remainingMSecs_.
It prevents the display of negative values like "-0.00%" in some cases.
ACKs for top commit:
hebasto:
ACK 71d33380ed
jarolrod:
ACK 71d3338
shaavan:
reACK 71d33380ed
Tree-SHA512: 5427cdf4441b542196008034355ea00a075adf8b9aeeb383bacdb4e5fbda23d665448a50035aac93cbf401d5d6211d39a2c7c294568d9f5548a5c7579e201c44
fad943821e scripted-diff: Rename touched member variables (MarcoFalke)
fa663a4c0d Use mockable time for peer connection time (MarcoFalke)
fad7ead146 refactor: Use type-safe std::chrono in net (MarcoFalke)
Pull request description:
Benefits:
* Type-safe
* Mockable
* Allows to revert a temporary test workaround
ACKs for top commit:
naumenkogs:
ACK fad943821e
shaavan:
ACK fad943821e
Tree-SHA512: af9bdfc695ab727b100c6810a7289d29b02b0ea9fa4fee9cc1f3eeefb52c8c465ea2734bae0c1c63b3b0d6264ba2c493268bc970ef6916570eb166de77829d82
fadc0c80ae p2p: Make timeout mockable and type safe, speed up test (MarcoFalke)
fa6d5a238d scripted-diff: Rename m_last_send and m_last_recv (MarcoFalke)
Pull request description:
Use type-safe time for better code readability/maintainability and mockable time for better testability. This speeds up the p2p_timeout test.
This is also a bugfix for intermittent test issues like: https://cirrus-ci.com/task/4769904156999680?command=ci#L2836Fixes#20654
ACKs for top commit:
laanwj:
Code review ACK fadc0c80ae
naumenkogs:
ACK fadc0c80ae
Tree-SHA512: 28c6544c97f188c8a0fbc80411c74ab74ffd055885322c325aa3d1c404b29c3fd70a737e86083eecae58ef394db1cb56bc122d06cff63742aa89a8e868730c64
b9f0aff6b4 qt: monospaced output in Console on macOS (randymcmillan)
Pull request description:
This PR addresses issue https://github.com/bitcoin-core/gui/issues/273
A monospace font is used on Linux and Windows for the console output - but not on MacOS.
This change forces the MacOS GUI to use the embedded RobotoMono-Bold.ttf font,
which is defined as the GUIUtil::fixedPitchFont()
ACKs for top commit:
hebasto:
ACK b9f0aff6b4, Tested on macOS Big Sur 11.6.1 (20G224) + Homebrew's Qt 5.15.2:
shaavan:
reACK b9f0aff6b4
jarolrod:
tACK b9f0aff6b4
Tree-SHA512: 53e6635a0189e133681c85d442c6c9c4a10438151e4bf7da5bbd62abca7ab55685caf2c9a75ff200aadea771c1602902e6ab14afdc4f411e1b3013dd49625dbc
dbde0558ce gui: Paste button in Open URI dialog (Kristaps Kaupe)
Pull request description:
Picking up https://github.com/bitcoin/bitcoin/pull/17955, with some review comments addressed.
ACKs for top commit:
shaavan:
tACK dbde055
jarolrod:
ACK dbde055
promag:
Tested ACK dbde0558ce.
Tree-SHA512: db47f19673aff6becd6d1f938cd2aa5dc2291d6e80150d2b99f435674330a5eae678b20e42ef327ea9b05c44925a941fc251e622c73b3585018fc7c1d245edb5
25a581419d GUI/Options: Restore "S" accelerator for "Start on system login" option (Luke Dashjr)
Pull request description:
bitcoin-core/gui#416 changed the option assigned to accelerator key "S", but there's no rationale given.
Best to leave it alone, and give the new option a new accelerator key.
Since "R" is already taken for Reset, this shifts the new RPC server option to use "P" instead
ACKs for top commit:
jarolrod:
ACK 25a581419d
hebasto:
ACK 25a581419d, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: 2212aa32572cbcbcce78304ffcb1dcfc65f9d7e2ffd6c6a4b65e4a3ca2a8a7cc7505c28314ad46e0bc13b4e3bb3fc61e7e196356d26354f3689fad71fb688b27
0e0f4fdd89 multiprocess: Add interfaces::Node::broadCastTransaction method (Russell Yanofsky)
Pull request description:
This fixes a null pointer crash in the bitcoin-gui PSBT dialog. The bitcoin-gui interfaces::Node object has a null NodeContext pointer, and can't broadcast transactions directly. It needs to broadcast transactions through the bitcoin-node process instead.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
lsilva01:
Code Review ACK 0e0f4fd
Tree-SHA512: cd2c1fe8dc15e7cecf01a21d64319d6add1124995305a9ef9cb72f8492dc692c62d4f846182567d47a5048a533178a925419250941a47cb39932467c36bea3e1
ad085f9ba1 multiprocess: Delay wallet client construction (Russell Yanofsky)
Pull request description:
Delay wallet client construction until after logging, thread and other init for two reasons:
- More responsive multiprocess GUI startup. When bitcoin-gui is started this moves the call from bitcoin-gui to bitcoin-node that spawns bitcoin-wallet off of the GUI event thread and onto the background GUI init executor thread.
- Avoids feature_logging.py test failures with bitcoin-node by making bitcoin-wallet logging start after bitcoin-node logging starts,
because the tests are not written to handle the bitcoin-wallet logging init code running first.
This partially reverts commit b266b3e0bf, moving wallet client creation back to the place it was located before.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
laanwj:
code review ACK ad085f9ba1
hebasto:
ACK ad085f9ba1, I have reviewed the code and it looks OK.
Tree-SHA512: 74d957ce2ee096db745c517124f60800185814b06c20db676090e10dce1b90311adbab02865a69731f8c39b9365f9ee14be0830ca1368cac9b474801ea92bad5
a032fa30d2 multiprocess: add interfaces::ExternalSigner class (Russell Yanofsky)
Pull request description:
Add `interfaces::ExternalSigner` class to let signer objects be passed between processes and let signer code run in the original process where the object was created.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
laanwj:
Concept and code review ACK a032fa30d2
hebasto:
re-ACK a032fa30d2
Tree-SHA512: 99a729fb3a64d010e142cc778a9f1f358e58345b77faaf2664de7d2277715d59df3352326e8f0f2a6628038670eaa4556310a549079fb28af6d2eeb05aea1460
This fixes a null pointer crash in the bitcoin-gui PSBT dialog. The
bitcoin-gui interfaces::Node object has a null NodeContext pointer, and
can't broadcast transactions directly. It needs to broadcast
transactions through the bitcoin-node process instead.
54011e7aa2 refactor: use CWallet const shared pointers when possible (Karl-Johan Alm)
96461989a2 refactor: const shared_ptrs (Karl-Johan Alm)
Pull request description:
```C++
const std::shared_ptr<CWallet> wallet = x;
```
means we can not do `wallet = y`, but we can totally do `wallet->DestructiveOperation()`, contrary to what that line looks like.
This PR
* introduces a new convention: always use const shared pointers to `CWallet`s (even when we mutate the pointed-to thing)
* uses `const shared_ptr<const CWallet>` everywhere where wallets are not modified
In the future, this should preferably apply to all shared pointers, not limited to just `CWallet`s.
Both of these serve the same purpose: to dispell the misconception that `const shared_ptr<X>` immutates `X`. It doesn't, and it's dangerous to leave this misconception as is, for obvious reasons.
ACKs for top commit:
theStack:
re-ACK 54011e7aa2
Tree-SHA512: 3bf4062fc821751be30770c6b4ead10a016847970f155a0a5156f304347d221b9830840030c2fbfba8cd1e282f4eda45f5b4107fe6df8138afdcb6c2e95a2836
d5f985e51f multiprocess: Add new bitcoin-gui, bitcoin-qt, bitcoin-wallet init implementations (Russell Yanofsky)
Pull request description:
Add separate `interfaces::Init` subclasses for `bitcoin-wallet`, `bitcoin-gui`, and `bitcoin-qt` binaries instead of sharing `bitcoind` and `bitcoin-node` init subclasses in different binaries. After this, the new init subclasses can be customized in #10102, so node and wallet code is dropped from the `bitcoin-gui` binary and wallet code is dropped from into the `bitcoin-node` binary.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
lsilva01:
reACK d5f985e
hebasto:
re-ACK d5f985e51f, only suggested changes since my [previous](https://github.com/bitcoin/bitcoin/pull/23006#pullrequestreview-787537444) review.
Tree-SHA512: 6784210bd9ce3a6fbc66852680d0e9bc513c072dc538aeb7f48cb6a41580d3f567ccef04f975ee767a714a4b05d4d87273e94a79abda1b9c25d5ac4bbe752006
Introduce convention to use const shared pointers everywhere, unless the shared pointer is modified at some point, which it very rarely is.
We want this convention, as it helps alleviate the misconception that a const shared pointer somehow results in a pointer to an immutable object, which is false.
58765a450c qt: Use only Qt translation primitives in GUI code (W. J. van der Laan)
Pull request description:
Use `Object::tr`, `QT_TRANSLATE_NOOP`, and `QCoreApplication::translate` as appropriate instead of using `_()` which doesn't get picked up.
Replaces bitcoin/bitcoin#22764
Edit: I checked that the strings end up in the appropriate context in `bitcoin_en.ts` after `make translate`:
- "Settings file could not be read" "Settings file could not be written" end up in `bitcoin-core`
- "(press q to shutdown and continue later)" and "press q to shutdown" end up in `SplashScreen`
ACKs for top commit:
hebasto:
ACK 58765a450c
Tree-SHA512: d0cc5901426c47d411d0fe75aac195b9684b51385f74c161da772dbf9f0977f7ad7a0976e17366f49b40718e9b6eb87c3e93306dc845f97adddbc47d00731742
2d2edc1248 tests: Use Descriptor wallets for generic wallet tests (Andrew Chow)
99516285b7 tests: Use legacy change type in subtract fee from outputs test (Andrew Chow)
dcd6eeb64a tests: Use descriptors in psbt_wallet_tests (Andrew Chow)
4b1588c6bd tests: Use DescriptorScriptPubKeyMan in coinselector_tests (Andrew Chow)
811319fea4 tests, gui: Use DescriptorScriptPubKeyMan in GUI tests (Andrew Chow)
9bf0243872 bench: Use DescriptorScriptPubKeyMan for wallet things (Andrew Chow)
5e54aa9b90 bench: remove global testWallet from CoinSelection benchmark (Andrew Chow)
a5595b1320 tests: Remove global vCoins and testWallet from coinselector_tests (Andrew Chow)
Pull request description:
Currently, various tests use `LegacyScriptPubKeyMan` because it was convenient for the refactor that introduced the `ScriptPubKeyMan` interface. However, with the legacy wallet slated to be removed, these tests should not continue to use `LegacyScriptPubKeyMan` as they are not testing any specific legacy wallet behavior. These tests are changed to use `DescriptorScriptPubKeyMan`s.
Some of the coin selection tests and benchmarks had a global `testWallet`, but this seemed to cause some issues with ensuring that descriptors were set up in that wallet for each test. Those have been restructured to not have any global variables that may be modified between tests.
The tests which test specific legacy wallet behavior remain unchanged.
ACKs for top commit:
laanwj:
Code review ACK 2d2edc1248
brunoerg:
tACK 2d2edc1248
Tree-SHA512: 6d60e5978e822d48e46cfc0dae4635fcb1939f21ea9d84eb72e36112e925554b7ee8f932c7ed0c4881b6566c6c19260bec346abdff1956ca9f300b30fb4e2dd1
9c1052a521 wallet: Default new wallets to descriptor wallets (Andrew Chow)
f19ad40463 rpc, wallet: Descriptor wallets are no longer experimental (Andrew Chow)
Pull request description:
Changes the default wallet type from legacy to descriptors. Descriptor wallets will now by the default type. Additionally, descriptor wallets will no longer be marked as experimental.
This follows the timeline proposed in #20160
ACKs for top commit:
lsilva01:
Tested ACK 9c1052a521 on Ubuntu 20.04
prayank23:
tACK 9c1052a521
meshcollider:
Code review ACK 9c1052a521
Tree-SHA512: 834e6fec88e0c18673af7ebe135bd5333694d1be502164eb93a90e3e76c27974165aa4e59426945100c88e4eca07356e16886ef5b05cf789683ecb23fc71a12a
6544ea5035 refactor: Block unsafe fs::path std::string conversion calls (Russell Yanofsky)
b39a477ec6 refactor: Add fs::PathToString, fs::PathFromString, u8string, u8path functions (Russell Yanofsky)
Pull request description:
The `fs::path` class has a `std::string` constructor which will implicitly convert from strings. Implicit conversions like this are not great in general because they can hide complexity and inefficiencies in the code, but this case is especially bad, because after the transition from `boost::filesystem` to `std::filesystem` in #20744 the behavior of this constructor on windows will be more complicated and can mangle path strings. The `fs::path` class also has a `.string()` method which is inverse of the constructor and has the same problems.
Fix this by replacing the unsafe method calls with `PathToString` and `PathFromString` function calls, and by forbidding unsafe method calls in the future.
ACKs for top commit:
kiminuo:
ACK 6544ea5035
laanwj:
Code review ACK 6544ea5035
hebasto:
re-ACK 6544ea5035, only added `fsbridge_stem` test case, updated comment, and rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22937#pullrequestreview-765503126) review. Verified with the following command:
Tree-SHA512: c36324740eb4ee55151146626166c00d5ccc4b6f3df777e75c112bcb4d1db436c1d9cc8c29a1e7fb96051457d317961ab42e6c380c3be2771d135771b2b49fa0
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.
01bff8f049 qt: Fix WalletControllerActivity progress dialog title (Shashwat)
Pull request description:
Throughout the GUI, the title of the window, tells about the purpose of the window. This was not true for the title of wallet loading wallet.
This PR fixes this issue by renaming the wallet loading window title to 'Open Wallet'
Changes introduced in this PR (Runned Bitcoin-GUI on signet network)
|Master|PR|
|---|---|
|||
ACKs for top commit:
jarolrod:
ACK 01bff8f
hebasto:
ACK 01bff8f049, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: cd21c40752eb1c0afb5ec61b8a40e900bc3aa05749963f7957ece6024e4957f5bb37e0eb4f95aac488f5e08aea51fe13b023b05d8302a08c88dcc6790410ba64
The WalletControllerActivity progress dialog had title of "Bitcoin-Qt".
The window title for opening wallet window should be "Open Wallet",
for creating wallet window should be "Create Wallet", and for the window
that is displayed when wallets are being loaded at startup should be
"Load Wallets". This PR fixes that.
Make the watch-only icon in the bottom bar enabled by default for a better user interface.
Currently, it's disabled by default with a 50% opacity which makes it hard to see the icon in dark mode.
Add interfaces::ExternalSigner to let signer objects be passed between
processes and signer code to run in the original process, without
multiple processes linking and running signer code.
There is no change in behavior. This just helps prepare for the
transition from boost::filesystem to std::filesystem by avoiding calls
to methods which will be unsafe after the transaction to std::filesystem
to due lack of a boost::filesystem::path::imbue equivalent and inability
to set a predictable locale.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Kiminuo <kiminuo@protonmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
9d0379cea6 consensus: use <cstdint> over <stdint.h> in amount.h (fanquake)
863e52fe63 consensus: make COIN & MAX_MONEY constexpr (fanquake)
d09071da5b [MOVEONLY] consensus: move amount.h into consensus (fanquake)
Pull request description:
A first step (of a few) towards some source code reorganization, as well as making libbitcoinconsensus slightly more self contained.
Related to #15732.
ACKs for top commit:
MarcoFalke:
concept ACK 9d0379cea6 🏝
Tree-SHA512: 97fc79262dcb8c00996852a288fee69ddf8398ae2c95700bba5b326f1f38ffcfaf8fa66e29d0cb446d9b3f4e608a96525fae0c2ad9cd531ad98ad2a4a687cd6a
2fe69efbc6 qt, wallet: Drop no longer used WalletController::getOpenWallets() (Hennadii Stepanov)
f6991cb906 qt, wallet: Add LoadWalletsActivity class (Hennadii Stepanov)
4a024fc310 qt, wallet, refactor: Move connection to QObject::deleteLater to ctor (Hennadii Stepanov)
f9b633eeab qt, wallet: Move activity progress dialog from data member to local (Hennadii Stepanov)
Pull request description:
This PR improves the GUI responsiveness during initial wallets loading at startup (especially ones that have tons of txs), and shows a standard progress dialog for long loading:
Fixes#247.
ACKs for top commit:
ryanofsky:
Code review ACK 2fe69efbc6. Just suggested changes since last review: squashing commits and dropping unused method (thanks!)
shaavan:
reACK 2fe69efbc6
promag:
Code review ACK 2fe69efbc6.
Tree-SHA512: 2ac3cb48886e0005fc36b3fd0c2b35abd557186be16db3145d753c34d94188e4f4ff14dc07fb0fb7558944f84498204a3988f8284fd56c6d85b47bc9081e71a6
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
10c6929d55 Include vout when copying transaction ID from coin selection (Samuel Dobson)
Pull request description:
Fixes#432
I think it makes sense to just add the vout to the existing function because I can't imagine a situation where a user in the coin selection dialog would want just the transaction ID rather than the specific outpoint, and they can just delete it from the end anyway.
ACKs for top commit:
kristapsk:
ACK 10c6929d55
hebasto:
ACK 10c6929d55, tested on Linux Mint 20.2 (Qt 5.12.8).
shaavan:
ACK 10c6929
Tree-SHA512: df4d132b6c2fd0b590594e91cf54f82c6c0f77ee9ca06296fb726bc3c52b9ae459ca3b50c48b2bf303ccafe832b6b4dba692a812f439991ca6d807ea0d8df934
bd5c826a96 gui: add RPC setting (Sjors Provoost)
Pull request description:
RPC access is disabled by default for the GUI.
With the proliferation of third party desktop applications that use the Bitcoin Core RPC (e.g. Specter Desktop, Sparrow and Wasabi), this PR makes them slight easier to configure. It's no longer required to find and edit `bitcoin.conf` to add `server=1` to it.
<img width="447" alt="Schermafbeelding 2021-09-02 om 14 25 58" src="https://user-images.githubusercontent.com/10217/131844201-be3b49a8-ae88-47e6-8992-e95ee6b70f69.png">
ACKs for top commit:
hebasto:
ACK bd5c826a96, tested on Linux Mint 20.2 (Qt 5.12.8):
shaavan:
reACK bd5c826a96
promag:
Code review ACK bd5c826a96. Just minor fixes to the .ui form since last review.
Tree-SHA512: ab377e2358826096b499013bc3a864b7b63dff9859e96041e93ff0897d2319a35e8b3adcfb8df5f83274466c83d040d4ea18c546699421425c835e6f42562ae0
4832737c7d qt: connection type translator comments (Jarol Rodriguez)
Pull request description:
This PR introduces Qt translator comments for `Connection Type` strings in `guiutil.cpp` as well as `rpcconsole.cpp`.
This is an alternate implementation of the idea presented in the last three commits of #289. It is especially inspired by commit 842f4e834dfe5fd2786a5092f78ea28da1b36e4f.
Per [Qt Dev Notes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Developer-Notes-for-Qt-Code), it is better to not break up strings when not necessary. This way we preserve the full context for translators.
ACKs for top commit:
jonatack:
Code review re-ACK 4832737c7d per `git diff 371e2b9 4832737`, changes are translator comment edits since my review yesterday (thank you for updating)
hebasto:
ACK 4832737c7d
Tree-SHA512: 67e1741e10a2e30cde6d50d3293eec89f0b7641b34463865dc6909d2926cdcf33a7d8c1dc8055d2f85906ad2002cdaa594d37b184d16e2f06614b6c5ad00c982
03a5fe06bd qt: Keep InitExecutor in main gui thread (João Barbosa)
Pull request description:
The `InitExecutor` constructor moves the instance to a dedicated thread. This PR changes that by using `GUIUtil::ObjectInvoke` to run the relevant code in that thread.
A possible follow-up is to ditch the dedicated thread and use `QThreadPool` or even `QtConcurrent::run` (if we want to enable that).
ACKs for top commit:
hebasto:
ACK 03a5fe06bd, tested on Linux Mint 20.2 (Qt 5.12.8).
jarolrod:
ACK 03a5fe06bd
Tree-SHA512: 8b40300371d4c04efb9913600a06ba4899af0b5f50fdb26ea23ec751df6d3bd52f8bd693a5e8f6a94ebf3790583dc96c6070e6878d247dece62347aa9bd99031
7e3ee4cdd0 GUI: Ask user to unlock wallet before signing psbt (Samuel Dobson)
0f3acecf33 Add test that walletprocesspsbt requires unlocked wallet when signing (Samuel Dobson)
0e895212bb Ensure wallet is unlocked before signing in walletprocesspsbt (Samuel Dobson)
Pull request description:
If signing a PSBT, we need to ensure the wallet is unlocked.
Fixes#22874, fixesbitcoin-core/gui#312
ACKs for top commit:
achow101:
ACK 7e3ee4cdd0
lsilva01:
Code Review ACK 7e3ee4cdd0
benthecarman:
ACK 7e3ee4cdd0
Tree-SHA512: 6726a873582747900ab454ea21153a92be86808a4c1517dc2856b389876a2da9e8df1ffa9b567b6bd017038342c3544ecf5ca3c97744e7debe0a5ee65563687d
This ensures that if we're going to add an action to open up
a transaction in a third-party link (block explorer) that it
is seperated into it's own section.
The text for an open third-party tx URL action
is improved by appending the host name with "Show in".
This makes it self-explanatory what the action will do.
Delay wallet client construction until after logging, thread and other
init for two reasons:
- More responsive multiprocess GUI startup. When bitcoin-gui is started
this moves the call from bitcoin-gui to bitcoin-node that spawns
bitcoin-wallet off of the GUI event thread and onto the background GUI
init executor thread.
- Avoids feature_logging.py test failures with bitcoin-node by making
bitcoin-wallet logging start after bitcoin-node logging starts,
because the tests are not written to handle the bitcoin-wallet logging
init code running first.
This partially reverts commit b266b3e0bf,
moving wallet client creation back to the place it was located before.
Add separate init implementations instead of sharing existing bitcoind
and bitcoin-node ones, so they can start to be differentiated in
upcoming commits with node and wallet code no longer linked into the
bitcoin-gui binary and wallet code no longer linked into the
bitcoin-node binary.
e4709c7b56 Start using init makeNode, makeChain, etc methods (Russell Yanofsky)
Pull request description:
Use `interfaces::Init::make*` methods instead of `interfaces::Make*` functions, so interfaces can be constructed differently in different executable without having to change any code. (So for example `bitcoin-gui` can make an `interfaces::Node` pointer that communicates with a `bitcoin-node` subprocess, while `bitcoin-qt` can make an `interfaces::Node` pointer that controls node code in the same process.)
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
jamesob:
reACK e4709c7b56
achow101:
ACK e4709c7b56
benthecarman:
utACK e4709c7b56
Tree-SHA512: 580c1979dbb2ef444157c8e53041e70d15ddeee77e5cbdb34f70b6d228cc2d2fe3843825f172da84e506200c58f7e0932f7cd4c006bb5058c1f4e43259394834
5cc783f5f3 qt: ensure translator comments end in full stop (Jarol Rodriguez)
Pull request description:
This is a follow-up to #318 which addresses this [nit](https://github.com/bitcoin-core/gui/pull/318#discussion_r706856893) by addressing it globally.
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.
ACKs for top commit:
hebasto:
ACK 5cc783f5f3, I have reviewed the code and it looks OK, I agree it can be merged.
shaavan:
Code Review ACK 5cc783f5f3
Tree-SHA512: 67a1d56175c974e0af9b460fa44163f7ce139a7b81cfaf8ed2c0e7fb6d5120957c3135d96010aeb6229689468e36673fe9571b5a8c3e1c07e047aba1bd563444
9bd168bf54 qt: add missing tooltips to options menu settings (Jarol Rodriguez)
Pull request description:
This adds missing tooltips to the text of the `Size of database cache` and the `Number of script verification threads` settings.
All settings in the Options window will now have appropriate tooltip texts.
ACKs for top commit:
jonatack:
ACK 9bd168bf54 tested on Debian 5.10.46-4 (2021-08-03)
hebasto:
ACK 9bd168bf54, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: d71946bfee33c624a8b79eafe514d2c902090a40bc25097be4c7da4a80270f53305002af1b27d5fd082a0f45f838e22036632f9445918c4b8898073b33c09c08
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.
0b869df1c9 qt: Add cancel button to configuration options popup (Shashwat)
Pull request description:
This PR renames the **OK** button to **Continue** and adds a **Cancel** button to the configuration options pop-up.
This feature will give the user an option to abort opening the configuration file if they want to. This is an essential helpful feature that was missing in the master branch.
In some windows managers such as Windows I3. The exit button at the top right corner is missing. So this feature becomes crucial there. And even when the exit button is there, it doesn't prevent the opening of the configuration file even when pressed.
Additionally, it will always be possible to close using Keyboard Shortcut. This PR helps accessibility for those who need to use a mouse.
<table>
<tr>
<td>Master
</td>
<td>PR
</td>
</tr>
<tr>
<td>
</td>
<td>
</td>
</tr>
</table>
ACKs for top commit:
hebasto:
ACK 0b869df1c9, tested on Linux Mint 20.2 (Qt 5.12.8):
prayank23:
tACK 0b869df1c9
Tree-SHA512: c314e8b84064134f028f66f5015eb0f6ba33d5d4174c9ff49dcb5d2b577dce6019f59f9c7913393a415a323ea98c26febf5ca26e3e2102e7a1d31171e01937f1
fade9a1a4d Remove confusing CAddrDB (MarcoFalke)
fa7f77b7d1 Fix addrdb includes (MarcoFalke)
fa3f5d0dae Move addrman includes from .h to .cpp (MarcoFalke)
Pull request description:
Split out from #22762 to avoid having to carry it around in (an)other rebase(s)
ACKs for top commit:
ryanofsky:
Code review ACK fade9a1a4d
lsilva01:
Code Review ACK fade9a1a4d
Tree-SHA512: 7615fb0b6235d0c1e6f8cd6263dd18c4d95890567c2b797fe6fce6cb12cc85ce6eacbe07dbb6d81b05d179ef03b42edfd61c940e35a1044ce6d363b54c2dae5c
fa57fa1a2e Enable clang-tidy bugprone-argument-comment and fix violations (MarcoFalke)
Pull request description:
Named arguments can be dangerous when they are wrong, because they are not enforced by the compiler. Currently there are only minor typos, no actual bugs.
Fix the typos and add the `.clang-tidy` file to make it easier to find them in the future.
ACKs for top commit:
practicalswift:
cr ACK fa57fa1a2e
fanquake:
ACK fa57fa1a2e
Tree-SHA512: b66f01e0a1e77e56ed8454002176df660cc2cc0947a90785aa33cc5b8003a1f99fd8b2f8f89f2a0bf180ff2c42c031d69e669d127bb557b879c17975275a220b
d319c4dae9 qt, refactor: Replace WalletFrame::addWallet with WalletFrame::addView (Hennadii Stepanov)
92ddc02a16 qt, refactor: Declare getWalletModel with const and noexcept qualifiers (Hennadii Stepanov)
ca0e680bdc qt, refactor: Drop redundant checks of walletModel (Hennadii Stepanov)
404373bc6a qt, refactor: Pass WalletModel object to WalletView constructor (Hennadii Stepanov)
Pull request description:
An instance of the `WalletView` class without the `walletModel` data member being set is invalid. So, it is better to set it in the constructor.
Establishing one more `WalletView` class's invariant in constructor:
- allows to drop all of checks of the`walletModel` in member functions
- makes reasoning about the code that uses instances of the `WalletView` class easier
Possible follow ups could extend this approach to other classes, e.g., `OverviewPage`, `TransactionView`, `ReceiveCoinsDialog`, `SendCoinsDialog`, `AddressBookPage`.
ACKs for top commit:
ShaMan239:
Code review ACK d319c4dae9
promag:
Code review ACK d319c4dae9.
jarolrod:
ACK d319c4dae9
Tree-SHA512: b0c61f82811bb5aba2738067b53dc9ea4439230d547ce5c8fd85c480d8d70ea15f9942dbf13842383acbce467fba1ab4e132e37c56b654b46ba897301a41066e
Improve readability of code, simplify future scripted diff cleanup PRs, and be
more consistent with naming for GetBoolArg.
This will also be useful for replacing runtime settings type checking
with compile time checking.
-BEGIN VERIFY SCRIPT-
git grep -l GetArg | xargs sed -i 's/GetArg(\([^)]*\( [0-9]\+\|-1\|port\|BaseParams().RPCPort()\|Params().GetDefaultPort()\|_TIMEOUT\|Height\|_WORKQUEUE\|_THREADS\|_CONNECTIONS\|LIMIT\|SigOp\|Bytes\|_VERSION\|_AGE\|_CHECKS\|Checks() ? 1 : 0\|_BANTIME\|Cache\|BLOCKS\|LEVEL\|Weight\|Version\|BUFFER\|TARGET\|WEIGHT\|TXN\|TRANSACTIONS\|ADJUSTMENT\|i64\|Size\|nDefault\|_EXPIRY\|HEIGHT\|SIZE\|SNDHWM\|_TIME_MS\)\))/GetIntArg(\1)/g'
-END VERIFY SCRIPT-
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
b367745cfe ci: Make Cirrus CI Windows build with --enable-werror (Hennadii Stepanov)
c713bb2b24 Fix Windows build with --enable-werror on Ubuntu Focal (Hennadii Stepanov)
Pull request description:
This PR makes possible to cross-compile Windows build with `--enable-werror --enable-suppress-external-warnings`.
Some problems are fixed, others are silenced.
Also `--enable-werror` is enabled for Cirrus CI Windows build (the last one on Cirrus CI without `--enable-werror`).
ACKs for top commit:
practicalswift:
cr ACK b367745cfe: patch looks correct
laanwj:
Code review ACK b367745cfe
vasild:
ACK b367745cfe
jarolrod:
ACK b367745cfe
Tree-SHA512: 64f5c99b7dad4c0efce80cd45d7074f275bd8411235dc9e0841287bdab64b812c6f8f9d632c35531d0b8210148531f53aaaac77be7699b29d2d6aaae304dbee0
ab1461d5d3 qt: Add copy IP/Netmask action for banned peer (Shashwat)
Pull request description:
This PR adds a Copy IP/Netmask context menu action to the Banned Peers Table.
This feature is helpful if a node using GUI might want to alert its peer about a particular malicious user. So it can copy that user’s IP/Netmask and broadcast it to its peers so they can ban it instantly using the setban command in the console.
| Master | PR |
| ----------- | ----------- |
|  |  |
ACKs for top commit:
jarolrod:
re-ACK ab1461d
hebasto:
re-ACK ab1461d5d3, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: a528f089bd4cb5b51fec987550d21c2587459ad80f854b55850bc62c776c21f3fa31052a17e2b0e9e9d0b3468799c8070ed306543730fb7b324f283847151e17
b8aa84b1a1 qt, refactor: Replace `if` check with `assert` (Hennadii Stepanov)
fcdc8b0fcb qt, refactor: Drop redundant signalling in WalletView::setWalletModel (Hennadii Stepanov)
37dcf161d3 qt, refactor: Emit WalletView::encryptionStatusChanged signal directly (Hennadii Stepanov)
7d0d4c0490 qt: Add WalletFrame::currentWalletSet signal (Hennadii Stepanov)
Pull request description:
This PR makes signal-slot paths to reach `setHDStatus` and `setEncryptionStatus` functions shorter and easier to reason about them.
Required to simplify #398 (see https://github.com/bitcoin-core/gui/pull/398#discussion_r686094883).
---
**Note for reviewers.** Please verify that "Encrypt Wallet..." menu item, and the following icons
and updated properly in each and every possible scenario.
ACKs for top commit:
jarolrod:
tACK b8aa84b1a1
Talkless:
Code review ACK b8aa84b1a1. Did build on Debian Sid with Qt 5.15.2 but no actual testing performed.
ryanofsky:
Code review ACK b8aa84b1a1. Only change since last review was rebase
Tree-SHA512: 275737cdba02baff71049df41bc24089e916f96326dd2dea26ec607c7949cb3aae368eeabbe3ad5a0a27651503a1d65536873726de854c5f6af259bcc29727e7
Use interfaces::Init::make* methods instead of interfaces::Make*
functions, so interfaces can be constructed differently in different
executables without having to change any code. (So for example
bitcoin-gui can make an interfaces::Node pointer that communicates with
a bitcoin-node subprocess, while bitcoin-qt can make an interfaces::Node
pointer that starts node code in the same process.)
The connection of the WalletFrame::currentWalletSet signal to
the BitcoinGUI::updateWalletStatus is a shorter and more descriptive way
to call both the setHDStatus and setEncryptionStatus member functions of
the BitcoinGUI class in comparison to using of the
WalletView::updateEncryptionStatus slot.
93cc53a2b2 gui: Unregister wallet notifications before unloading wallets (Russell Yanofsky)
Pull request description:
This change was originally part of both bitcoin/bitcoin#10102 and bitcoin/bitcoin#19101 and is required for both because it avoids the IPC wallet implementation in bitcoin/bitcoin#10102 and the WalletContext implementation in bitcoin/bitcoin#19101 needing to deal with notification objects that have stale pointers to deleted wallets.
ACKs for top commit:
promag:
Code review ACK 93cc53a2b2.
hebasto:
ACK 93cc53a2b2
Tree-SHA512: 805f50a493291ad0f7c48725fbc5058d58ebbdb0770befd51d8aa241209a13f8a46f5982481336ab8338cdc83e9017668089a71deccf1587308e841cf8697825
62b125fd19 qt, refactor: Fix indentation (Prateek Sancheti)
ad28b66e98 qt: Add SubFeeFromAmount option (Prateek Sancheti)
Pull request description:
This PR adds **_SubFeeFromAmount_** option which lets the user select their preferred setting of whether fee for a transaction is to be subtracted from the amount or not for future transactions. The setting chosen by the user is remembered even when the GUI mode is turned off.
**_Functionality and Usage:_**
- Go to `Settings > Options > Wallet` on _Windows/Linux_ or `bitcoin-qt > Preferences > Wallet` on _macOS_.
- The checkbox **Subtract Fee From Amount** corresponds to the added option **SubFeeFromAmount**.
- The preferred setting intended to be the default for all future send transactions should be selected by the user.
- Click on **OK**.
- Go to the **Send** tab in the wallet.
- You shall notice, any new Send transaction created will have the preferred setting as chosen by the user.<br> (Try clicking on Add recipient or even restarting the Node in GUI)
Attaching ScreenRecordings to explain the added feature.
> Master.mov: Master Branch
https://user-images.githubusercontent.com/54016434/127763378-be91837d-d0ab-4ae5-87c0-d303fa70a336.mov
> PR.mov: PullRequest
https://user-images.githubusercontent.com/54016434/127763404-05b834c1-4082-4fbd-9b05-1528ac898a21.movClose#386
ACKs for top commit:
Talkless:
tACK 62b125fd19, tested on Debian Sid with 5.15.2 and it works as described.
hebasto:
re-ACK 62b125fd19, only removed the unused `SubFeeFromAmountChanged` signal since my [previous](https://github.com/bitcoin-core/gui/pull/390#pullrequestreview-726531766) review.
meshcollider:
utACK 62b125fd19
Tree-SHA512: 932ca89ae578a1e1c426561400d87cf005c231944feaf0f662ff8d88f32bdd65a927a090ea41510a15f8ec0ebcd5529672e9917720eb5ea85f413f081e45d5bb
0237d95323 qt: Add Load PSBT functionaliy with nowallet (Prateek Sancheti)
Pull request description:
This PR provides a fix to the issue mentioned in #232.
Currently, the **_Load PSBT_** functionality works well in case a wallet is loaded but does nothing when a wallet isn't loaded.
If a function cannot work without a wallet being loaded, it is disabled by default (It is unclickable as shown in the image).
For e.g. One cannot `Close Wallet` or `Backup Wallet` or `Sign Messages` without a wallet being loaded. And hence they are disabled. But if you notice, `Load PSBT` options are not disabled by default even when a wallet isn't loaded.
> 
As mentioned by hebasto in the issue description :
```
<hebasto> achow101: does "File" -> "Load PSBT from {file|clipboard}" make any sense when no wallet is loaded?
<achow101> hebasto: yes, for finalize and sending
```
This means **_Load PSBT_** should be working just as similar whether wallets are being loaded or not.
After making the required changes to the code, The **_Load PSBT_** works as expected even with no wallet loaded and the PSBT is finalized.
| Master | PR |
|-------------|---------------|
|  |  |
Close#232
ACKs for top commit:
achow101:
re-ACK 0237d95323
hebasto:
ACK 0237d95323, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: 8d928c5bfd3c2b286ddcacd0b367c872de8bc3d3d9d82280faeadc60d738b86af328c060b5763ade364c9b386b23f95580c2eb1147b16373fbb713170c100350
6971e790c3 gui: add Direction column to peers tab (Jon Atack)
Pull request description:
Picking up #289
This adds a `Direction column`, making the peers tab the same as the `Direction/Type` row in the peer details and the direction and type columns in our other user-facing peer connections table in `-netinfo`.
Users can now sort the peers table by direction. The default sort is set to inbound, then outbound.
| Master | PR |
| ----------- | ----------- |
|  |  |
ACKs for top commit:
jonatack:
Tested ACK 6971e790c3
ShaMan239:
tACK 6971e790c3
hebasto:
ACK 6971e790c3, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: 9716cdedd435f88245a097fed6d4b2b486104d0dd09df739bdb4f2bfad709cbd9c9a231168cc3326e94fa5fddc77dd68f992f20417d04d94930db9fccdbb7de1
4830f4912a qt: Refactor open date range to use std::optional (João Barbosa)
Pull request description:
Use `std::nullopt` for open date range instead of `TransactionFilterProxy::MIN_DATE` and `TransactionFilterProxy::MAX_DATE`.
ACKs for top commit:
hebasto:
re-ACK 4830f4912a, only missed header included since my [previous](https://github.com/bitcoin-core/gui/pull/354#pullrequestreview-682108182) review.
Talkless:
tACK 4830f4912a, tested on Debian Sid, filtering seems to work as expected.
Tree-SHA512: dcecbcc129cb401d6ac13a20f015b8cb2a7434fae6bd3e5b19fca5531e8bd915e2a0835f9c601371381750cdc8cd6fcf4f8c6669177d679773046cbe13bed68b
a9b9ca82da gui: ensure external signer option remains disabled without signers (Andrew Chow)
Pull request description:
When no external signers are available, the option to enable external signers should always be disabled. However the encrypt wallet checkbox can erroneously re-enable the external signer checkbox. To avoid this, CreateWalletDialog now stores whether signers were available during setSigners so that future calls to external_signer_checkbox->setEnabled can account for whether signers are available.
Fixes#395
ACKs for top commit:
hebasto:
ACK a9b9ca82da, tested on Linux Mint 20.2 (Qt 5.12.8).
Sjors:
tACK a9b9ca82da
jarolrod:
ACK a9b9ca82da
Tree-SHA512: 98951bcadc23fce99a66ea2d367c44360989e888c253845a767e1f7085c594562d0f099de4130f4a078c5072aa7806294097d976ee6407291f3d3c5a4a608b44
When no external signers are available, the option to enable external
signers should always be disabled. However the encrypt wallet checkbox
can erroneously re-enable the external signer checkbox. To avoid this,
CreateWalletDialog now stores whether signers were available during
setSigners so that future calls to external_signer_checkbox->setEnabled
can account for whether signers are available.
6969b2bb98 qt, test: use regex search in apptests (Jarol Rodriguez)
d09d1cf1a2 qt, test: introduce FindInConsole function (Jarol Rodriguez)
Pull request description:
This PR refactors our GUI `apptests` so that it uses regex search to find values in our console/qtextedit output regardless if it is in `plaintext`, `html`, or `markdown`.
This introduces a new function `FindInConsole` which uses [QRegularExpression](https://doc.qt.io/qt-5/qregularexpression.html) to search the output of the console. The function must be provided with a [perl compatible regex](https://www.debuggex.com/cheatsheet/regex/pcre) pattern which wants to match a single group. The function then returns the matched group. If no match is found, an empty `QString` is returned.
We then use this new function in `TestRpcCommand` to find the current `chain` value instead of reading with univalue.
This approach can apply to a wider variety of testing scenarios as we can reuse this function to search for values when the console output is exported in a different format than `plaintext`. As an example, A follow up PR will add tests for console resizing and needs to look for the size in `html` tags after exporting the console text with `toHtml()`.
ACKs for top commit:
hebasto:
ACK 6969b2bb98
ShaMan239:
ACK 6969b2bb98
Tree-SHA512: 4db8bcd4a1acc4539ca64bbd7de572fe7dd6afc3e95108235abfc2891585bc4db3a56a33928fa38e8d44ac87023ce0dee3abcfadfbcd4440e3a21a52fef02536
8169fc4e73 qt, refactor: Fix code styling of moved InitExecutor class (Hennadii Stepanov)
c82165a557 qt, refactor: Move InitExecutor class into its own module (Hennadii Stepanov)
dbcf56b6c6 scripted-diff: Rename BitcoinCore class to InitExecutor (Hennadii Stepanov)
19a1d00831 qt: Add BitcoinCore::m_thread member (Hennadii Stepanov)
Pull request description:
This PR makes the `BitcoinCore` class reusable, i.e., it can be used by the widget-based GUI or by the [QML-based](https://github.com/bitcoin-core/gui-qml/tree/main/src/qml) one, and it makes the divergence between these two repos minimal.
The small benefit to the current branch is more structured code.
Actually, this PR is ported from https://github.com/bitcoin-core/gui-qml/pull/10.
The example of the re-using of the `BitcoinCore` class is https://github.com/bitcoin-core/gui-qml/pull/11.
ACKs for top commit:
laanwj:
ACK 8169fc4e73
ryanofsky:
Code review ACK 8169fc4e73. Only change is switching from `m_executor` from pointer to optional type (thanks for update!)
Tree-SHA512: a0552c32d26d9acf42921eb12bcdf68f02d52f7183c688c43257b1a58679f64e45f193ee2d316850c7f0f516561e17abe989fe545bfa05e158ad3f4c66d19bca
3c4c8e79ba build: Add -Werror=implicit-fallthrough compile flag (Hennadii Stepanov)
014110c47d Use C++17 [[fallthrough]] attribute, and drop -Wno-implicit-fallthrough (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
fanquake:
ACK 3c4c8e79ba - looks ok to me now. Checked that warnings occur in our code & leveldb by removing a `[[fallthrough]]` or `FALLTHROUGH_INTENDED`.
jarolrod:
ACK 3c4c8e79ba
theStack:
ACK 3c4c8e79ba
Tree-SHA512: 4dce91f0f26b8a3de09bd92bb3d7e1995e078e3a8b3ff861c4fbf6c0b32b2327d063633b07b89c4aa94a1141d7f78d46d9d43ab8df865273e342693ad30645b6
986bf78d7e qt: Emit dataChanged signal to dynamically re-sort Peers table (Hennadii Stepanov)
Pull request description:
[By default](https://doc.qt.io/qt-5/qsortfilterproxymodel.html#details), the `PeerTableSortProxy`
> dynamically re-sorts ... data whenever the original model changes.
That is not the case on master (8cdf91735f) as in ecbd911538 (#164) no signals are emitted to notify about model changes.
This PR uses a dedicated [`dataChanged`](https://doc.qt.io/qt-5/qabstractitemmodel.html#dataChanged) signal.
Fixes#367.
An alternative to #374.
ACKs for top commit:
jarolrod:
ACK 986bf78d7e
Tree-SHA512: dcb92c2f9a2c632880429e9528007db426d2ad938c64dfa1f1538c03e4b62620df52ad7daf33b582976c67b472ff76bc0dae707049f4bbbd4941232cee9ce3d4
cd46c11577 qt: Draw "eye" sign at the beginning of watch-only addresses (Hennadii Stepanov)
9ea1da6fc9 qt: Do not extend recent transaction width to address/label string (Hennadii Stepanov)
Pull request description:
This PR guaranties that the "eye" sign won't be hidden for very long addresses/labels.
No longer need to extend `TransactionOverviewWidget` widget width to make "eye" signs shown:
Fixes https://github.com/bitcoin-core/gui/issues/373
ACKs for top commit:
jarolrod:
ACK cd46c11577
Tree-SHA512: 0602b5bb65d53c5b18e86260750006bba03adbae181917b5a2b7f89b17290bd1f57b4f80adaba32f42cc6fb468598a888b12c0b6b09005d2f2c07bd4d1ad334a
2f23ad2c40 qt: allow prompt icon to be colorized (Jarol Rodriguez)
Pull request description:
Opening the console on macOS, while in dark mode, the console prompt icon will not be colorized white like other icons. This applies the `platformStyle` to the icon so that It can be colorized white.
While here, refactor the `promptIcon` widget from a `QPushButton` to `QLabel`; which is more appropriate, per [Qt Docs](https://doc.qt.io/qt-5/qlabel.html#details):
> QLabel is used for displaying text or an image. No user interaction functionality is provided.
| Master | PR |
| ----------- | ----------- |
|  |  |
ACKs for top commit:
hebasto:
ACK 2f23ad2c40
Tree-SHA512: 21f8b1610e4820c9064bbd08608b5467e5b9499e2a3b149ff223e37b60e7d560497255c733eafa5434628a84b9f7b7c91d8b0f34b02be2f9ceb3ab21a4d555a8
9d5bf6bf01 GUI: Always call parent changeEvent handler (Luke Dashjr)
c901d4d8ce GUI: Enable palette change adaptation on all platforms (Luke Dashjr)
Pull request description:
The changes to support macOS "Dark Mode" are valid for any platform, and should work so long as Qt implements the PaletteChange event. (Worst case, we're no worse off with trying.)
Additionally, we shouldn't block the parent classes from implementing event handlers. Who knows what side effects that could have.
ACKs for top commit:
hebasto:
ACK 9d5bf6bf01, tested on Linux Mint 20.1 (Qt 5.12.8) with the [`qt5ct`](https://packages.ubuntu.com/focal/qt5ct) package installed.
kristapsk:
ACK 9d5bf6bf01. Tested on Gentoo Linux with Xfce4 and Qt 5.15.2, does not break anything on my computer.
Tree-SHA512: dce2fff0ff129eda208132390a37424ff9607539287dbdbfdfd659ed9c4ea0472541e987489a04fd935e391dc006a35bfc9cfa9bcff33602b7dbd29b81c51626
