8b6f5aabb9 qt: Replace QFontMetrics::width() with TextWidth() (Hennadii Stepanov)
Pull request description:
Compiling master (d8fc997913) on macOS Catalina (with a patch from #16720) reveals one more instance of `QFontMetrics::width()` which is supposed to be replaced with `TextWidth()` in the merged #16701.
Sorry for incomplete solution provided in #16701. It’s especially sad that the line I missed lies in only 7 lines from the code touched in #16701.
ACKs for top commit:
fanquake:
ACK 8b6f5aabb9
Tree-SHA512: 65cd8bea550150e5ee47c1e906d8c2393547cf4feba3701a933a4f24fad5ecdb552ac2de4e1200ed14efaa0df0480150dd58fccbddc3b902f6c2141603874902
c53667764e qt: Remove obsolete QModelIndex::child() (Hennadii Stepanov)
Pull request description:
The `QModelIndex::child()` member function is [obsolete](https://doc.qt.io/qt-5.12/qmodelindex-obsolete.html) since Qt 5.12.
This PR removes it, does not change behavior and keeps compatibility with [Qt 5.5.1](https://github.com/bitcoin/bitcoin/pull/15393).
Here is an excerpt from the master build log:
```
qt/receivecoinsdialog.cpp: In member function ‘void ReceiveCoinsDialog::copyColumnToClipboard(int)’:
qt/receivecoinsdialog.cpp:264:111: warning: ‘QModelIndex QModelIndex::child(int, int) const’ is deprecated: Use QAbstractItemModel::index [-Wdeprecated-declarations]
GUIUtil::setClipboard(model->getRecentRequestsTableModel()->data(firstIndex.child(firstIndex.row(), column), Qt::EditRole).toString());
^
In file included from /home/hebasto/Qt/5.13.0/gcc_64/include/QtWidgets/qabstractitemview.h:45:0,
from /home/hebasto/Qt/5.13.0/gcc_64/include/QtWidgets/qheaderview.h:44,
from /home/hebasto/Qt/5.13.0/gcc_64/include/QtWidgets/QHeaderView:1,
from ./qt/guiutil.h:12,
from ./qt/receivecoinsdialog.h:8,
from qt/receivecoinsdialog.cpp:7:
/home/hebasto/Qt/5.13.0/gcc_64/include/QtCore/qabstractitemmodel.h:457:20: note: declared here
inline QModelIndex QModelIndex::child(int arow, int acolumn) const
^~~~~~~~~~~
```
ACKs for top commit:
laanwj:
Code review ACK c53667764e
promag:
ACK c53667764e, just read the change.
jonasschnelli:
utACK c53667764e
Tree-SHA512: 99fcb6ff60a6d47b925bda9f14006269eaad09ba4f7a41ac4975c6cf04bd906b58aed721cbfa0be7da8e6613d92e30d4be18b7e4d3960f026c7226558a4c3196
0912134039 qt: Remove QSignalMapper from TransactionView (Hennadii Stepanov)
9e0c1d676c qt: Remove QSignalMapper from RPCConsole (Hennadii Stepanov)
Pull request description:
The [`QSignalMapper`](https://doc.qt.io/qt-5/qsignalmapper.html) class has been [deprecated](https://doc-snapshots.qt.io/qt5-5.10/obsoleteclasses.html) since Qt 5.10.
This PR replaces it by lambdas and does not change behavior.
ACKs for top commit:
jonasschnelli:
utACK 0912134039
Tree-SHA512: 0c102d5cab4adc8b6252f72e07123ac87c65434c88cada3e72816ecea8fc4803f15b9c050fb5e1c7e8a96f709265521fd6813ab1890dbf5634032f7ee0d50675
c6dd32da69 qt: Replace obsolete functions of QDesktopWidget (Hennadii Stepanov)
1260ecd812 qt: Add TextWidth() wrapper (Hennadii Stepanov)
Pull request description:
The following functions are obsolete in Qt 5.13:
- [`QFontMetrics::width()`](https://doc.qt.io/qt-5/qfontmetrics-obsolete.html#width)
- [`QDesktopWidget::availableGeometry()`](https://doc.qt.io/qt-5/qdesktopwidget-obsolete.html#availableGeometry)
- [`QDesktopWidget::screenGeometry()`](https://doc.qt.io/qt-5/qdesktopwidget-obsolete.html#screenGeometry)
This PR replaces them and does not change behavior.
Here are some excerpts from the master build log:
```
qt/bitcoingui.cpp: In constructor ‘BitcoinGUI::BitcoinGUI(interfaces::Node&, const PlatformStyle*, const NetworkStyle*, QWidget*)’:
qt/bitcoingui.cpp:84:57: warning: ‘const QRect QDesktopWidget::availableGeometry(int) const’ is deprecated: Use QGuiApplication::screens() [-Wdeprecated-declarations]
move(QApplication::desktop()->availableGeometry().center() - frameGeometry().center());
^
In file included from /home/hebasto/Qt/5.13.0/gcc_64/include/QtWidgets/QDesktopWidget:1:0,
from qt/bitcoingui.cpp:43:
/home/hebasto/Qt/5.13.0/gcc_64/include/QtWidgets/qdesktopwidget.h:88:67: note: declared here
QT_DEPRECATED_X("Use QGuiApplication::screens()") const QRect availableGeometry(int screen = -1) const;
^~~~~~~~~~~~~~~~~
```
```
qt/bitcoingui.cpp:1410:74: warning: ‘int QFontMetrics::width(const QString&, int) const’ is deprecated: Use QFontMetrics::horizontalAdvance [-Wdeprecated-declarations]
max_width = qMax(max_width, fm.width(BitcoinUnits::longName(unit)));
^
In file included from /home/hebasto/Qt/5.13.0/gcc_64/include/QtWidgets/qwidget.h:50:0,
from /home/hebasto/Qt/5.13.0/gcc_64/include/QtWidgets/qdialog.h:44,
from /home/hebasto/Qt/5.13.0/gcc_64/include/QtWidgets/QDialog:1,
from ./qt/optionsdialog.h:8,
from ./qt/bitcoingui.h:12,
from qt/bitcoingui.cpp:5:
/home/hebasto/Qt/5.13.0/gcc_64/include/QtGui/qfontmetrics.h:108:9: note: declared here
int width(const QString &, int len = -1) const;
^~~~~
```
```
qt/splashscreen.cpp: In constructor ‘SplashScreen::SplashScreen(interfaces::Node&, Qt::WindowFlags, const NetworkStyle*)’:
qt/splashscreen.cpp:127:50: warning: ‘const QRect QDesktopWidget::screenGeometry(int) const’ is deprecated: Use QGuiApplication::screens() [-Wdeprecated-declarations]
move(QApplication::desktop()->screenGeometry().center() - r.center());
^
In file included from /home/hebasto/Qt/5.13.0/gcc_64/include/QtWidgets/QDesktopWidget:1:0,
from qt/splashscreen.cpp:24:
/home/hebasto/Qt/5.13.0/gcc_64/include/QtWidgets/qdesktopwidget.h:79:67: note: declared here
QT_DEPRECATED_X("Use QGuiApplication::screens()") const QRect screenGeometry(int screen = -1) const;
^~~~~~~~~~~~~~
```
ACKs for top commit:
jonasschnelli:
utACK c6dd32da69
Tree-SHA512: deb7bcbf86e1dcc6508bd91288772c2fe8811db79fa2011de37d0469cdd094fbf7fd8c4512c607bed0bd08dc2968e893c0bbc190732c43c69ed1085259df766c
In Qt 5.12 and before the QFontMetrics::width() is used and it is
deprecated since Qt 13.0.
In Qt 5.11 the QFontMetrics::horizontalAdvance() was introduced.
If sending to multiple recipients and one of the recipient fields is malformed,
the highlighted field may not be visible due to being scrolled out of view,
leading to a confusing lack of error feedback when clicking Send. To avoid this
problem ensure the first field containing an error is scrolled into view
when Send is clicked.
bca388db0d gui: remove unused PlatformStyle::TextColorIcon (fanquake)
Pull request description:
This is unused after #16612.
ACKs for top commit:
emilengler:
Concept ACK bca388d
practicalswift:
ACK bca388db0d -- diff looks correct & less cruft is better
hebasto:
ACK bca388db0d, I have reviewed the code and it looks OK, I agree it can be merge.
jonasschnelli:
utACK bca388db0d
Tree-SHA512: ae5c5cfc02cf2e6f138e75e227320f4c430b93bd529a8ead7ba971ceb5bbc997cdb721c6fd8a46a5a897008f0bc6e519422d6beef240b9d2e28ecf4f8cbbcc6a
ffea41f530 Enable all tests in feature_config_args.py (Hennadii Stepanov)
66f5c17f8a Use CheckDataDirOption() for code uniformity (Hennadii Stepanov)
7e33a18a34 Fix datadir handling in bitcoin-cli (Hennadii Stepanov)
b28dada374 Fix datadir handling in bitcoin-qt (Hennadii Stepanov)
50824093bb Fix datadir handling in bitcoind (Hennadii Stepanov)
740d41ce9f Add CheckDataDirOption() function (Hennadii Stepanov)
c1f325126c Return absolute path early in AbsPathForConfigVal (Hennadii Stepanov)
Pull request description:
Fix#15240, see: https://github.com/bitcoin/bitcoin/issues/15240#issuecomment-487353760Fix#15745
Fix broken `feature_config_args.py` tests (disabled by MarcoFalke@fabe28a0cdcfa13e0e595a0905e3642a960d3077). All test are enabled now.
This PR is alternative to #13621.
User's `$HOME` directory is not touched unnecessarily now.
~To make reviewing easier only `bitcoind` code is modified (neither `bitcoin-cli` nor `bitcoin-qt`).~
Refs:
- https://github.com/bitcoin/bitcoin/issues/15745#issuecomment-479852569 by **laanwj**
- #16220
Top commit has no ACKs.
Tree-SHA512: 4a4cda10e0b67c8f374da0c9567003d2b566d948e7f8550fe246868b5794c15010e88ea206009480b9cd2f737f310a15e984f920730448f99a895893bed351df
a2714a5c69 Give QApplication dummy arguments (Andrew Chow)
Pull request description:
QApplication takes the command line arguments and parses them itself for some [built in command line arguments](https://doc.qt.io/qt-5/qapplication.html#QApplication) that it has. We don't want any of those built in arguments, so instead give it dummy arguments.
To test, you can use the `-reverse` option. Without this patch, everything will appear right-to-left; things that were on the left side will be on the right and everything is right aligned.
After this patch, `-reverse` will now give a startup error since we do not support this argument.
ACKs for top commit:
laanwj:
ACK a2714a5c69
hebasto:
ACK a2714a5c69
fanquake:
ACK a2714a5c69 - Have tested that arguments like `-reverse` are no longer being passed through and result in an error.
Tree-SHA512: 983bd948ca6999f895b6662b58c37e33af7ed61fdd600c6b4623febb87ec06a92c66e3b3300783530110cc711902793ef82d751d7f563696c4c3a8416b2b1f51
c5b404e8f1 Add functional tests for flexible whitebind/list (nicolas.dorier)
d541fa3918 Replace the use of fWhitelisted by permission checks (nicolas.dorier)
ecd5cf7ea4 Do not disconnect peer for asking mempool if it has NO_BAN permission (nicolas.dorier)
e5b26deaaa Make whitebind/whitelist permissions more flexible (nicolas.dorier)
Pull request description:
# Motivation
In 0.19, bloom filter will be disabled by default. I tried to make [a PR](https://github.com/bitcoin/bitcoin/pull/16176) to enable bloom filter for whitelisted peers regardless of `-peerbloomfilters`.
Bloom filter have non existent privacy and server can omit filter's matches. However, both problems are completely irrelevant when you connect to your own node. If you connect to your own node, bloom filters are the most bandwidth efficient way to synchronize your light client without the need of some middleware like Electrum.
It is also a superior alternative to BIP157 as it does not require to maintain an additional index and it would work well on pruned nodes.
When I attempted to allow bloom filters for whitelisted peer, my proposal has been NACKed in favor of [a more flexible approach](https://github.com/bitcoin/bitcoin/pull/16176#issuecomment-500762907) which should allow node operator to set fine grained permissions instead of a global `whitelisted` attribute.
Doing so will also make follow up idea very easy to implement in a backward compatible way.
# Implementation details
The PR propose a new format for `--white{list,bind}`. I added a way to specify permissions granted to inbound connection matching `white{list,bind}`.
The following permissions exists:
* ForceRelay
* Relay
* NoBan
* BloomFilter
* Mempool
Example:
* `-whitelist=bloomfilter@127.0.0.1/32`.
* `-whitebind=bloomfilter,relay,noban@127.0.0.1:10020`.
If no permissions are specified, `NoBan | Mempool` is assumed. (making this PR backward compatible)
When we receive an inbound connection, we calculate the effective permissions for this peer by fetching the permissions granted from `whitelist` and add to it the permissions granted from `whitebind`.
To keep backward compatibility, if no permissions are specified in `white{list,bind}` (e.g. `--whitelist=127.0.0.1`) then parameters `-whitelistforcerelay` and `-whiterelay` will add the permissions `ForceRelay` and `Relay` to the inbound node.
`-whitelistforcerelay` and `-whiterelay` are ignored if the permissions flags are explicitly set in `white{bind,list}`.
# Follow up idea
Based on this PR, other changes become quite easy to code in a trivially review-able, backward compatible way:
* Changing `connect` at rpc and config file level to understand the permissions flags.
* Changing the permissions of a peer at RPC level.
ACKs for top commit:
laanwj:
re-ACK c5b404e8f1
Tree-SHA512: adfefb373d09e68cae401247c8fc64034e305694cdef104bdcdacb9f1704277bd53b18f52a2427a5cffdbc77bda410d221aed252bc2ece698ffbb9cf1b830577
6285a318d7 Remove redundant WalletController::addWallet slot (Hennadii Stepanov)
Pull request description:
~~Fix #15453.~~ It is fixed by https://github.com/bitcoin/bitcoin/pull/16348#issuecomment-509308347
The _only_ reason of these lines on master (8c69fae944)
2679bb8919/src/qt/walletcontroller.cpp (L121-L128)
is to `Q_EMIT walletAdded(wallet_model);` in a thread-safe manner;
This PR makes this in a line of code:
1b83875006/src/qt/walletcontroller.cpp (L121)
EDITED:
To establish the ownership of a new `WalletModel` object is not necessary on the master (https://github.com/bitcoin/bitcoin/pull/16349#discussion_r301679192 by **promag**).
But:
> it's good habit to set ownership
And I agree. It is a safe practice.
ACKs for top commit:
promag:
ACK 6285a318d7.
jonasschnelli:
utACK 6285a318d7
ryanofsky:
utACK 6285a318d7. Only change since last review is rebasing and restoring a deleted comment. I do think the comments I suggested last review would be better than this one, but this is at least better than before.
Tree-SHA512: 90370cb1fe853b84dd16c3781ba4f97f3f4deca56bba0203e457f37b3220fd13228cf8495fd882ff18b7c782c27544cc2e7a88aaec5b69b9ef6d8626bdaaf332
QApplication takes the command line arguments and parses them itself
for some built in command line arguments that it has. We don't want
any of those built in arguments, so instead give it dummy arguments.
e6f649cb2c test: Make tests arg type specific (Hennadii Stepanov)
b70cc5d733 Revamp option negating policy (Hennadii Stepanov)
db08edb303 Replace IsArgKnown() with FlagsOfKnownArg() (Hennadii Stepanov)
dde80c272a Use ArgsManager::NETWORK_ONLY flag (Hennadii Stepanov)
9a12733508 Remove unused m_debug_only member from Arg struct (Hennadii Stepanov)
fb4b9f9e3b scripted-diff: Use ArgsManager::DEBUG_ONLY flag (Hennadii Stepanov)
1b4b9422ca scripted-diff: Use Flags enum in AddArg() (Hennadii Stepanov)
265c1b58d8 Add Flags enum to ArgsManager (Hennadii Stepanov)
e0d187dfeb Refactor InterpretNegatedOption() function (Hennadii Stepanov)
e0e18a1017 refactoring: Check IsArgKnown() early (Hennadii Stepanov)
Pull request description:
This PR adds the `Flags` enum to the `ArgsManager` class. Also the `m_flags` member is added to the `Arg` struct. Flags denote an allowed type of an arg value and special hints.
This PR is only a refactoring and does not change behavior.
ACKs for top commit:
jamesob:
ACK e6f649cb2c
MarcoFalke:
ACK e6f649cb2c thanks for adding types to the command line options
Tree-SHA512: b867f8a9cbce2d2473c293d534af662d8cd5be15060ff0682e97af678974bdaac35e8bc6328ccba32f105034bcd38f169b92a6fb67798667891ce14d5d2a2dea
4057b7acb7 wallet: Recognize -disablewallet option early (Hennadii Stepanov)
Pull request description:
This PR makes early check for the `-disablewallet` option.
If `-disablewallet=1`, objects `PaymentServer` and `WalletController` are nor created.
ACKs for top commit:
jonasschnelli:
utACK 4057b7acb7
laanwj:
ACK 4057b7acb7
Tree-SHA512: 74633cd1eacd0914c73712e6dff190255b5378595cfee7eaeb91e17671fc9120928034739f4ae1c53b86f46c4b400390877241384376b2fc534de326d3ab0944
4d94916f0d Get rid of PendingWalletTx class. (Russell Yanofsky)
Pull request description:
No reason for this class to exist if it doesn't have any code to run in the destructor. e10e1e8db0 from https://github.com/bitcoin/bitcoin/pull/16208 recently removed the destructor code that would return an unused key if the transaction wasn't committed.
This is just cleanup, there's no change in behavior.
ACKs for top commit:
ariard:
utACK 4d94916. Successfully built both `bitcoind` and `bitcoin-qt`. `PendingWalletTx` was only a wrapper to enforce call to `ReturnDestination` if `CommitTransaction` doesn't `KeepDestination` before.
promag:
ACK 4d94916f0d, refactor looks good to me.
meshcollider:
utACK 4d94916f0d
Tree-SHA512: f3f93d2f2f5d8f1e7810d609d881c1b1cbbaa8629f483f4293e20b3210292605e947bc4903fde9d2d8736277ca3bd6de182f7eac1e13515d5a327f2ebc130839
This prevents premature tries to access or create the default datadir.
This is useful when the -datadir option is specified and the default
datadir is unreachable.
-BEGIN VERIFY SCRIPT-
sed -i 's/inline std::string _(const char\* psz)/inline bilingual_str _(const char\* psz)/' src/util/translation.h
sed -i 's/return G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz;/return bilingual_str{psz, G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz};/' src/util/translation.h
sed -i 's/\b_("\([^"]\|\\"\)*")/&.translated/g' $(git grep --files-with-matches '\b_("' src)
echo Hard cases - multiline strings.
sed -i 's/"Visit %s for further information about the software.")/&.translated/g' src/init.cpp
sed -i "s/\"Only rebuild the block database if you are sure that your computer's date and time are correct\")/&.translated/g" src/init.cpp
sed -i 's/" restore from a backup.")/&.translated/g' src/wallet/db.cpp
sed -i 's/" or address book entries might be missing or incorrect.")/&.translated/g' src/wallet/wallet.cpp
echo Special case.
sed -i 's/_(COPYRIGHT_HOLDERS)/&.translated/' src/util/system.cpp test/lint/lint-format-strings.py
-END VERIFY SCRIPT-
fa6f402bde Call node->initError instead of InitError from GUI code (Russell Yanofsky)
fad2502240 init: Use InitError for all errors in bitcoind/qt (MarcoFalke)
Pull request description:
Using the same InitError for startup error in the daemon and the gui makes it possible to run the tests with the gui again:
```sh
BITCOIND=bitcoin-qt ./test/functional/test_runner.py feature_includeconf feature_config_args
ACKs for top commit:
hebasto:
ACK fa6f402bde
ryanofsky:
utACK fa6f402bde. Only changes since last review are removing more includes and adding Node::initError method to avoid accessing node `InitError` function and global variables from GUI code.
Tree-SHA512: bd19e08dcea4019dfe40356bc5c63cb583cefed54b6c9dcfb82f1b5b00308d8e2b363549afcaea5e93bf83864dbe0917400c3b70f43a8a5bdff45c9cd34cc294
No reason for this class to exist if it doesn't have any code to run in the
destructor. e10e1e8db0 from
https://github.com/bitcoin/bitcoin/pull/16208 recently removed code destructor
code that would return an unused key if the transaction wasn't committed.
ae311bc036 Fix autostart filenames on Linux (Hennadii Stepanov)
Pull request description:
Currently, on master the `bitcoin-test.lnk` and `bitcoin-regtest.lnk` files do not work as autostart application `.desktop` files.
This PR fixes it.
Refs:
- #7045
- [Autostart Of Applications During Startup](https://standards.freedesktop.org/autostart-spec/autostart-spec-latest.html)
ACKs for top commit:
promag:
utACK ae311bc, weird why extension `.lnk` was used in #7045.
laanwj:
Code review ACK ae311bc036
Tree-SHA512: 210cc346600d52b0a262c81ed5f258365a3cea2e5522f4b5f4798fd3b54f45ed82aba68eefae59a6b6f1d8e4d00221476c23bdffc038f16f2f45c1acc837f522
