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
0d101a340c test: Add test for maxtxfee option (MarcoFalke)
177550101b wallet: Remove unreachable code in CreateTransaction (MarcoFalke)
5c1b9714cb wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction (João Barbosa)
Pull request description:
Follow up to #16257, this PR makes `bumpfee` aware of `-maxtxfee`.
It also prevents dangling locked unspents when calling `fundrawtransaction` - because the previous check was after `LockCoin`.
ACKs for top commit:
MarcoFalke:
re-ACK 0d101a340c, only change is small test fixup
Tree-SHA512: 3464b24ae7cd4e72ed41438c6661828ba1304af020f05da62720b23668ae734e16cf47c6d97e150cc84ef631ee099b16fc786c858f3d089905845437338fd512
8e7f930828 Add GetNewChangeDestination for getting new change Destinations (Andrew Chow)
33d13edd2b Replace CReserveKey with ReserveDestinatoin (Andrew Chow)
172213be5b Add GetNewDestination to CWallet to fetch new destinations (Andrew Chow)
Pull request description:
The wallet should give out destinations instead of keys. It should be the one that handles the conversion from key to destination and the setting of the label, not the caller. In order to do this, two new member functions are introduced `GetNewDestination()` and `GetNewChangeDestination()`. Additionally, `CReserveKey` is changed to be `ReserveDestination` and represents destinations whose keys can be returned to the keypool.
ACKs for top commit:
instagibbs:
re-utACK 8e7f930828
sipa:
ACK 8e7f930828. Concept ACK as this gives a much cleaner abstraction to work with, and light code review ACK.
laanwj:
ACK 8e7f930828
Tree-SHA512: 5be7051409232b71e0ef2c1fd1a3e76964ed2f5b14d47d06edc2ad3b3687abd0be2803a1adc45c0433aa2c3bed172e14f8a7e9f4a23bff70f86260b5a0497500
Instead of having the same multiple lines of code everywhere
that new destinations are fetched, introduce GetNewDestination as
a member function of CWallet which does the key fetching, label
setting, script generation, and destination generation.
64fee48944 qt: Assert QMetaObject::invokeMethod result (João Barbosa)
f27bd96b5f gui: Fix missing qRegisterMetaType(WalletModel*) (João Barbosa)
Pull request description:
Invalid/wrong dynamic calls aren't verified by the compiler. This PR asserts those dynamic calls. Once we bump Qt to at least 5.10 these can be refactored to use the `invokeMethod` overload that allows connecting to lambdas or member pointers, which are compile checked.
For reference, one of the overloaded versions is https://doc.qt.io/qt-5/qmetaobject.html#invokeMethod-5.
ACKs for top commit:
laanwj:
ACK 64fee48944
Tree-SHA512: d332e5d7eb2c7be5d3fe90e2e4ff20a67800b9664f6637c122a23647a964f7915703d3f086e2de440f695cfe14de268ff581d0092b7736e911952a4f4d248e25
e1a55690e6 Delete error-prone CScript constructor (Gregory Sanders)
Pull request description:
The behavior of this constructor is not the expected behavior compared to the other constructors which directly interpret the vector as a CScript, rather than serialize it into a new CScript. It has only four uses in the entire codebase. Delete this constructor and replace its four uses with the more clear serialization construction.
ACKs for top commit:
Empact:
ACK e1a55690e6
sipa:
Concept and code review ACK e1a55690e6, but I'd like to make sure we have tests covering the FindAndDelete usage.
Tree-SHA512: b6721e343c867ca401a80ec87c25939d7f1fc798f3bf7e5feb0ea6f8280eecb6bd65afc8286912c76ff8119ccea50ad7726b1a4137cae70c9d4fed7d960e10d3
fa64b947bb util: No translation of `Bitcoin Core` in the copyright (MarcoFalke)
fab85208f6 qt: Run «make translate» in ./src/ (MarcoFalke)
fabe87d2c9 scripted-diff: Avoid passing PACKAGE_NAME for translation (MarcoFalke)
fa5e9f157e build: Stop translating PACKAGE_NAME (MarcoFalke)
Pull request description:
Generally the package name is not translated, but the package description is.
E.g. `GIMP` or `Firefox` are always called that way regardless of the system language. However, "`Firefox` webbrowser" or "`GIMP` image manipulation program" are translated.
ACKs for top commit:
hebasto:
ACK fa64b947bb, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
Tree-SHA512: 626f811531182d0ba0ef1044930d32726773349bcb49b10261288a86ee6b80a183db30a87d817d5b0d501fad058ac22d6272311716b4f5a154f17c6f391a5a1a
1ac454a384 Enable ShellCheck rules (Hennadii Stepanov)
Pull request description:
Enable some simple ShellCheck rules.
Note for reviewers: `bash` and `shellcheck` on macOS are different from ones on Ubuntu.
For local tests the latest `shellcheck` version 0.6.0 should be used (see #15166).
ACKs for top commit:
practicalswift:
utACK 1ac454a384
dongcarl:
utACK 1ac454a
fanquake:
ACK 1ac454a384
Tree-SHA512: 8d0a3a5c09fe1a0c22120178f5e6b80f81f746f8c3356b7701ff301c117acb2edea8fe08f08fb54ed73f94b1617515fb239fa28e7ab4121f74872e6494b6f20e
faa1e0fb17 qt: test: Create at most one testing setup (MarcoFalke)
Pull request description:
It is assumed that ideally only one BasicTestingSetup exists at any point in time for each process (due to use of globals).
This assumption is violated in the GUI tests, as a testing setup is created as the first step of the `main` function and then (sometimes) another one for the following test cases.
So, the gui tests create two testing setups:
* `BasicTestingSetup` in `main` (added in fa4a04a5a9)
* a testing setup for individual test cases
Avoid that by destructing the testing setup in main after creation and then move the explicit `ECC_Stop` to the only places where it is needed (before and after `apptests`).
ACKs for top commit:
laanwj:
code review ACK faa1e0fb17
Tree-SHA512: b8edceb7e2a8749e1de3ea80bc20b6fb7d4390bf366bb9817206ada3dc8669a91416f4803c22a0e6c636c514e0c858dcfe04523221f8851b10deaf472f107d82
FundTransaction calls GetMinimumFee which, when the fee rate is absurdly high, quietly reduced the fee to -maxtxfee. Becaue an absurdly high fee rate is usually the result of a fat finger, aborting seems safer behavior.
9a841696c1 tests: Reduce compilation time and unneccessary recompiles by removing unused includes in tests (practicalswift)
Pull request description:
Reduce compilation time and unneccessary recompiles by removing unused includes in tests.
A subset of #16273 ("refactor: Reduce total compilation time by 2% and avoid unnecessary recompiles by removing unused includes") as requested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/16273#issuecomment-505022643.
ACKs for top commit:
Sjors:
ACK 9a84169 on macOS 10.14.5 (I rebased on #16289)
Tree-SHA512: bcb6ecffef689a9839bee1a5cb93abe83db1f30819a54226c5630fee456b5a5d187507d06861454adfda939c3556a975113f97662e415cb47fa0327ea4fd09fb
fabc57e07d test: Log to debug.log in all tests (MarcoFalke)
fa4a04a5a9 test: use common setup in gui tests (MarcoFalke)
fad3d2a624 test: Create data dir in BasicTestingSetup (MarcoFalke)
Pull request description:
This makes it easier to debug a frozen test or a test that failed. To debug a failed test, remove the line `fs::remove_all(m_path_root);`.
The pull is done in three commits:
* Create a datadir for every unit test once (and only once). This requires the `SetDataDir` function to go away.
* Use the common setup in the gui unit tests. Some of those tests are testing the init sequence, so we'd have to undo some of what the testing setup did.
* Log to the debug.log in all tests
ACKs for top commit:
laanwj:
ACK fabc57e07d
Tree-SHA512: 73444210b88172669e2cd22c2703a1e30e105185d2d5f03decbdedcfd09c64ed208d3716c59c8bebb0e44214cee5c8095e3e995d049e1572ee98f1017e413665
099e4b9ad3 Set AA_EnableHighDpiScaling attribute early (Hennadii Stepanov)
Pull request description:
Running `bitcoin-qt` compiled against Qt 5.12.4 causes a warning:
```
hebasto@bionic-qt:~/bitcoin$ src/qt/bitcoin-qt
Attribute Qt::AA_EnableHighDpiScaling must be set before QCoreApplication is created.
```
This PR fixes this issue.
From Qt docs:
- [Qt::AA_EnableHighDpiScaling](https://doc.qt.io/qt-5/qt.html#ApplicationAttribute-enum):
> Enables high-DPI scaling in Qt on supported platforms (see also High DPI Displays). _Supported platforms are X11, Windows and Android._ Enabling makes Qt scale the main (device independent) coordinate system according to display scale factors provided by the operating system. This corresponds to setting the `QT_AUTO_SCREEN_SCALE_FACTOR` environment variable to 1. This attribute must be set before `QGuiApplication` is constructed. This value was added in Qt 5.6.
- [QCoreApplication::setAttribute()](https://doc.qt.io/qt-5/qcoreapplication.html#setAttribute)
ACKs for commit 099e4b:
MarcoFalke:
ACK 099e4b9ad3
jonasschnelli:
utACK 099e4b9ad3
fanquake:
ACK 099e4b9ad3. Did some testing on `Bionic` and `Windows 10` (using VirtualBox). I couldn't see any obvious visual difference, but given Marco's screens above, this change is obviously better. I also checked that there wasn't any sort of regression on macOS.
Tree-SHA512: 1965a427ee14ffb3871bac317685032406cf02d1fa2b2dc11c8b643bfe4ba09195674d149d1e41752f14c0d000446b35e142f3ce60d987ba97082fd7ee39a094
a2aabfb749 Use qInfo() if no error occurs (Hennadii Stepanov)
Pull request description:
[Warning and Debugging Messages](https://doc.qt.io/qt-5/debug.html#warning-and-debugging-messages):
> - `qInfo()` is used for informational messages.
> - `qWarning()` is used to report warnings and recoverable errors in your
application.
>
> If the `QT_FATAL_WARNINGS` environment variable is set, `qWarning()` exits after printing the warning message. This makes it easy to obtain a backtrace in the debugger.
[`qWarning()`](https://doc.qt.io/qt-5/qtglobal.html#qWarning):
> Calls the message handler with the warning message message... This function does nothing if `QT_NO_WARNING_OUTPUT` was defined during compilation; it exits if at the nth warning corresponding to the counter in environment variable `QT_FATAL_WARNINGS`.
This PR allows more productive debugging using the environment variable `QT_FATAL_WARNINGS`.
Examples:
- https://github.com/bitcoin/bitcoin/pull/16118#issuecomment-503184695
- https://github.com/bitcoin/bitcoin/pull/16254#issuecomment-504223404
The behavior, when option `-debug=qt` is set/unset, remains unchanged.
ACKs for commit a2aabf:
promag:
ACK a2aabfb, I also have this change locally.
Empact:
ACK a2aabfb749
laanwj:
ACK a2aabfb749
fanquake:
ACK a2aabfb749.
Tree-SHA512: b4df300c9c00a1705b0d3a10227e3deaac19a98b0a898bb60d5a88872cf450fb131eba150d9dd6c29e021566ee04b3b86b7d486bbe28bd894743c128d2309155
5224be5a33 gui: Fix open wallet menu initialization order (João Barbosa)
Pull request description:
Fixes#16230, the menu must be created before connecting to aboutToShow signal.
ACKs for commit 5224be:
hebasto:
ACK 5224be5a33, I have tested the code on Bionic with Qt 5.12.4.
ryanofsky:
utACK 5224be5a33. Looks good, fix is simple and makes perfect sense after seeing explanation in https://github.com/bitcoin/bitcoin/pull/16118#issuecomment-503166407. Without this change (and since #16118), the menu pointer passed to `connect(m_open_wallet_action->menu(), ...)` is null and connecting has no effect. With this change, the menu is constructed earlier so the connect call can work.
fanquake:
ACK 5224be5a33 Testing included in a comment above. The segfaulting with QT_FATAL_WARNINGS is unrelated to this change.
Tree-SHA512: 97b42493b37b96683058bccf39a0ee93589293d4ba8f0c60aef7f4fb9dd084cc6d5608cd5ef531cadf5e03b1f01627ef96bc2d79f784fb38cb87aa6643183d41
fa8f195195 Replace remaining fprintf with tfm::format manually (MarcoFalke)
fac03ec43a scripted-diff: Replace fprintf with tfm::format (MarcoFalke)
fa72a64b90 tinyformat: Add doc to Bitcoin Core specific strprintf (MarcoFalke)
Pull request description:
This should be a refactor except in the cases where we use the wrong format specifier [1], in which case this patch is a bug fix.
[1] : e.g. depends: Add libevent compatibility patch for windows #8730
ACKs for commit fa8f19:
promag:
ACK fa8f195195. Ideally this should be rebased before merge.
practicalswift:
utACK fa8f195195
Empact:
ACK fa8f195195
laanwj:
code review and lightly tested ACK fa8f195195
jonatack:
ACK fa8f195195 from light code review, building, and running linter/unit tests/extended functional tests.
Tree-SHA512: 65f648b0bc383e3266a5bdb4ad8c8a1908a719635d49e1cd321b91254be24dbc7e22290370178e29b98ddcb3fec0889de9cbae273c7140abc9793d849534a743
75485ef09 gui: Enable open wallet menu on setWalletController (João Barbosa)
Pull request description:
`BitcoinApplication::initializeResult` and `BitcoinGUI::setWalletController` are only called after the startup rescan is completed. While the rescan is in progress the window menus are already available.
This PR makes the Open Wallet menu disabled until `BitcoinGUI::setWalletController` is called.
Fixes#16087
ACKs for commit 75485e:
jonasschnelli:
utACK 75485ef096
ryanofsky:
utACK 75485ef096. It's a simple, sensible fix.
Tree-SHA512: 9395ceed54bbceb6cbf1cd443f783d07a6ebb8fc5515b63c6e1b8b19b216b08d1cba7eaf872814d7c426ab7192f3b416ba0d57fc84f3bcbfebf01ce153794201
67f4e9c522 Include core_io.h from core_read.cpp (practicalswift)
eca9767673 Make reasoning about dependencies easier by not including unused dependencies (practicalswift)
Pull request description:
Make reasoning about dependencies easier by not including unused dependencies.
Please note that the removed headers are _not_ "transitively included" by other still included headers. Thus the removals are real.
As an added bonus this change means less work for the preprocessor/compiler. At least 51 393 lines of code no longer needs to be processed:
```
$ git diff -u HEAD~1 | grep -E '^\-#include ' | cut -f2 -d"<" | cut -f1 -d">" | \
sed 's%^%src/%g' | xargs cat | wc -l
51393
```
Note that 51 393 is the lower bound: the real number is likely much higher when taking into account transitively included headers :-)
ACKs for commit 67f4e9:
Tree-SHA512: 0c8868aac59813f099ce53d5307eed7962dd6f2ff3546768ef9e5c4508b87f8210f1a22c7e826c3c06bebbf28bdbfcf1628ed354c2d0fdb9a31a42cefb8fdf13
6e6494b3fb qt: fix opening bitcoin.conf via Preferences on macOS; see #15409 (shannon1916)
Pull request description:
Fix#15409. The QT wallet fail to open the configuration file on Mac, when these is no default application for `*.conf` files.
Here is a feasible way to solve this bug. When `QDesktopServices::openUrl` fails to open `file:///path/bitcoin.conf` with its default application, use `QProcess::startDetached` to run `open -t /path/bitcoin.conf` command instead, so as to open the configuration file with system's default text editor.
ACKs for commit 6e6494:
hebasto:
re-ACK 6e6494b3fb
fanquake:
tACK 6e6494b3fb on macOS 10.14.x
Tree-SHA512: 60e898f4cb77cfd7b8adbc8d33fbebf46bac2a801bdcf40cae15e24b78ad56b1f32358b1879b670623d9f8651dea93961d34269358cea18f4e15b089a8ffcfbf
2d8ad2f997 gui: Enable console line edit on setClientModel (João Barbosa)
Pull request description:
Make console line edit disable by default, and only enable once `RPCConsole::setClientModel` is called.
Fixes#16119.
ACKs for commit 2d8ad2:
fanquake:
tACK 2d8ad2f997 on macOS.
Tree-SHA512: 1418ce3c120c08e5ec3e7a7a063572a24402ce0ec541bd4adc21f61d60c4e86b711e82e940ebf5f0445ab861f89c146c2a2e7990fb52bed2c65fc199a1981f71
9f85e9cb3d scripted-diff: Rename LockAnnotation to LockAssertion (practicalswift)
de9b5dbca3 Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) (practicalswift)
3a809446b3 Move LockAnnotation to make it reflect the truth (practicalswift)
cc2588579c Move LockAnnotation from threadsafety.h (imported code) to sync.h (our code) (practicalswift)
Pull request description:
`LockAnnotation lock(mutex);` is a guarantee to the compiler thread-analysis that `mutex` is locked (when it couldn't be determined otherwise).
Before this PR it was possible to make the mistake of adding a `LockAnnotation` where the correct mutex is _not_ held. This in turn makes the thread-analysis reasoning being based on incorrect premises.
This PR adds an assertion in the `LockAnnotation` ctor which checks that the guarantees given by us at compile-time are held also in practice (`ifdef DEBUG_LOCKORDER`).
Issues like the one described in #16028 will be discovered immediately with this PR merged.
Changes in this PR:
* Move `LockAnnotation` from `threadsafety.h` (imported code) to `sync.h` (our code)
* Move `LockAnnotation` in `wallet_tests` to make it reflect the truth
* Make sure the compile-time locking promises given via `LockAnnotation`:s hold also in practice at runtime (`ifdef DEBUG_LOCKORDER`)
* Rename `LockAnnotation` to `LockAssertion`
ACKs for commit 9f85e9:
ryanofsky:
utACK 9f85e9cb3d. No changes at all since last review except clean rebase after base PR #16033 was merged
Tree-SHA512: fb80e78fe362adfd6ea8405bcb142c09b99f834fe8be4397282b223ca2c3a2bb9719a074a47a043b44757f840b239a6fcd2f98d14771f8729204834ecf608c3a
0b09a57ae Give WalletModel::UnlockContext move semantics (Pieter Wuille)
Pull request description:
WalletModel::UnlockContext seems to implement "move upon copy" semantics; with C++11 this can be done more safely using move semantics (making attempts to actually copy fail instead).
Not a big deal if this isn't worth review time.
ACKs for commit 0b09a5:
Empact:
utACK 0b09a57aec
jonasschnelli:
utACK 0b09a57aec
jb55:
utACK 0b09a57aec
Tree-SHA512: f827856586afd03666c2d9f50320776afb3dd511ac1bcd293b330f015acd1588551b163dccc97b1351301e3295f4c74d90e5754bcee89faeadf6437d7db165c8
78e407ad0c GetKeyBirthTimes should return key ids, not destinations (Gregory Sanders)
70946e7fee Replace CScriptID and CKeyID in CTxDestination with dedicated types (Gregory Sanders)
Pull request description:
The current usage seems to be an overloading of meanings. `CScriptID` is used in the wallet as a lookup key, as well as a destination, and `CKeyID` likewise. Instead, have all destinations be dedicated types.
New types:
`CScriptID`->`ScriptHash`
`CKeyID`->`PKHash`
ACKs for commit 78e407:
ryanofsky:
utACK 78e407ad0c. Only changes are removing extra CScriptID()s and fixing the test case.
Sjors:
utACK 78e407a
meshcollider:
utACK 78e407ad0c
Tree-SHA512: 437f59fc3afb83a40540da3351507aef5aed44e3a7f15b01ddad6226854edeee762ff0b0ef336fe3654c4cd99a205cef175211de8b639abe1130c8a6313337b9
486c1eea86 refactoring: remove unused chainActive (James O'Beirne)
631940aab2 scripted-diff: replace chainActive -> ::ChainActive() (James O'Beirne)
a3a609079c refactoring: introduce unused ChainActive() (James O'Beirne)
1b6e6fcfd2 rename: CChainState.chainActive -> m_chain (James O'Beirne)
Pull request description:
This is part of the assumeutxo project:
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
This change refactors the `chainActive` reference into a `::ChainActive()` call. It also distinguishes `CChainState`'s `CChain` data member as `m_chain` instead of the current `chainActive`, which makes it easily confused with the global data.
The active chain must be obtained via function because its reference will be swapped at some point during runtime after loading a UTXO snapshot.
This change, though lengthy, should be pretty easy to review since most of it is contained within a scripted-diff. Once merged, the parent PR should be easier to review.
ACKs for commit 486c1e:
Sjors:
utACK 486c1ee
promag:
utACK 486c1ee.
practicalswift:
utACK 486c1eea86
Tree-SHA512: 06ed8f9e77f2d25fc9bea0ba86436d80dbbce90a1e8be23e37ec4eeb26060483e60b4a5c4fba679cb1867f61e3921c24abeb9cabdfb4d0a9b1c4ddd77b17456a
fc929842c2 GUI: Move QRImageWidget to its own file-pair (Luke Dashjr)
77851ab682 GUI: Refactor actual QR code rendering into new QRImageWidget::setQR (Luke Dashjr)
Pull request description:
For at least QR-code based pairing of mobile wallets with nodes, it will be desirable to render QR codes even without wallet support.
Therefore, this prepares by moving the QRImageWidget out of a wallet-specific file into its own `qrencoder` file-pair.
ACKs for commit fc9298:
laanwj:
utACK fc929842c2
jonasschnelli:
utACK fc929842c2
Tree-SHA512: 95529a38c0573a4b3f1253fb5f11ca07a5b3a9840ec24acc7d87270212f3c9f7c5b186d9274d297517a3b80494f38a57574fb9730b1574db01688539b987bd91
Though at the moment ChainActive() simply references `g_chainstate.m_chain`,
doing this change now clears the way for multiple chainstate usage and allows
us to script the diff.
-BEGIN VERIFY SCRIPT-
git grep -l "chainActive" | grep -E '(h|cpp)$' | xargs sed -i '/chainActive =/b; /extern CChain& chainActive/b; s/\(::\)\{0,1\}chainActive/::ChainActive()/g'
-END VERIFY SCRIPT-
This work is prerequisite to attaching thread names to log lines and deadlock
debug utilities. This code allows setting of an "internal" threadname per
thread on platforms where thread_local is available.
This commit also moves RenameThread() out of a more general module and adds a
numeric suffix to disambiguate between threads with the same name. It
explicitly names a few main threads using the new util::ThreadRename().
5c759c73b2 [wallet] Move maxTxFee to wallet (John Newbery)
Pull request description:
Closes#15355
Moves the `-maxtxfee` from the node to the wallet. See discussion in issue for details.
This is a cleanup. There is no change in behaviour.
Completes #15620
ACKs for commit 5c759c:
MarcoFalke:
utACK 5c759c73b2
ryanofsky:
utACK 5c759c73b2. Changes since last review: updated commit message and an error message and method name.
meshcollider:
utACK 5c759c73b2
Tree-SHA512: 2f9b2729da3940a5cda994d3f3bc11ee1a52fcc1c5e9842ea0ea63e4eb0300e8416853046776311298bc449ba07554aa46f0f245ce28598a5b0bd7347c12e752
This pull request attempts to resolve the `qt/guiutil` <-> `qt/optionsmodel`
circular dependency.
The circular dependency is resolved by moving the `Intro::getDefaultDataDirectory`
member function to `GUIUtil::getDefaultDataDirectory`.
418d3230f8 Resolve the checkpoints <-> validation CD. (251)
Pull request description:
This pull request attempts to resolve the `checkpoints -> validation -> checkpoints` circular dependency.
The circular dependency is resolved by moving the `CheckPoints::GetLastCheckpoint(const CCheckpointData& data)` function to `validation.cpp` where it used exclusively by the private function `ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& params, const CBlockIndex* pindexPrev, int64_t nAdjustedTime)`.
ACKs for commit 418d32:
promag:
utACK 418d323, only `GetLastCheckpoint` usage is in `validation.cpp` and so makes sense to move it there.
practicalswift:
utACK 418d3230f8
MarcoFalke:
utACK 418d3230f8
sipa:
utACK 418d3230f8
Tree-SHA512: 03c3556bc192e65f5e3fa76fd545d4ee7d63d3fb06b132f7a1fa6131aa21ddd2e5b2d19e2222dfe524f422daaca30efde219bed188db8c74ff4b088876b5bc16
8a33f4d63f GUI: Options: Remove the upper-bound limit from pruning size setting (Luke Dashjr)
4ddeb2f860 GUI: Options: Set the range of pruning size before loading its value (Luke Dashjr)
Pull request description:
This fixes two bugs:
1. The prune setting range was set *after* loading the current value. If users had a prune of (eg) 200, it would get limited to 99 before the range was raised. This is fixed by setting the range first.
2. The prune setting was limited to <= the chainparams' "assumed blockchain size". There's no reason for this limit (the UX is the same either way), and there are use cases it breaks (eg, setting a prune size such that it begins pruning at some future point). Therefore, I raised it to the max value.
This is a daggy fix, so should cleanly merge to both master and 0.18 branches.
ACKs for commit 8a33f4:
MarcoFalke:
utACK 8a33f4d63f
laanwj:
utACK 8a33f4d63f
promag:
utACK 8a33f4d.
Tree-SHA512: 480570fa243ab5cc76af76fded18cb8cb2d3194b9f050fec5e03ca551edeeda72ee8b06312e200a9e49404ec1cdffa62f7150cf9982ec1b282f17d90879ce438
This commit moves the maxtxfee setting to the wallet. There is only
one minor behavior change:
- an error message in feebumper now refers to -maxtxfee instead of
maxTxFee.
faf400077d scripted-diff: Bump copyright headers in test, bench (MarcoFalke)
fa821904bf scripted-diff: Rename test_bitcoin to test/setup_common (MarcoFalke)
fa8685d49e test: Use test_bitcoin setup in bench, Add test utils (MarcoFalke)
666696b673 test: Have segwit always active in (Basic)TestingSetup (MarcoFalke)
Pull request description:
Now that the fuzz tests can use the BasicTestingSetup [1], do the same for bench.
Also move some duplicate code to a common "test/util" module.
[1]: fuzz: Link BasicTestingSetup (shared with unit tests) #15504
ACKs for commit faf400:
jonatack:
ACK faf400077d
Tree-SHA512: 8ac5692e72cf50e460958f291643ae6b8bb04d5c1331ed50dce9eb4e9457e5a925144c532c42b360a26707e11eeece74aab27db8c76ab9a429b9dd7167e7cdc4
Adds the following util units and adds them to libbitcoin_util:
- `util/url.cpp` takes `urlDecode` from `httpserver.cpp`
- `util/error.cpp` takes `TransactionErrorString` from
`node/transaction.cpp` and `AmountHighWarn` and `AmountErrMsg` from
`ui_interface.cpp`
- `util/fees.cpp` takes `StringForFeeReason` and `FeeModeFromString` from `policy/fees.cpp`
- `util/rbf.cpp` takes `SignalsOptInRBF` from `policy/rbf.cpp`
- 'util/validation.cpp` takes `FormatStateMessage` and `strMessageMagic` from 'validation.cpp`
This commit resolves the checkpoints -> validation -> checkpoints
cirular dependency by moving
`CheckPoints::GetLastCheckpoint(const CCheckpointData& data)` from
`checkpoints.cpp` to `validation.cpp`.
signrawtransaction was deprecated in 0.17 and removed in 0.18. A warning
was left in place to tell users to migrate to using
signrawtransactionswithwallet or signrawtransactionwithkey. Remove the
warning now that it's been two releases since the method was removed.
d358466de Remove remaining wallet accesses to node globals (Russell Yanofsky)
b1b2b2389 Remove use of CCoinsViewMemPool::GetCoin in wallet code (Russell Yanofsky)
4e4d9e9f8 Remove use of CRPCTable::appendCommand in wallet code (Russell Yanofsky)
91868e628 Remove use CValidationInterface in wallet code (Russell Yanofsky)
Pull request description:
This PR is the last in a chain of PRs (#14437, #14711, and #15288) that make the wallet code access node state through an abstract [`Chain`](https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep/src/interfaces/chain.h) class in [`src/interfaces/`](https://github.com/ryanofsky/bitcoin/tree/pr/wipc-sep/src/interfaces) instead of using global variables like `cs_main`, `chainActive`, and `g_connman`. After this PR, wallet code no longer accesses global variables declared outside the wallet directory, and no longer calls functions accessing those globals (as verified by the `hide-globals` script in #10244).
This PR and the previous PRs have been refactoring changes that do not affect behavior. Previous PRs have consisted of lots of mechanical changes like:
```diff
- wtx.nTimeReceived = GetAdjustedTime();
+ wtx.nTimeReceived = m_chain->getAdjustedTime();
```
This PR is smaller, but less mechanical. It replaces last few bits of wallet code that access node state directly (through `CValidationInterface`, `CRPCTable`, and `CCoinsViewMemPool` interfaces) with code that uses the `Chain` interface.
These changes allow followup PR #10102 (multiprocess gui & wallet PR) to work without any significant updates to wallet code. Additionally they:
* Provide a single place to describe the interface between wallet and node code.
* Can make better wallet testing possible, because the `Chain` object consists of virtual methods that can be overloaded for mocking. (This could be used to test edge cases in the rescan code, for example).
Tree-SHA512: e6291d8a3c50bdff18a9c8ad11e729beb30b5b7040d7aaf31ba678800b4a97b2dd2be76340b1e5c01fe2827d67d37ed1bb4c8380cf8ed653aadfea003e9b22e7
28c86de3b gui: Drop unused return values in WalletFrame (João Barbosa)
Pull request description:
This is a small cleanup since the return value of `WalletFrame` methods are not used. This is in line with the usual async slot declaration.
Tree-SHA512: ff0ca098804118bba200a58cd796ff90e853a6430e58125bd178b7bfa9b2b763c13d17b81e8f3ebd94395cac249d80379ba1529680c47682ba6a2ed81492ba33
fd46c4c00 Bump minimum Qt version to 5.5.1 (Sjors Provoost)
Pull request description:
Fixes#13478
Compiled and lightly tested on 10.14.3 against QT 5.12.0.
Tree-SHA512: 6890331969bbf4c66dc0993b8817b1f0831d008f5863554e9c09a38f4700260b84044ff961664c377decc9fb8300e3543c267f935ec64fbc97b20f8fb396247a
84f53154e Travis: Add test without BIP70 (but still full wallet + tests) (Luke Dashjr)
113f0004b GUI: If BIP70 is disabled, give a proper error when trying to open a payment request file (Luke Dashjr)
9975282fa GUI: If BIP70 is disabled, attempt to fall back to BIP21 parsing (Luke Dashjr)
Pull request description:
Tree-SHA512: 66a684ce4336d0eac8b0107b405ff3a2cf312258a967f3e1b14734cd39db11e2db3e9b03492755583170d94d54754ef536b0776e5f19a0cc2caca8379eeb4495
1951ea434 gui: Show indeterminate progress dialog while opening walllet (João Barbosa)
8847cdaaa gui: Add OpenWalletActivity (João Barbosa)
4c8982a88 interfaces: Avoid interface instance if wallet is null (João Barbosa)
be82dea23 gui: Add thread to run background activity in WalletController (João Barbosa)
6c49a55b4 gui: Add Open Wallet menu (João Barbosa)
32a8c6abf gui: Add openWallet and getWalletsAvailableToOpen to WalletController (João Barbosa)
ab288b4e5 interfaces: Add loadWallet to Node (João Barbosa)
17abc0fd5 wallet: Factor out LoadWallet (João Barbosa)
Pull request description:
The *Open Wallet* menu has all the available wallets currently not loaded. The list of the available wallets comes from `listWalletDir`.
In the future the menu can be replaced by a custom dialog.
<img width="674" alt="screenshot 2019-01-12 at 12 17 02" src="https://user-images.githubusercontent.com/3534524/51073166-ac041480-1664-11e9-8302-be81702bc146.png">
Tree-SHA512: ebfd75eee0c8264863748899843afab67dadb7dff21313c11e3cb5b6108d954978dd1f1ae786bc07580c5a771ea4ab38d18c1643c9b9b3683ed53f0f6c582e38
7687f7873 [wallet] Support creating a blank wallet (Andrew Chow)
Pull request description:
Alternative (kind of) to #14938
This PR adds a `blank` parameter to the `createwallet` RPC to create a wallet that has no private keys initially. `sethdseed` can then be used to make a clean wallet with a custom seed. `encryptwallet` can also be used to make a wallet that is born encrypted.
Instead of changing the version number as done in #14938, a wallet flag is used to indicate that the wallet should be blank. This flag is set at creation, and then unset when the wallet is no longer blank. A wallet becomes non-blank when a HD seed is set or anything is imported. The main change to create a blank wallet is primarily taken from #14938.
Also with this, the term "blank wallet" is used instead of "empty wallet" to avoid confusion with wallets that have balance which would also be referred to as "empty".
This is built on top of #15225 in order to fix GUI issues.
Tree-SHA512: 824d685e11ac2259a26b5ece99c67a7bda94a570cd921472c464243ee356b7734595ad35cc439b34357135df041ed9cba951e6edac194935c3a55a1dc4fcbdea
A blank wallet is a wallet that has no keys, script or watch only things.
A new wallet flag indicating that it is blank will be set when the wallet
is blank. Once it is no longer blank (a seed has been generated, keys or
scripts imported, etc), the flag will be unset.
e8db6b8044 Qt: Fix update headers-count (Jonas Schnelli)
7bb45e4b7a Qt: update header count regardless of update delay (Jonas Schnelli)
Pull request description:
Update the block and header tip is constraint to have a minimal distance of 250ms between updates... which can lead to miss the last header update.
The modal overlay then assumes we are still in header sync and the view get stuck in "syncing headers,..." (while it's actually syncing blocks).
This removes the 250ms minimal delta for header updates as well as it fixes the correct display of how header updates should update the labels.
Tree-SHA512: 57608dac822b135cd604fc6ba1c80f25c0202a6e20bb140362026615d4bf243ef4fcc254a11bad36419c554a222a2f4947438d4ce44aa14041d1874751643d68
364cff1cab Fix issue #9683 "gui, wallet: random abort (segmentation fault) running master/HEAD". (Chris Moore)
Pull request description:
Patch taken from @ryanofsky's comment https://github.com/bitcoin/bitcoin/issues/9683#issuecomment-448035913.
[MarcoFalke wrote](https://github.com/bitcoin/bitcoin/issues/9683#issuecomment-454066004):
> Mind to submit this patch as a pull request?
So that's what I'm doing.
I was regularly seeing crashes on startup before applying this patch and haven't seen a single crash on startup since applying it almost a month ago.
Tree-SHA512: 3bbb2291cdf03ab7e7b5b796df68d76272491e35d473a89f4550065554c092f867659a7b8d7a1a91461ae4dc9a3b13b72541eafdbd732536463e9f3cf82300c8
0164b0f5cf build: Remove WINVER pre define in Makefile.leveldb.inlcude (Chun Kuan Lee)
d0522ec94e Drop defunct Windows compat fixes (Ben Woosley)
d8a2992067 windows: Call SetProcessDEPPolicy directly (Chun Kuan Lee)
1bd9ffdd44 windows: Set _WIN32_WINNT to 0x0601 (Windows 7) (Chun Kuan Lee)
Pull request description:
The current minimum support Windows version is Vista. So set it to 0x0600
5a88def8ad/mingw-w64-headers/include/sdkddkver.h (L19)
Tree-SHA512: 38e2afc79426ae547131c8ad3db2e0a7f54a95512f341cfa0c06e4b2fe79521ae67d2795ef96b0192e683e4f1ba6183c010d7b4b8d6b3e68b9bf48c374c59e7d
aebafd0edf Rename Chain getLocator -> getTipLocator (Russell Yanofsky)
2c1fbaa771 Drop redundant get_value_or (Russell Yanofsky)
84adb206fc Fix ScanForWalletTransactions start_block comment (Russell Yanofsky)
2efa66b464 Document rescanblockchain returned stop_height being null (Russell Yanofsky)
db2d093233 Add suggested rescanblockchain comments (Russell Yanofsky)
a8d645c934 Update ScanForWalletTransactions result comment (Russell Yanofsky)
95a812b599 Rename ScanResult stop_block field (Russell Yanofsky)
Pull request description:
This implements suggested changes from #14711 review comments that didn't make make it in before merging.
There are no changes in behavior in this PR, just documentation updates, simplifications, and variable renames.
Tree-SHA512: 39f1a5718195732b70b5e427c3b3e4295ea5af6328a5991763a422051212dfb95383186db0c0504ce2c2782fb61998dfd2fe9851645b7cb4e75d849049483cc8
- Update transifex slug
- Mention update of MSVC build in `doc/translation_process.md`
- Do a `make translate` to update English translations
- Pull current translations from transifex
This change forwards the shutdown request on the GUI (close the
application for instace) to the node as soon as possible. This way the
GUI doesn't have to wait for long operations to complete (rescan the
wallet for instance), instead those operations detect the shutdown
request and abort/interrupt.
Move only change that makes unsubscribeFromCoreSignals public. It must be
called if the event loop is not running otherwise core signals handlers
can deadlock.
