fa90fe6440 refactor: Rename getWallets to getOpenWallets in WalletController (João Barbosa)
224eb9534a gui: Sort wallets in open wallet menu (João Barbosa)
Pull request description:
Ensure wallets are sorted by name in the open wallet menu. This also improves the change done in #15957, since it avoids a second call to `listWalletDir`.
ACKs for top commit:
laanwj:
code review ACK fa90fe6440
Tree-SHA512: 349ea195021e56760dea3551126d1b9dc4821faab44aaf2858229db4fddaf0ce0b5eb0a8fa5025f47c77134b003067a77e8c340f9655a99e1305d430ccecced8
7195fa792f test: Tool wallet test coverage for unexpected writes to wallet (Jon Atack)
3bf2b3a37b test: Split tool_wallet.py test into subtests (Jon Atack)
1eb13f09a9 test: Add log messages to test/functional/tool_wallet.py (Jon Atack)
Pull request description:
This pull request adds test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet as described in https://github.com/bitcoin/bitcoin/issues/15608 and serve as a benchmark for fixing the issue:
- Wallet tool `info` unexpectedly writes to the wallet file if the wallet file permissions are read/write.
- Wallet tool `info` raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only.
Goals:
1. Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix. Add debug-level logging for sanity checking and commented-out assertions to be uncommented when fixing the issue. Add the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue.
2. Add info log messages as there are currently none in the test file.
3. Split the tests out to separate functions as per review feedback.
Thanks to Marco Falke for pointing me in the right direction.
ACKs for top commit:
laanwj:
code review ACK 7195fa792f
Tree-SHA512: 16a41cce989c8f819cf5b02c6cf8ea84653ede2738fb402f6c36cf4dc075b424dff3e2c73a1cfa1ec9c75f614675baecc71e588845a2596db06ba0957db2df7b
e8fabd9253 build: prune dbus from depends (fanquake)
Pull request description:
Since #8210 (59d063d076), we've been passing `-dbus-runtime` when configuring Qt.
```
qtbase-opensource-src-5.9.7 $ ./configure -h | grep -i dbus
-no-dbus ............. Do not build the Qt D-Bus module
-dbus-linked ......... Build Qt D-Bus and link to libdbus-1 [auto]
-dbus-runtime ........ Build Qt D-Bus and dynamically load libdbus-1 [no]
```
This means we don't actually seem to be using the `D-Bus` we build in depends. This was pointed out by theuni at the time, [here](https://github.com/bitcoin/bitcoin/pull/7993#issuecomment-223114395) and [here](https://github.com/bitcoin/bitcoin/pull/8210#issuecomment-226930545), but was never followed up. dongcarl also bought it up as part of #16150.
I've tested building and running `bitcoin-qt` using depends on Debian. Needs further testing.
ACKs for top commit:
laanwj:
code review ACK e8fabd9253
Tree-SHA512: 164e6e52b6f97c04aef42bd185e2a157bc1a42103840f9404c5a795749f45a8c2c35f35873395a3a56398b3cd5955496b90d9c885d929b434c9bc871695abe20
This commit adds test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet as described in https://github.com/bitcoin/bitcoin/issues/15608:
- wallet tool `info` unexpectedly writes to the wallet file if the wallet file permissions are read/write.
- wallet tool `info` raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only.
1. Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix in the form of commented-out assertions to be uncommented when testing/fixing.
2. Provisionally extend the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue.
3. Add some logging for sanity checking.
------
Changes after rebase:
5. Make wallet_path an instance method instead of a function in tool_wallet.py as per Marco Falke review suggestion.
6. Assert wallet permissions instead of logging them in tool_wallet.py. This ran into an issue with Appveyor keeping permissions at 666 so allowed for 666 as a workaround.
7. Change the added logging from info to debug level.
8. More helpful assertions order in tool_wallet.py#assert_tool_output. This change makes #assert_tool_output raise "Error loading wallet.dat. Is wallet being used by another process?" rather than a less-helpful message when debugging the read-only wallet permissions issue.
84ad4d2b9d doc: Include static members in Doxygen (Carl Dong)
Pull request description:
This makes our Doxygen output more useful by generating them for static members.
ACKs for top commit:
practicalswift:
ACK 84ad4d2b9d
laanwj:
ACK 84ad4d2b9d
fanquake:
ACK 84ad4d2b9d
Tree-SHA512: f47fe6f36739ba8d7978169b28a29ad3d0796d7535052e447740077f4827c9bf5082d14c9cac2fbaf91f01bb2bffc25d9d7c3f702c0848c79a48a311ebd3344f
64b27c46e4 docs: add reduce-memory.md (fanquake)
Pull request description:
Following some discussion in https://github.com/bitcoin-core/docs/issues/50, this adds Wladimir's [reducing bitcoind memory usage gist](https://gist.github.com/laanwj/efe29c7661ce9b6620a7) to `/doc`.
The conclusion seemed to be that if the main repo already has [reduce-traffic.md](https://github.com/bitcoin/bitcoin/blob/master/doc/reduce-traffic.md), then we could also add `reduce-memory.md`.
ACKs for top commit:
practicalswift:
ACK 64b27c46e4
hebasto:
ACK 64b27c46e4, I have reviewed the changes and they look OK, I agree they can be merged. Also a link from `/doc/README.md` has been tested.
jonasschnelli:
ACK 64b27c46e4
Tree-SHA512: 0ab3035403e5145cfe33c29990a8d082df834ac6602b4ad6bfa821523d57e8451f0cde3017fbf3c2c4e0b34941b6374909d11d27f9598e211bbc14accd487be1
976b034b13 [build]: use #if HAVE_SYSTEM instead of defined(HAVE_SYSTEM) (Sjors Provoost)
Pull request description:
It seems that `AC_DEFINE([HAVE_SYSTEM], [HAVE_STD__SYSTEM || HAVE_WSYSTEM]` causes `HAVE_SYSTEM` to always be defined, so we need to use `#if HAVE_SYSTEM` instead of `#if defined(HAVE_SYSTEM)`.
Followup for #15457, can be tested with #12557.
ACKs for top commit:
dongcarl:
ACK 976b034b13.
promag:
ACK 976b034b13.
fanquake:
ACK 976b034b13
Tree-SHA512: b8cdd04c2ec399fd15638aef5d75ea0886ec1572d3cf4fcea27c193e1e6390344315908262cad8981a9b0a905ab9520619ce2ffe9a717f4ee6bfa8b028ebbdc6
91cc18f602 [docs] Add release notes for PR 15427 (John Newbery)
3b11420b3c [RPC] add new utxoupdatepsbt arguments to the CRPCCommand and CPRCConvertParam tables (John Newbery)
Pull request description:
The new `descriptors` argument was not added to the CRPCCommand and CPRCCvertParam tables, meaning that it couldn't be used with bitcoin-cli or named arguments.
Before this PR:
```
> bitcoin-cli utxoupdatepsbt 'cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' "[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
error code: -3
error message:
Expected type array, got string
> bitcoin-cli --named utxoupdatepsbt psbt='cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' descriptors="[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
error code: -8
error message:
Unknown named parameter descriptors
```
After this PR:
```
bitcoin-cli utxoupdatepsbt 'cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' "[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==
bitcoin-cli --named utxoupdatepsbt psbt='cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' descriptors="[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==
```
ACKs for top commit:
promag:
ACK 91cc18f.
fanquake:
re-ACK 91cc18f602
Tree-SHA512: 279b2339a5cac17e363002e4ab743e251d6757c904c89f1970575bdce18d4f63d5e13507e171bf2bdc1bf6dd457db345a4b11b15d4ff71b96c2fedc4ffe52b23
f874e14cd3 [build]: check std::system for -[alert|block|wallet]notify (Sjors Provoost)
cc3ad56ff2 [build] MSVC: set HAVE_SYSTEM for desktop apps (Sjors Provoost)
c1c91bb78d [build] detect std::system or ::wsystem (Sjors Provoost)
Pull request description:
Platforms such as iOs and Universal Windows Platform do not support launching a process through system().
ACKs for top commit:
laanwj:
code review ACK f874e14cd3
Tree-SHA512: 16bb4a8fa1896046ccb22a46c8985e1aa45f5b11ecf5539eb2299e9a58f1a5b085c0c12cb6939c7493d93abce7e84fadcbfc73374c887db63da6d00c08aa476d
08c721dab2 [MSVC] Copy build output to src/ automatically after build (nicolas.dorier)
Pull request description:
I got frustrated many time that I run functional tests on an outdated binary.
This make sure that files are properly copied (and debuggable) for functional tests after a build.
This PR add a `common.vcxproj` which can be used to extend the build steps.
I can't move it in `common.init`, because build step customization must always come at the end of a project file.
ACKs for top commit:
sipsorcery:
tACK 08c721dab2.
fanquake:
ACK 08c721dab2
Tree-SHA512: 7c033552a7bbbd3bb51a72d13bf4254e3b779c59ec93629c85f17745a1b17a8c1d6c87bb5e50eb5f4e5486e767929c0126e555b185608fd666aa39729b5068e1
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
The new `descriptors` argument needs to be added to the Command and
ConvertParams tables to by usable as a named argument and by
bitcoin-cli.
Also update the test to use named arguments to test this.
88fd556a96 Use placeholder instead of key expiration date (Hennadii Stepanov)
Pull request description:
Use a placeholder instead of the actual expiration date, so that the documentation doesn't require updating every time an expiry date changes.
ACKs for top commit:
fanquake:
ACK 88fd556a96
Tree-SHA512: 391707833cc0e701cf560ec82fd91368468c90a95f85e4ce2a211b20d12463c85775142f28a3536b57c5f6950b9e6e0785632f6f071fa2180bc8aab53008603b
3d60a03a7c bench: Move generated data to a dedicated translation unit (João Barbosa)
Pull request description:
With this change multiple benchmarks can use the same data without incurring in a bigger binary.
ACKs for top commit:
laanwj:
code review ACK 3d60a03a7c
Tree-SHA512: 8903bb09e4327c88e585a09bc7df1cbdfc18ebdc5d9c86bf3d6d9252a05eaf18b14ecd2bafdacd82f05a659e4b35ecd301c36011c97f7bf89302793165b00fdc
fab0c820fa rpc: Clarify that block count means height excl genesis (MarcoFalke)
Pull request description:
There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in https://github.com/bitcoin/bitcoin/pull/16292#issuecomment-506303256.
However, it really returns the height, which is `0` for the genesis block.
So clarify that and also remove the misleading "longest blockchain" comment.
Finally, fix the wallet test that incorrectly used this rpc.
ACKs for top commit:
instagibbs:
utACK fab0c820fa
promag:
ACK fab0c82, sorry for the misconception.
Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
8a6810d0d2 Add a 'logpath' field to getrpcinfo (darosior)
Pull request description:
as discussed in #15438
ACKs for top commit:
laanwj:
Tested ACK 8a6810d0d2
Tree-SHA512: 752c7d90f670677c8144efb338c5c97c2264f85f1e65e031fd5a44f04230b6eafbabd0f634db263eb42c25642ecc1c4b1b602d4735e3fab07ec00b566134ddab
0f459d868d fix an undefined behavior in uint::SetHex (Kaz Wesley)
Pull request description:
Decrementing psz beyond the beginning of the string is UB, even though
the out-of-bounds pointer is never dereferenced.
I don't think any clang sanitizer covers this, so I don't see any way a test could catch the original behavior.
ACKs for top commit:
promag:
utACK 0f459d8.
l2a5b1:
utACK 0f459d868d
Tree-SHA512: 388223254ea6e955f643d2ebdf74d15a3d494e9f0597d9f05987ebb708d7a1cc06ce64bd25d447d75b5f5561bdae9630dcf25adb7bd75f7a382298b95d127162
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
f53a70ce95 Improve documentation of memory_cleanse() (Tim Ruffing)
cac30a436c Clean up logic in memory_cleanse() for MSVC (Tim Ruffing)
Pull request description:
When working on https://github.com/bitcoin-core/secp256k1/issues/185, I noticed that the logic in memory_cleanse(), which is supposed to clear memory securely, is weird on MSVC. While it's correct, it's at least a code smell because the code clears the memory twice on MSVC. This weirdness was introduced by #11558.
This PR fixes the logic on MSVC and also improves the docs around this function. Best reviewed in individual commits, see the commit messages for more rationale. The second commit touches only comments.
ACKs for top commit:
practicalswift:
utACK f53a70ce95 :-)
laanwj:
code review ACK f53a70ce95
Tree-SHA512: 1c2fd98ae62b34b3e6e59d1178b293af969a9e06cbb7df02a699ce8802f145a336f72edb178c520e3ecec81f7e8083828f90a5ba6367d966a2c7d7c0dd6c0475
3b9bf0eb0e rpc: Allow shutdown while in generateblocks (Patrick Strateman)
Pull request description:
By checking the shutdown flag every loop we can use the entire 32 bit nonce space instead of breaking every 16 bits to check the flag.
This is possible now because the shutdown flag is an atomic where before it was controlled by a condition variable and lock.
ACKs for top commit:
kallewoof:
Re-ACK 3b9bf0e
Tree-SHA512: d0664201a55215130c2e9199a31fb81361daf4102a65cb3418984fd61cb98bfb9136d9ee8d23a85d57e50051f9bb0059bd71fe0488a17f63c38ea5caa6004504
01174596e6 signrawtransactionwithkey: report error when missing redeemScript/witnessScript param (Anthony Towns)
Pull request description:
Adding support for "witnessScript" as an alternative to "redeemScript" when using "signrawtransactionwithkey" meant that the `RPCTypeCheckObj()` call in `SignTransaction` can't error out just because either parameter is missing -- it's only a problem if both are missing, which isn't a state `RPCTypeCheckObj()` tests for. This results in the regression described in #16249. This patch adds some code to test for this case and give a similar error, namely:
error code: -8
error message:
Missing redeemScript/witnessScript
Fixes: #16249
ACKs for top commit:
meshcollider:
utACK 01174596e6
promag:
ACK 01174596e. Could also write test without `dict`/`del`:
Tree-SHA512: cf51346b7dea551b7f18f2a93c2a336a293b2535c62c03a5263cd2be8c58cf0cc302891da659c167e88ad1a68a756472c3c07e99f71627c61d32886fc5a3a353
26fe9b9909 Add support for descriptors to utxoupdatepsbt (Pieter Wuille)
3135c1a2d2 Abstract out UpdatePSBTOutput from FillPSBT (Pieter Wuille)
fb90ec3c33 Abstract out EvalDescriptorStringOrObject from scantxoutset (Pieter Wuille)
eaf4f88734 Abstract out IsSegWitOutput from utxoupdatepsbt (Pieter Wuille)
Pull request description:
This adds a descriptors argument to the `utxoupdatepsbt` RPC. This means:
* Input and output scripts and keys will be filled in when known.
* P2SH-witness inputs will be filled in from the UTXO set when a descriptor is provided that shows they're spending segwit outputs.
This also moves some (newly) shared code to separate functions: `UpdatePSBTOutput` (an analogue to `SignPSBTInput`), `IsSegWitOutput`, and `EvalDescriptorStringOrObject` (implementing the string or object notation parsing used in `scantxoutset`).
ACKs for top commit:
jnewbery:
utACK 26fe9b9909
laanwj:
utACK 26fe9b9909 (will hold merging until response to promag's comments)
promag:
ACK 26fe9b9, checked refactors and tests look comprehensive. Still missing a release note but can be added later.
Tree-SHA512: 1d833b7351b59d6c5ded6da399ff371a8a2a6ad04c0a8f90e6e46105dc737fa6f2740b1e5340280d59e01f42896c40b720c042f44417e38dfbee6477b894b245
fa2b083c3f [test] Add test to check mempool consistency in case of reorgs (MarcoFalke)
fabeb1f613 validation: Add missing mempool locks (MarcoFalke)
fa0c9dbf91 txpool: Make nTransactionsUpdated atomic (MarcoFalke)
Pull request description:
Take the mempool read lock during reorgs, so that we don't accidentally read an inconsistent mempool.
ACKs for top commit:
laanwj:
code review ACK fa2b083c3f
ryanofsky:
utACK fa2b083c3f [EDIT: was ~e284e422e75189794e24fe482819d8b1407857c3~, from bad copy and paste]. Changes since last review: rebase after #15976, adding vTxHashes lock annotation, adding new commit dropping mempool lock for nTransactionsUpdated and making it atomic to avoid deadlock between mempool lock and g_best_block_mutex
Tree-SHA512: cfe7777993589087753e000e3736d79d320dca412383fb77b56bef8946a04049722bf888c11b6f722adf677165185c7e58b4a269f7c5fa25e84dda375f6c8a7d
276972cb95 wallet_bumpfee.py: Make sure coin selection produces change (Gregory Sanders)
Pull request description:
I was hitting the case where change-less transactions were being made.
ACKs for top commit:
ryanofsky:
utACK 276972cb95
Tree-SHA512: e2b7a50363daddd3ee749cacfc9d3d685a6c0c7e3e48118bb60131d205bf83ea06cdd66b69dfa3bd4dbb3bbf2b5b673d7225171486ae72fc762e5dabe2c01ef5
d9753383b9 addrdb: Remove temporary files created in SerializeFileDB. Fixes non-determinism in unit tests. (practicalswift)
Pull request description:
Remove temporary files created in `SerializeFileDB` in case of errors.
_Edit: Previously this was hit non-deterministically from the tests: that is no longer the case but the cleanup issue remains :-)_
ACKs for top commit:
laanwj:
code-review ACK d9753383b9
Tree-SHA512: e72b74b8de411f433bd8bb354cacae07ab75a240db6232bc6a37802ccd8086bff5275ce3d196ddde033d8ab9e2794bb8f60eb83554af7ec2e9f91d6186cb4647
806b0052c3 [wallet] abort when attempting to fund a transaction above maxtxfee (Sjors Provoost)
Pull request description:
`FundTransaction` calls `GetMinimumFee` which, when the fee rate is absurdly high, quietly reduces the fee to `-maxtxfee`.
Becaue an absurdly high fee rate is usually the result of a fat finger, aborting seems safer behavior.
Before:
```
bitcoin-cli walletcreatefundedpsbt '[]' '[{"tb1q...": 0.01}]' 0 '{"feeRate": 10}' true
{
"psbt": "cHNidP8...gAA=",
"fee": 0.10000000,
"changepos": 1
}
```
After:
```
bitcoin-cli walletcreatefundedpsbt '[]' '[{"tb1q...": 0.01}]' 0 '{"feeRate": 10}' true
error code: -25
error message:
Fee exceeds maximum configured by -maxtxfee
```
QT still checks the max fee rate as expected:
<img width="566" alt="Schermafbeelding 2019-06-20 om 19 52 00" src="https://user-images.githubusercontent.com/10217/59888424-a2aa7100-9395-11e9-8ae6-8a3c1f7de585.png">
ACKs for top commit:
laanwj:
Code review ACK 806b0052c3
Tree-SHA512: bee95811711cdab100b614d2347921407af3b400aea613ca156953ed3f60b924ad29a1d335bd0e240c0b7c0fbb360226bab03294d226a5560cdf2a3f21e6d406
So far, the documentation of memory_cleanse() is a verbatim copy of
the commit message in BoringSSL, where this code was originally
written. However, our code evolved since then, and the commit message
is not particularly helpful in the code but is rather of historical
interested in BoringSSL only.
This commit improves improves the comments around memory_cleanse()
and gives a better rationale for the method that we use. This commit
touches only comments.
