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
f466c4ce84 Add missing ECC_Stop(); in GUI rpcnestedtests.cpp (Jonas Schnelli)
Pull request description:
Fixes#16288
Was probably missing in #7783
ACKs for top commit:
Sjors:
ACK f466c4c. Tested by comparing `make check` on master and this PR with macOS 10.14.5. I also tried with and without `--enable-debug` / `--without-gui`.
fanquake:
ACK f466c4ce84. Tested running `make check` on macOS.
Tree-SHA512: 648e10c2e35bd01fb92e63709169a6c185ac4b62c69af0109d2cd2d7db47e56ae804c788f9a1a1845746f818764799732f9e58e9dbfca3bffeea8f14683c8c7f
9824a0d6e9 Remove extra CBlockIndex declaration (RJ Rybarczyk)
Pull request description:
Remove duplicate `class CBlockIndex;` declaration.
ACKs for top commit:
promag:
ACK 9824a0d. Is this a random finding or you have searched for more similar cases?
practicalswift:
utACK 9824a0d6e9
fanquake:
ACK 9824a0d6e9
Tree-SHA512: aaf88450f53cb8859778102fe971b1121808819c04e64802e5a5cf47bf1403b42531361c52b097b41b905f9fa1bb7acc82b446cfa659c6ac41d00fab29e114e4
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
30ea931dee depends: xtrans: Configure flags cleanup. (Carl Dong)
Pull request description:
```
xtrans does not understand the --with-pic and --disable-shared flags we
pass it because it is not a library. Instead, we should pass it flags
that disable features/packages we're not using so they don't get a
chance to sneak in.
```
Here's a comparison of stdout and stderr of `make -j(nproc) V=1 xtrans_built` before and after this PR: https://gist.github.com/dongcarl/4ebf6fe9985ebc1508190f75932e4237
ACKs for top commit:
fanquake:
ACK 30ea931dee
Tree-SHA512: 49e060dffbae8c148c1164b493d03e7db4f260e7746d94c783377185c665e16fb6638827473709ed543eb266cc293d2313f61ade05ee8af4edb0bfbda0d1f785
dddd9270f8 net: Document what happens to getdata of unknonw type (MarcoFalke)
Pull request description:
Any getdata of unknown type will never be processed and blocks all future messages from a peer. This isn't obviously clear from reading the code, so document it.
Top commit has no ACKs.
Tree-SHA512: 4f8e43bbe6534242facfcfffae28b7a6aa2d228841fa2146a87d494e69f614b0da23cf7a5f3d4367358a7c1981fe2ec196a21c437ae1653f1c7e0351be22598a
0e01e4522e devtools: Fetch and display ACKs at sign-off time in github-merge (Wladimir J. van der Laan)
Pull request description:
- Fetch the ACKs only at sign-off time. This makes sure that any last-minute ACKs are included (fixes#16200)
- Show a list of ACKs that will be included and their author before signing off, and warn if there are none
There's a slight change to the merge commit format—before it was
```
ACKs for commit 88884c:
(list of ACKs, could be empty)
```
now it is
```
ACKs for top commit:
jnewbery:
ACK 5ebc6b0eb
... (list of ACKs cannot be empty)
```
or
```
Top commit has no ACKs.
```
I don't think there's a reason to have the abbreviated commit ID there, after all the full commit id is already in the beginning of the merge commit message, and at least the abbreviated one is in every single ACK message.
ACKs for commit 0e01e4:
fanquake:
ACK 0e01e4522e
Tree-SHA512: 8576de016137d71cfc101747e9bb6779c13e0953cf2babee7afc9972bf2bd46f6912be4982b54fa5abf4d91e98e8fdae6b4ca3eef7d6892b7a5f04a7017b6882
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
71d0344cf2 docs: release note wording (Karl-Johan Alm)
3d2ff37913 wallet/rpc: use static help text (Karl-Johan Alm)
53c3c1ea9e wallet/rpc/getbalances: add entry for 'mine.used' balance in results (Karl-Johan Alm)
Pull request description:
This addresses a few remaining issues pointed out in #13756:
* First commit addresses https://github.com/bitcoin/bitcoin/pull/13756#discussion_r284907468
* Second commit addresses https://github.com/bitcoin/bitcoin/pull/13756#discussion_r294868973
Ping jnewbery and achow101 as they pointed out these issues.
ACKs for commit 71d034:
jnewbery:
ACK 71d0344cf2
meshcollider:
re-utACK 71d0344cf2
Tree-SHA512: 5e28822af0574ad07dbbed21aa2fe7866bf5770b4c0a1c150ad0da8af3152bcfb7170330a7497fa500326c594740ecf63733cf58325821e2811d7b911d5783a0
Always show the same help topic regardless of wallet flags, and explain that something is not always available, rather than runtime-modifying the help output.
5a88ea7c67 doc: remove orphaned header in developer notes (Jon Atack)
Pull request description:
The "Git and GitHub tips" section was moved from doc/developer-notes.md to doc/productivity.md in 5b76c31, but the header link to that long-gone section in the developer notes remains and needs to go.
So long, Git and GitHub tips, we barely knew ya.
ACKs for commit 5a88ea:
fanquake:
ACK 5a88ea7c67
Tree-SHA512: d2a0bea27cd9209adec2127fae5e336c44771aa46af7c544fa2c80a3df4868adafbccc30ef370369404b882fa009f7198cbf41777afd30eccf0c21a7eb1d0ad1
e61de6306f Change ismine to take a CWallet instead of CKeyStore (Andrew Chow)
7c611e2000 Move ismine to wallet module (Andrew Chow)
Pull request description:
`IsMine` isn't used outside of the wallet except for the tests. It also doesn't make sense to be outside of the wallet. This PR moves `IsMine` into the wallet module and for it to take a `CWallet` instead of `CKeyStore`. The test that used `IsMine` is also moved to the wallet tests.
This is first [prerequisites](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes#ismine) for the wallet structure changes.
ACKs for commit e61de6:
MarcoFalke:
re-ACK e61de6306f (only change is rebase with git auto-merge)
meshcollider:
Very light code review ACK e61de6306f
Tree-SHA512: 1cb4ad12652aef7922ab7460c6d413e8b9d1855dca78c0a286ae49d5c0765bc7996c55f262c742001d434eb9bd4215dc2cc7aae1b371ee1a82d46b32c17e6341
a49503402b Make and get the multisig redeemscript and destination in one function instead of two (Andrew Chow)
Pull request description:
`CreateMultisigRedeemscript()` is changed to `AddAndGetMultisigDestination()` so that the process of constructing the redeemScript and then getting the `CTxDestination` are done in the same function. This allows that function to see what the keys in the multisig are so that the correct address type is returned from `AddAndGetDestinationForScript()`.
This only effects the `createmultisig` and `addmultisigaddress` RPCs and does not change signing logic as #16022 does.
Alternative to #16022 and #16012Fixes#16011
ACKs for commit a49503:
Tree-SHA512: 5b0154a714deea3b2cc3a54beb420c95eeeacf4ca30c40ca80940d9d640f8b03611b0fc14c2f0710bfd8a79e8d27ad7d9ae380b4b83d52b40ab201624f2a63f0
The "Git and GitHub tips" section was moved from doc/developer-notes.md to doc/productivity.md in 5b76c31, but the header link to that long-gone section in the developer notes remains and needs to go.
So long, Git and GitHub tips, we barely knew ya.
fa7dd88b71 test: Add test for unknown args (MarcoFalke)
Pull request description:
Currently uncovered.
Further reading:
* https://marcofalke.github.io/btc_cov/total.coverage/src/util/system.cpp.gcov.html
* Fail on unknown config file options #15021
ACKs for commit fa7dd8:
promag:
ACK fa7dd88b71, tests looks good to me.
hebasto:
ACK fa7dd88b71, I have tested the code.
Tree-SHA512: 86ab370ce8e85925f945a52e81457b5678d71bbabcef01205a97782b780003f363552e0bad1ff678bccc784f82c6b511c3b88de3f8f25f62b0b713c387950564
Instead of creating a redeemScript with CreateMultisigRedeemscript and
then getting the destination with AddAndGetDestinationForScript, do
both in the same function.
CreateMultisigRedeemscript is changed to AddAndGetMultisigDestination.
It creates the redeemScript and returns it via an output parameter. Then
it calls AddAndGetDestinationForScript to add the destination to the
keystore and get the proper destination.
This allows us to inspect the public keys in the redeemScript before creating
the destination so that the correct destination is used when uncompressed
pubkeys are in the multisig.
e91f0a7af2 doc: Remove travis badge from readme (MarcoFalke)
Pull request description:
The readme(s) are shipped in the released source-code archive, in which case the travis badge is useless since it doesn't link to the travis result of the correct commit/tag/branch. GitHub embeds the correct links for each tag or commit that ci ran on, so we don't need this link in the readme.
ACKs for commit e91f0a:
hebasto:
ACK e91f0a7af2
Tree-SHA512: 860435a58b38a9bd0bc62a1e74b3a63c138c9a2f09008a090d5ecc7fd86fa908d2e5eda41d16606507a238d9488fa5323405364a9556b670684a2e4838aead2d
4971be76a7 docs: add rapidcheck to vcpkg install list (fanquake)
Pull request description:
Setting up a fresh Windows 10 VM using [this guide](https://github.com/fanquake/core-review/blob/master/windows.md), installing `rapidcheck` and building using the changes in #16235 resulted in a working `bitcoind` and all tests passing.
ACKs for commit 4971be:
Tree-SHA512: d0cb2d1d6ac5cdecf77c21f9b05e8803266511dbd06cb644352a229d101c7bf63f8022822852877371cce83c414275a850eb9ab6876a61c2fd1061627b7223f7
750d1bc7df Cleaned up and consolidated msvc build files to allow faster builds and easier migration to vs2019. (Aaron Clauson)
Pull request description:
This PR does the following:
- Big cleanup of the msbuild project files and consolidation of common build parameters into a single common include,
- Gets ready for switch to building with Visual Studio 2019 and Platform Toolset v142,
- Will allow me to rebase #15529 to include the Qt Projects in the msbuild config.
ACKs for commit 750d1b:
fanquake:
ACK 750d1bc7df
NicolasDorier:
tACK 750d1bc . Good to see the boilerplate go away.
Tree-SHA512: 15b2c3e095037d2b927aecf81593be6b6963a944c40d8c6b270cd0a2e1f8b67347bf6a4676a15d7ba1831a60f4ee4e6a405611e1ff5a27aa4888b2f857eef22b
5ebc6b0eb2 bitcoind: update -avoidpartialspends description to account for auto-enable for avoid_reuse wallets (Karl-Johan Alm)
ada258f8c8 doc: release notes for avoid_reuse (Karl-Johan Alm)
27669551da wallet: enable avoid_partial_spends by default if avoid_reuse is set (Karl-Johan Alm)
8f2e208f7c test: add test for avoidreuse feature (Karl-Johan Alm)
0bdfbd34cf wallet/rpc: add 'avoid_reuse' option to RPC commands (Karl-Johan Alm)
f904723e0d wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS (Karl-Johan Alm)
8247a0da3a wallet: enable avoid_reuse feature (Karl-Johan Alm)
eec15662fa wallet: avoid reuse flags (Karl-Johan Alm)
58928098c2 wallet: make IsWalletFlagSet() const (Karl-Johan Alm)
129a5bafd9 wallet: rename g_known_wallet_flags constant to KNOWN_WALLET_FLAGS (Karl-Johan Alm)
Pull request description:
Add a new wallet flag called `avoid_reuse` which, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination.
This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments.
This replaces #10386 and together with the (now merged) #12257 it addresses #10065 in full. The concerns raised in https://github.com/bitcoin/bitcoin/pull/10386#issuecomment-302361381 are also addressed due to #12257.
~~Note: this builds on top of #15780.~~ (merged)
ACKs for commit 5ebc6b:
jnewbery:
ACK 5ebc6b0eb
laanwj:
Concept and code-review ACK 5ebc6b0eb2
meshcollider:
Code review ACK 5ebc6b0eb2
achow101:
ACK 5ebc6b0eb2 modulo above nits
Tree-SHA512: fdef45826af544cbbb45634ac367852cc467ec87081d86d08b53ca849e588617e9a0a255b7e7bb28692d15332de58d6c3d274ac003355220e4213d7d9070742e
