Quoting src/test/README.md, 'Adding test cases':
"The file naming convention is `<source_filename>_tests.cpp`
and such files should wrap their tests in a test suite
called `<source_filename>_tests`."
Currently the unit test source file txvalidationcache_tests.cpp contains a unit
test suite with the name tx_validationcache_tests, which is fixed by this commit.
The following shell script shows that this is the only mismatch and for all other
unit test source files the test suite names are correct:
#!/bin/bash
shopt -s globstar
for test_full_filename in **/*_tests.cpp; do
test_name_file=`basename $test_full_filename .cpp`
test_name_suite=`sed -n "s/^.*TEST_SUITE(\(.*_tests\).*$/\1/p" $test_full_filename`
if [ $test_name_file != $test_name_suite ]; then
echo "TestFilename: $test_name_file != TestSuitname: $test_name_suite"
fi
done
4f7127d1e3 gui: Make Intro consistent with prune checkbox (Hennadii Stepanov)
4824a7d36c gui: Add Intro::UpdateFreeSpaceLabel() (Hennadii Stepanov)
daa3f3fa90 refactor: Add Intro::UpdatePruneLabels() (Hennadii Stepanov)
e4caa82a03 refactor: Replace static variable with data member (Hennadii Stepanov)
2bede28cd9 util: Add PruneGBtoMiB() function (Hennadii Stepanov)
e35e4b2ba0 util: Add PruneMiBtoGB() function (Hennadii Stepanov)
Pull request description:
On master (a6f6333ba2) and on 0.19.0.1 the intro dialog with prune enabled (checkbox "Discard blocks..." is checked) provides a user with wrong info about the required disk space:
Also the paragraph "If you have chosen to limit..." is missed.
---
With this PR when prune checkbox is toggled, the related text labels and the amount of required space shown are updated (previously they were only updated when the data directory was updated):
---
This PR is an alternative to #17035.
**ryanofsky**'s [suggestion](https://github.com/bitcoin/bitcoin/pull/17035#discussion_r337594268) also has been implemented.
ACKs for top commit:
emilengler:
ACK 4f7127d1e3
Sjors:
tACK 4f7127d1e3
ryanofsky:
Code review ACK 4f7127d1e3. It seems like there are a few visible changes here:
jonasschnelli:
utACK 4f7127d1e3
Tree-SHA512: fa0bbdcfafde97d7906cda066cbd4608b936a71cae1b4cda3ee3aa2eed3a9795f279f14c6b1b4997278e094db891c7d3bb695368ba0882347aa42165a86e5172
4c524f0aad Bugfix: GUI: Hide the HD/encrypt icons earlier so they get re-shown if another wallet is open (Luke Dashjr)
Pull request description:
To reproduce bug, open 2 wallets, and close 1. You end up left without the HD/encrypt icons, despite having a wallet open still.
This works because the icons are re-shown after we remove the current wallet (if there's another wallet still open).
ACKs for top commit:
promag:
Tested ACK 4c524f0aad.
jonasschnelli:
utACK 4c524f0aad
hebasto:
ACK 4c524f0aad, tested on Linux Mint 19.3.
Tree-SHA512: 4ef1bd4a0ae2f20ace9d02bc5d778640c11e46a86f30b762f8502e577f85114f0644d51a70cfbc4c23b51869c3caf20e94548aa64f51fdb85aea5f194a23fca6
44f15cfdcf gui: renamed 'debug window' to 'node window' (Zero)
Pull request description:
**Edit**: I have now limited the change in this PR to only renaming the window title from `Debug Window` to `Node Window`. Check [this comment](https://github.com/bitcoin/bitcoin/pull/17096#issuecomment-542837511) for more details.
This PR is in response to #17082, which aims to rename the `Debug window` title to a more user friendly term; `Node window`.
Closes#17082
ACKs for top commit:
hebasto:
ACK 44f15cfdcf, tested on Linux Mint 19.3:
theStack:
ACK 44f15cfdcf, tested on Linux (Lubuntu 16.04):
Tree-SHA512: 9fc73f2e67badb38525c550ce4c313288858b3fde30ef17fee85230be5bf31cf94408c699265b5e1256dfed60f8d04f48927d9b2831ba9f25498b98e6fa7180f
Needed for future ScriptPubKeyMans which may need to create
SigningProviders dynamically and thus a normal pointer is not enough
This commit does not change behavior.
Add wallet logic for dealing with multiple ScriptPubKeyMan instances. This
doesn't change current behavior because there is still only a single
LegacyScriptPubKeyMan. But in the future the new logic will be used to support
descriptor wallets.
c491368d8c scripts: add MACHO dylib checking to symbol-check.py (fanquake)
76bf97213f scripts: fix check-symbols & check-security argument passing (fanquake)
Pull request description:
Based on #17857.
This adds dynamic library checks for MACHO executables to symbol-check.py. The script has been modified to function more like `security-check.py`. The error output is now also slightly different. i.e:
```bash
# Linux x86
bitcoin-cli: symbol operator new[](unsigned long) from unsupported version GLIBCXX_3.4
bitcoin-cli: export of symbol vtable for std::basic_ios<char, std::char_traits<char> > not allowed
bitcoin-cli: NEEDED library libstdc++.so.6 is not allowed
bitcoin-cli: failed IMPORTED_SYMBOLS EXPORTED_SYMBOLS LIBRARY_DEPENDENCIES
# RISCV (skips exported symbols checks)
bitcoin-tx: symbol operator new[](unsigned long) from unsupported version GLIBCXX_3.4
bitcoin-tx: NEEDED library libstdc++.so.6 is not allowed
bitcoin-tx: failed IMPORTED_SYMBOLS LIBRARY_DEPENDENCIES
# macOS
Checking macOS dynamic libraries...
libboost_filesystem.dylib is not in ALLOWED_LIBRARIES!
bitcoind: failed DYNAMIC_LIBRARIES
```
Compared to `v0.19.0.1` the macOS allowed dylibs has been slimmed down somewhat:
```diff
src/qt/bitcoin-qt:
/usr/lib/libSystem.B.dylib
-/System/Library/Frameworks/DiskArbitration.framework/Versions/A/DiskArbitration
/System/Library/Frameworks/IOKit.framework/Versions/A/IOKit
/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation
/System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices
/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit
/System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices
/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
-/System/Library/Frameworks/Security.framework/Versions/A/Security
-/System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration
/System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics
-/System/Library/Frameworks/OpenGL.framework/Versions/A/OpenGL
-/System/Library/Frameworks/AGL.framework/Versions/A/AGL
/System/Library/Frameworks/Carbon.framework/Versions/A/Carbon
/usr/lib/libc++.1.dylib
-/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork
/System/Library/Frameworks/CoreText.framework/Versions/A/CoreText
/System/Library/Frameworks/ImageIO.framework/Versions/A/ImageIO
/usr/lib/libobjc.A.dylib
```
ACKs for top commit:
laanwj:
ACK c491368d8c
Tree-SHA512: f8624e4964e80b3e0d34e8d3cc33f3107938f3ef7a01c07828f09b902b5ea31a53c50f9be03576e1896ed832cf2c399e03a7943a4f537a1e1c705f3804aed979
75163f4729 bug-fix macos: give free bytes to F_PREALLOCATE (Karl-Johan Alm)
Pull request description:
The macos manpage for `fcntl` (for `F_PEOFPOSMODE`) states:
> Allocate from the physical end of file. In this case, fst_length indicates the number of newly allocated bytes desired.
This would result in the rev files being essentially pre-allocating 2x their necessary size (this is the case for block files as well, but these are flushed down to their right sizes every time) as they would pre-allocate `pos + length` **free** bytes, rather than allocating `length` bytes after `pos`, as expected.
Fixes#17827.
ACKs for top commit:
eriknylund:
ACK 75163f4729 built locally. All tests passing. Manual test as per my previous comment above on an older commit, using an APFS unencrypted disk image with 3 GB.
laanwj:
code review ACK 75163f4729
Tree-SHA512: 105c8d56c20acad8febdf0583f1e5721b63376ace325a7a62c2e4b15a442c7131404ed604c32c0cda716791d7ca5aa9f5b6a774ff86e39838bc7e87ca3c42760
70e4706093 Revert "refactor: Remove never used default parameter" (Hennadii Stepanov)
219417b388 Revert "refactor: Simplify connection syntax" (Hennadii Stepanov)
Pull request description:
The code, the `bool* ret = nullptr` parameter in the `BitcoinGUI::message()` slot, removed in #17943 is not dead actually. It is used in `ThreadSafeMessageBox()` function:
a654626f07/src/qt/bitcoingui.cpp (L1363-L1368)
Now in master (a654626f07):
```
$ ./src/qt/bitcoin-qt -prune=-1
Error: Prune cannot be configured with a negative value.
bitcoin-qt: qt/bitcoingui.cpp:1369: bool ThreadSafeMessageBox(BitcoinGUI*, const string&, const string&, unsigned int): Assertion `invoked' failed.
Aborted (core dumped)
```
This PR reverts all commits of #17943
Additional notes: the bug was missed due to dynamic function call `QMetaObject::invokeMethod()` which cannot be checked at compile time. See #16348 for more discussion.
Sorry for introducing a bug.
ACKs for top commit:
Sjors:
ACK 70e4706093
laanwj:
ACK 70e4706093
Tree-SHA512: b968a026eaa4f5f39fd36ddc715d8e233f3c6420e6580f11d4ca422a5ff5d1d9d3df9ac11b353c3d4f434d67d6a69e37d2e26b8248d72bedd14ecba0a545a327
9dd58ca611 init: Stop indexes on shutdown after ChainStateFlushed callback. (Jim Posen)
Pull request description:
Replaces https://github.com/bitcoin/bitcoin/pull/17852.
Currently, the latest index state may not be committed to disk on shutdown. The state is committed on `ChainStateFlushed` callbacks and the current init order unregisters the indexes as validation interfaces before the final `ChainStateFlushed` callback is called on them.
Issue identified by paulyc.
For review: an alternative or supplemental solution would be to call `Commit` at the end of `BaseIndex::Stop`. I don't see any harm in doing so and it makes the less prone to user error. However, the destructor would have to be modified to not call `Stop` because `Commit` calls a virtual method, so I figured it wasn't worth it. But I'm curious how others feel.
ACKs for top commit:
fjahr:
tested ACK 9dd58ca611
paulyc:
> Code review ACK [9dd58ca](9dd58ca611), but failed to test because I can't reproduce the original problem.
kallewoof:
Tested ACK 9dd58ca611
promag:
Code review ACK 9dd58ca611, but failed to test because I can't reproduce the original problem.
Tree-SHA512: 2918380b699833cb7eab07456d1667dbf8ebbe2d2b5988300a3cf5b6a6cfc818b6d9086e1936ffe7881f67e409306c4b91d61a08a169cfd0a301383479d4f3cb
a5a2654bbc test: add missing #include to fix compiler errors (Karl-Johan Alm)
Pull request description:
I believe this fixes AppVeyor errors in master. Will close if that is not the case.
Closes#17976
ACKs for top commit:
fanquake:
ACK a5a2654bbc - glad the fix turned out to be this simple.
Tree-SHA512: 8fed8c2050d0f435e7ed6db1c2927d5daccc3540c6cf9e57e644d0931a740359550a5270201c893f40200960101f11cd039d807d4ed0190f1e0c674f86fd7290
3c30d7118a QT: Change bumpFee asserts to simple error message (Gregory Sanders)
e3b19d8696 QT: bump fee returns PSBT on clipboard for watchonly-only wallets (Gregory Sanders)
Pull request description:
Very small set of changes to support PSBT-based fee bumping on watchonly wallets in QT.
quasi-companion to https://github.com/bitcoin/bitcoin/pull/16373
ACKs for top commit:
gwillen:
code review ACK 3c30d71
promag:
Code review ACK 3c30d7118a.
Sjors:
utACK 3c30d71
achow101:
ACK 3c30d7118a
Tree-SHA512: 7a706141e46d7fd0ad513a08a96c16f2e7e531427a6776b689362f82e32cbd9d4b7eeb98f6936aa3f9347d23ccc94128516fcffa695efacd9cac43606ea916e2
3d5d7aad26 windows: remove call to SetProcessDEPPolicy (fanquake)
f2645c2601 windows: Enable heap terminate-on-corruption (fanquake)
Pull request description:
This PR is currently two separate changes:
#### Enable heap terminate-on-corruption
This is default behavior from Windows 8 onwards, however we still support Windows 7, so it should make sense to explicitly enable this. This is also done by projects like tor, chromium etc.
> Enables the terminate-on-corruption feature. If the heap manager detects an error in any heap used by the process, it calls the Windows Error Reporting service and terminates the process.
After a process enables this feature, it cannot be disabled.
More info [here](https://docs.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heapsetinformation).
#### Remove call to SetProcessDEPPolicy()
DEP is always enabled on 64-bit Windows processes, and `SetProcessDEPPolicy()` only works when called from a 32-bit process. I've tested that our current usage always fails ([as expected](https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setprocessdeppolicy#remarks)) with [ERROR_NOT_SUPPORTED](16151c441e/mingw-w64-headers/include/error.h (L42)).
Please don't add a "Needs gitian build" tag here yet.
ACKs for top commit:
sipsorcery:
ACK 3d5d7aad26.
laanwj:
ACK 3d5d7aad26
Tree-SHA512: 0948bcf165685b6b573f2cd950680c34356b856690de655ced2b93d497e02e7b22aa195c99f6ce33202f182622c67302ff31c98ab51b7d050574af3debdee5ce
297e098557 Fix doxygen errors (Ben Woosley)
Pull request description:
These are all the remaining errors identified via -Werror=documentation, e.g.:
```
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
```
You can use this to run with `-Wdocumentation` yourself: #14920
ACKs for top commit:
laanwj:
ACK 297e098557
Tree-SHA512: a232d893b170873d923e77fa56c56a6567e7fd120b5af1f52cfeeae1093eec55621604cc80a523678f6fedc8bbb31228c4aa8dc2a630ce9ffc91525988522af7
9b66083788 Convert chain to new serialization (Pieter Wuille)
2f1b2f4ed0 Convert VARINT to the formatter/Using approach (Pieter Wuille)
ca62563df3 Add a generic approach for (de)serialization of objects using code in other classes (Pieter Wuille)
Pull request description:
This is a second carve-out from #10785.
This introduces a const-correct generic approach for serializing objects using custom serializers (defined separately from the object being serialized), then converts VARINT to use that approach, and then converts chain.h to the new framework (including the new const-correct VARINT macro).
ACKs for top commit:
jamesob:
ACK 9b66083788 ([`jamesob/ackr/17896.1.sipa.serialization_improvemen`](https://github.com/jamesob/bitcoin/tree/ackr/17896.1.sipa.serialization_improvemen))
ryanofsky:
Code review ACK 9b66083788. Only change since last review is suggested lvalue reference tweak
Tree-SHA512: 2da4af1754699cb223d6beae44c587555e39ef6951448488a04783c92e2dfd4a305934f71cc3a75d06faf6d722723d8cdbd5ccb12039783f8d62039b83987bb8
1a53b0da60 refactor: Simplify connection syntax (Hennadii Stepanov)
7d0a8f4f53 refactor: Remove never used default parameter (Hennadii Stepanov)
Pull request description:
In `BitcoinGUI::message()` slot the `bool* ret = nullptr` parameter is never used.
This PR removes it and simplifies connections syntax by replacing lambdas with the `&BitcoinGUI::message` slot.
ACKs for top commit:
promag:
Code review ACK 1a53b0da60.
Sjors:
Tested ACK 1a53b0da60
Empact:
Code review ACK 1a53b0da60
Tree-SHA512: e287c3218d31a387338d50da3de79c27e8691829449c3a75a2f75bb1c680bd81eb9de43e4dd3646560a422d4a45c84debfce9783c4376b50aa5cde491f300688
c279a81e9c gui: Remove warning "unused variable 'wallet_model'" (João Barbosa)
Pull request description:
This was part of the abandoned #15150.
ACKs for top commit:
theStack:
utACK c279a81e9c
fanquake:
ACK c279a81e9c - tested wallet loading/unloading in the qt rpc console.
Tree-SHA512: 8fbd55c7e213599c7be843b52e960a16cf965b3e01489f426ac3ed9d579d78bb4b2ac230bcccd8abe0397a8b1166ee10e0d685738441a77a5dcb5135c15790fa
Identified via -Wdocumentation, e.g.:
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
2b1641492f wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa)
Pull request description:
Improve `CWallet:MarkDestinationsDirty` by skipping transactions that already have the cache invalidated. Skipping a transaction avoids at worst case extracting all output destinations.
ACKs for top commit:
meshcollider:
re-utACK 2b1641492f
Tree-SHA512: 479dc2dde4b653b856e3d6a0c59a34fe33e963eb131a2d88552a8b30471b8725a087888fe5d7db6e4ee19b74072fe64441497f033be7d1931637f756e0d8fef5
fac86ac7b3 scripted-diff: Add missed copyright headers (Hennadii Stepanov)
6fde9d5e47 script: Update EXLUDE list in copyright_header.py (Hennadii Stepanov)
1998152f15 script: Add empty line after C++ copyright (Hennadii Stepanov)
071f2fc204 script: Add ability to insert copyright to *.sh (Hennadii Stepanov)
Pull request description:
This PR improves `contrib/devtools/copyright_header.py` script and adds copyright headers to the files in `src` and `test` directories with two exceptions:
- [`src/reverse_iterator.h`](https://github.com/bitcoin/bitcoin/blob/master/src/reverse_iterator.h) (added to exceptions)
- [`src/test/fuzz/FuzzedDataProvider.h`](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/FuzzedDataProvider.h) (added to exceptions)
On master 5622d8f315:
```
$ ./contrib/devtools/copyright_header.py report . | grep zero
25 with zero copyrights
```
With this PR:
```
$ ./contrib/devtools/copyright_header.py report . | grep zero
2 with zero copyrights
```
~I am uncertain about our copyright policy with `build_msvc` and `contrib` directories content, so they are out of scope of this PR.~
ACKs for top commit:
MarcoFalke:
ACK fac86ac7b3
Tree-SHA512: d7832c4a7a1a3b7806119775b40ec35d7982f49ff0e6199b8cee4c0e0a36e68d51728b6ee9924b1c161df4bc6105bd93391b79d42914357fa522f499cb113fa8
6dd59d2e49 Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders)
4b8f1e989f IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders)
Pull request description:
Regression introduced in https://github.com/bitcoin/bitcoin/pull/17621 which causes p2sh-segwit addresses to be erroneously missed.
Tests are only failing in 0.19 branch, likely because that release still uses p2sh-segwit addresses rather than bech32 by default.
I'll devise a test case to catch this going forward.
ACKs for top commit:
achow101:
ACK 6dd59d2e49
MarcoFalke:
ACK 6dd59d2
meshcollider:
Code review ACK 6dd59d2e49
Tree-SHA512: b3e0f320c97b8c1f814cc386840240cbde2761fee9711617b713d3f75a4a5dce2dff2df573d80873df42a1f4b74e816ab8552a573fa1d62c344997fbb6af9950
6fc554f591 wallet: Reset reused transactions cache (Fabian Jahr)
Pull request description:
Fixes#17603 (together with #17824)
`getbalances` is using the cache within `GetAvailableCredit` under certain conditions [here](35fff5be60/src/wallet/wallet.cpp (L1826)). For a wallet with `avoid_reuse` activated this can lead to inconsistent reporting of `used` transactions/balances between `getbalances` and `listunspent` as pointed out in #17603. When an address is reused before the first transaction is spending from this address, the cache is not updated even after the transaction is sent. This means the remaining outputs at the reused address are not showing up as `used` in `getbalances`.
With this change, any newly incoming transaction belonging to the wallet marks all the other outputs at the same address as dirty.
ACKs for top commit:
kallewoof:
Code review re-ACK 6fc554f591
promag:
ACK 6fc554f591.
achow101:
Re-ACK 6fc554f591
meshcollider:
Code review ACK 6fc554f591
Tree-SHA512: c4cad2c752176d16d77b4a4202291d20baddf9f27250896a40274d74a6945e0f6b34be04c2f9b1b2e756d3ac669b794969df8f82a98e0b16f10e92f276649ea2
5855cc564f bitcoin-wallet: Use PACKAGE_NAME in usage help (Luke Dashjr)
7f5db163a4 GUI: Use PACKAGE_NAME in modal overlay (Luke Dashjr)
Pull request description:
ACKs for top commit:
hebasto:
ACK 5855cc564f, checked with
fanquake:
ACK 5855cc564f - checked `bitcoin-wallet` and a `--disable-wallet` `bitcoin-qt`.
Tree-SHA512: 3526eb122bfdbc63349d12251f17ffa20c7f3754af4ac9c554e6d36bb14b351f31c413c30401bb3d6e0e6200b72614dfc8475489b1f742b0423bd83fba758b94
The macos manpage for fcntl (for F_PEOFPOSMODE) states:
> Allocate from the physical end of file. In this case, fst_length indicates the number of newly allocated bytes desired.
-BEGIN VERIFY SCRIPT-
# Delete outdated alias for RecursiveMutex
sed -i -e '/CCriticalSection/d' ./src/sync.h
# Replace use of outdated alias with RecursiveMutex
sed -i -e 's/CCriticalSection/RecursiveMutex/g' $(git grep -l CCriticalSection)
-END VERIFY SCRIPT-
When prune checkbox is toggled, the related text labels and the amount
of required space shown are updated (previously they were only updated
when the data directory was updated).
8313fa8e81 gui: Set CConnman byte counters earlier to avoid uninitialized reads (Russell Yanofsky)
Pull request description:
Initialize CConnman byte counters during construction, so GetTotalBytesRecv() and GetTotalBytesSent() methods don't return garbage before Start() is called.
Change shouldn't have any effect outside of the GUI. It just fixes a race condition during a qt test that was observed on travis: https://travis-ci.org/bitcoin/bitcoin/jobs/634989685
ACKs for top commit:
MarcoFalke:
ACK 8313fa8e81
promag:
ACK 8313fa8e81.
Tree-SHA512: 97c246da4e28e6e0b48f685b840f96746ad75c4b157a692201c6c4702db328a88ead8507d8e1b4e608aa1882513174ec60cf3977c31b7a9d76678cc9f49b45f8
This adds the (internal) Wrapper class, and the Using function that uses it. Given
a class F that implements Ser(stream, const object&) and Unser(stream, object&)
functions, this permits writing e.g. READWRITE(Using<F>(object)).
If a destination is reused we mark the cache of the other transactions going to that destination dirty so they are not accidentally reported as trusted when the cache is hit.
02b9511d6b tests: add tests for GetCoinsCacheSizeState (James O'Beirne)
b17e91d842 refactoring: introduce CChainState::GetCoinsCacheSizeState (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
This pulls out the routine for detection of how full the coins cache is from
FlushStateToDisk. We use this logic independently when deciding when to flush
the coins cache during UTXO snapshot activation ([see here](231fb5f17e (diff-24efdb00bfbe56b140fb006b562cc70bR5275))).
ACKs for top commit:
ariard:
Code review ACK 02b9511.
ryanofsky:
Code review ACK 02b9511d6b. Just rebase, new COIN_SIZE comment, and new test message since last review
Tree-SHA512: 8bdd78bf68a4a5d33a776e73fcc2857f050d6d102caa4997ed19ca25468c1358e6e728199d61b423033c02e6bc8f00a1d9da52cf17a2d37d70860fca9237ea7c
Initialize CConnman byte counters during construction, so GetTotalBytesRecv()
and GetTotalBytesSent() methods don't return garbage before Start() is called.
Change shouldn't have any effect outside of the GUI. It just fixes a race
condition during a qt test that was observed on travis:
https://travis-ci.org/bitcoin/bitcoin/jobs/634989685
In TransactionTablePriv::index, avoid calling
interfaces::Wallet::tryGetTxStatus if the status is up to date as of the most
recent NotifyBlockTip notification. Store height from the most recent
notification in a new ClientModel::cachedNumBlocks variable in order to check
this.
This avoids floods of IPC traffic from tryGetTxStatus with #10102 when there
are a lot of transactions. It might also make the GUI a little more efficient
even when there is no IPC.
f9abf4ab6d Add logging for CValidationInterface events (Jeffrey Czyz)
6edebacb21 Refactor FormatStateMessage for clarity (Jeffrey Czyz)
72f3227c83 Format CValidationState properly in all cases (Jeffrey Czyz)
428ac70095 Add VALIDATION to BCLog::LogFlags (Jeffrey Czyz)
Pull request description:
Add logging of `CValidationInterface` callbacks using a new `VALIDATIONINTERFACE` log flag (see #12994). A separate flag is desirable as the logging can be noisy and thus may need to be disabled without affecting other logging.
This could help debug issues where there may be race conditions at play, such as #12978.
ACKs for top commit:
jnewbery:
ACK f9abf4ab6d
hebasto:
ACK f9abf4ab6d
ariard:
ACK f9abf4a, only changes since 0cadb12 are replacing log indication `VALIDATIONINTERFACE` by `VALIDATION` and avoiding a forward declaration with a new include
ryanofsky:
Code review ACK f9abf4ab6d. Just suggested changes since last review (thanks!)
Tree-SHA512: 3e0f6e2c8951cf46fbad3ff440971d95d526df2a52a2e4d6452a82785c63d53accfdabae66b0b30e2fe0b00737f8d5cb717edbad1460b63acb11a72c8f5d4236
6d6a7a8403 gui: Fix duplicate wallet showing up (João Barbosa)
81ea66c30e Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky)
Pull request description:
This PR includes 2 fixes:
- prevent GUI LoadWallet handlers from crashing on startup when multiple handlers are attached, because the first handler takes ownership of the wallet unique pointer. Now every handler will receive its own unique pointer;
- prevent showing a wallet twice in the GUI on startup due to a race with `loadwallet`.
Fixes#16937
ACKs for top commit:
fjahr:
code review ACK 6d6a7a8403
ryanofsky:
Code review ACK 6d6a7a8403. No changes since last ACK other than rebase due to #17070
kallewoof:
Code review ACK 6d6a7a8403
Tree-SHA512: 7f0658c9011f81dfa176a094c2263448ee1d14fda7dc94e8b55ee9c8b81538bd2d1e4bf8a8dbfcd029ebfc9feb6d3cda9dee3f911122df0a4b1e0ca75f653ba4
3e730bf90a zmq: Fix due to invalid argument and multiple notifiers (João Barbosa)
Pull request description:
ZMQ initialization is interrupted if any notifier fails, and in that case all notifiers are destroyed. The notifier shutdown assumes that the initialization had occurred. This is not valid when there are multiple notifiers and any except the last fails to initialize.
Can be tested by running test/functional/interface_zmq.py from this branch with bitcoind from master.
Closes#17185.
ACKs for top commit:
laanwj:
Code review ACK 3e730bf90a, thanks for adding a test
Tree-SHA512: 5da710e97dcbaa94896d019e75162d470f6d381ee07c60e5b3e9db93d11e8f7ca9bf2c509efa4486199e88c96c3e720cc96b4e35b62725d4c7db8e8e9bf6e09d
af112ab628 qt: Rename SetPrune() to InitializePruneSetting() (Hennadii Stepanov)
b0bfbe5028 refactor: Drop `bool force' parameter (Hennadii Stepanov)
68c9bbe9bc qt: Force set nPruneSize in QSettings after intro (Hennadii Stepanov)
a82bd8fa57 util: Replace magics with DEFAULT_PRUNE_TARGET_GB (Hennadii Stepanov)
Pull request description:
On master (5622d8f315), having `QSettings` set already
```
$ grep nPruneSize ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf
nPruneSize=6
```
enabling prune option in the intro dialog
```
$ ./src/qt/bitcoin-qt -choosedatadir -testnet
```
has no effect:
```
$ grep Prune ~/.bitcoin/testnet3/debug.log
2019-12-08T10:04:41Z Prune configured to target 5722 MiB on disk for block and undo files.
```
---
With this PR:
```
$ grep Prune ~/.bitcoin/testnet3/debug.log
2019-12-08T10:20:35Z Prune configured to target 1907 MiB on disk for block and undo files.
```
This PR has been split of #17453 (the first two commits) as it fixes an orthogonal bug.
Refs:
- https://github.com/bitcoin/bitcoin/pull/17453#discussion_r345424240
- https://github.com/bitcoin/bitcoin/pull/17453#discussion_r350960201
ACKs for top commit:
Sjors:
Code review re-ACK af112ab628
ryanofsky:
Code review ACK af112ab628. Just suggested changes since last review (thanks!)
promag:
Tested ACK af112ab628. Latest suggestions and changes look good to me.
Tree-SHA512: 8ddad34b30dcc2cdcad6678ba8a0b36fa176e4e3465862ef6eee9be0f98d8146705138c9c7995dd8c0990af41078ca743fef1a90ed9240081f052f32ddec72b9
fa37e0a68b test: Show debug log on unit test failure (MarcoFalke)
Pull request description:
Often, it is hard to debug unit test failures without the debug log. Especially when the failure happens remotely (e.g. on a ci system).
Fix that by printing the log on failure.
ACKs for top commit:
jamesob:
ACK fa37e0a68b ([`jamesob/ackr/16975.1.MarcoFalke.test_show_debug_log_on_u`](https://github.com/jamesob/bitcoin/tree/ackr/16975.1.MarcoFalke.test_show_debug_log_on_u))
Tree-SHA512: 2ca4150c4ae3d4ad47e03b5e5e70da2baffec928ddef1fdf53a3ebc061f14aee249205387cb1b12ef6d4eb55711ef0080c0b41d9d18000b5da124ca80299793b
a004673c54 qt: Add LogQtInfo() function (Hennadii Stepanov)
Pull request description:
This PR adds some info to `debug.log` I found useful for testing (e.g., on Wayland) and debugging issues like #17153:
```
$ ./src/qt/bitcoin-qt -printtoconsole | head -n 6
2020-01-04T14:57:40Z [main] Bitcoin Core version v0.19.99.0-0df287f4e (release build)
2020-01-04T14:57:40Z [main] InitParameterInteraction: parameter interaction: -externalip set -> setting -discover=0
2020-01-04T14:57:40Z [main] Qt 5.9.5 (dynamic), plugin=xcb (dynamic)
2020-01-04T14:57:40Z [main] System: Linux Mint 19.3, x86_64-little_endian-lp64
2020-01-04T14:57:40Z [main] Screen: HDMI-1 1600x1200, pixel ratio=1.0
2020-01-04T14:57:40Z [main] Assuming ancestors of block 00000000000000b7ab6ce61eb6d571003fbe5fe892da4c9b740c49a07542462d have valid signatures.
```
ACKs for top commit:
laanwj:
ACK a004673c54
Tree-SHA512: 496bcfd4870a2730eab92b96b3e74989a7904b21369c372b6d4368f4ca2c141e2fdc1348a1fdd18cb68bb144dcea01d3023bb782f9d030e330c187f6a5a1a082
63bf06afc3 Restore English translation option (Andrew Chow)
Pull request description:
It was [reported on Bitcointalk](https://bitcointalk.org/index.php?topic=5204167.msg53540137#msg53540137) that the normal English language option was lost in 0.19. This PR restores it. For some reason it was removed during the last periodic translation update.
ACKs for top commit:
laanwj:
ACK 63bf06afc3
Tree-SHA512: 94c7c7407f69e8df91fbbd8f8c5e3e8e031d308b72d775a00bcee564f2762a92f65c140029ce805faccdb767a25c0e222a396708c6ce29a5882bab939a45b772
e1e1442f3e Activate no-privkey -> ISMINE_WATCH_ONLY behavior for LegacySPKM only (Gregory Sanders)
Pull request description:
Slight cleanup following https://github.com/bitcoin/bitcoin/pull/16944
This should allow future scriptpubkeymans to transparently work, since the current plan is to have ismine always be spendable.
ACKs for top commit:
achow101:
ACK e1e1442f3e
Sjors:
Code review ACK e1e1442f3e
meshcollider:
Code review ACK e1e1442f3e
Tree-SHA512: c0a86587d33b8b1646494a5cb0bf8681ee4a88e6913918157746943a0996b501903e0e6ee954cf04154c1e0faee0cbb375c74ca789f46ba9244eb5296632b042
8925df86c4 doc: update release notes (Jon Atack)
8bb405bbad test: getaddressinfo labels purpose deprecation test (Jon Atack)
60aba1f2f1 rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack)
7851f14ccf rpc: incorporate review feedback from PR 17283 (Jon Atack)
Pull request description:
This PR builds on #17283 (now merged) and is followed by #17585.
It modifies the value returned by rpc getaddressinfo `labels` to an array of label name strings and deprecates the previous behavior of returning an array of JSON hash structures containing label `name` and address `purpose` key/value pairs.
before
```
"labels": [
{
"name": "DOUBLE SPEND",
"purpose": "receive"
}
```
after
```
"labels": [
"DOUBLE SPEND"
]
```
The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=labelspurpose`.
For context, see:
- https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001
- http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-13.html#l-425 (lines 425-427)
- http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622
Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=labelspurpose` flag while verifying the rpc getaddressinfo help text and `labels` output.
Next steps: deprecate the rpc getaddressinfo `label` field (EDIT: done in #17585) and add support for multiple labels per address. This PR will unblock those.
ACKs for top commit:
jnewbery:
reACK 8925df8
promag:
Code review ACK 8925df86c4.
meshcollider:
Code review ACK 8925df86c4
Tree-SHA512: c2b717209996da32b6484de7bb8800e7048410f9ce6afdb3e02a6866bd4a8f2c730f905fca27b10b877b91cf407f546e69e8c4feb9cd934325a6c71c166bd438
091a876664 Test watchonly wallet bumpfee with PSBT return (Gregory Sanders)
e9b4f9419c bumpfee: Return PSBT when wallet has privkeys disabled (Gregory Sanders)
75a5e478b6 Change bumpfee to use watch-only funds for legacy watchonly wallets (Gregory Sanders)
Pull request description:
The main use-case here is for using with watch-only wallets with PSBT-signing cold wallets of all kinds.
ACKs for top commit:
achow101:
ACK 091a876664
Sjors:
Tested ACK 091a876664
meshcollider:
utACK 091a876664
Tree-SHA512: f7cf663e1af0b029e5c99eac88c5fdc3bc9e9a3841da8a608e8a9957e9bcf6a78864b8c2706fcaf78a480ffe11badd80c4fad29f97c0bb929e0470fafda5c22e
09502452bb IsUsedDestination should count any known single-key address (Gregory Sanders)
Pull request description:
This plugs the privacy leak detailed at https://github.com/bitcoin/bitcoin/issues/17605, at least for the single-key case.
ACKs for top commit:
meshcollider:
Code Review ACK 09502452bb
Tree-SHA512: e1d68281675f05072b3087171cba1df9416a69c9ccf70c72e8555e55eadda2d0fd339e5a894e3a3438ff94b9e3827fb19b8b701faade70c08756b19ff157ee0c
71af793512 scripts: fix check-symbols & check-security argument passing (fanquake)
Pull request description:
The first argument in `bin_PROGRAMS` (`bitcoind`) was being silently consumed and never passed into the [`security-check.py`](https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/security-check.py) or [`symbol-check.py`](https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/symbol-check.py) scripts.
This seems to have been the case since the scripts were added to the makefile in f3d3eaf78e.
Example of the behavior:
```python
# touch a, touch b, touch c
# python3 args.py < a b c
import sys
if __name__ == '__main__':
print(sys.argv)
# ['args.py', 'b', 'c']
# if you add some lines to "a",
# you'll see them here..
for line in sys.stdin:
print(line)
```
ACKs for top commit:
laanwj:
ACK 71af793512
Tree-SHA512: 9d0b975a11f66fd87a76654d210808000a629c9cce4c760f71e8a2bcb4e99b9109419f2306db67cf9b12c28e40b96ae722b7c9b4569b2b8bacd469fb99db30c3
-BEGIN VERIFY SCRIPT-
s() { contrib/devtools/copyright_header.py insert "$1"; }
s build_msvc/bitcoin_config.h
s build_msvc/msvc-autogen.py
s build_msvc/testconsensus/testconsensus.cpp
s contrib/devtools/circular-dependencies.py
s contrib/devtools/gen-manpages.sh
s contrib/filter-lcov.py
s contrib/gitian-build.py
s contrib/install_db4.sh
s src/crypto/sha256_avx2.cpp
s src/crypto/sha256_sse41.cpp
s src/fs.cpp
s src/qt/test/addressbooktests.cpp
s src/qt/test/addressbooktests.h
s src/qt/test/util.cpp
s src/qt/test/util.h
s src/qt/test/wallettests.cpp
s src/qt/test/wallettests.h
s src/test/blockchain_tests.cpp
s test/functional/combine_logs.py
s test/lint/lint-locale-dependence.sh
sed -i '1G' test/lint/lint-shebang.sh
s test/lint/lint-shebang.sh
-END VERIFY SCRIPT-
9250a087d2 Convert addrdb/addrman to new serialization (Pieter Wuille)
ca33451535 Introduce new serialization macros without casts (Pieter Wuille)
Pull request description:
This is a minimal subset of #10785 that still does *something*.
It adds a new saner serialization macro, which can be used in parallel with the old one. Then the addrdb code is converted to use this new macro.
I'll add follow-up PRs that add more functionality + converting of other modules as things get merged.
ACKs for top commit:
jamesob:
ACK 9250a087d2 ([`jamesob/ackr/17850.1.sipa.serialization_improvemen`](https://github.com/jamesob/bitcoin/tree/ackr/17850.1.sipa.serialization_improvemen))
kallewoof:
ACK 9250a087d2
laanwj:
code review ACK 9250a087d2
Tree-SHA512: d4f58c7f85d8ada7543ee43159be57d320746abe003af11395508d280d339fac7faa198e707d1a689fb0a775fc36b3945178c3ae1c0cf9ffe685773c6ddc10c1
The first argument in bin_PROGRAMS (bitcoind) was being silently
dropped and never passed into the check-security.py or check-symbols.py scripts.
This has been the case since the scripts were added to the makefile in
f3d3eaf78e.
Example of the behavior:
```python
# touch a, touch b, touch c
# python3 args.py < a b c
import sys
if __name__ == '__main__':
print(sys.argv)
# ['args.py', 'b', 'c']
# if you add some lines to "a",
# you'll see them here..
for line in sys.stdin:
print(line)
```
This flag is for logging from within CValidationInterface (see #12994).
A separate flag is desirable as the logging can be noisy and thus may
need to be disabled without affecting other logging.
- change the value returned in the RPC getaddressinfo `labels` field to an array
of label name strings
- deprecate the previous behavior of returning a JSON hash structure containing
label `name` and address `purpose` key/value pairs
- update the relevant tests
The first argument in bin_PROGRAMS (bitcoind) was being silently
dropped and never passed into the check-security.py or check-symbols.py scripts.
This has been the case since the scripts were added to the makefile in
f3d3eaf78e.
Example of the behavior:
```python
# touch a, touch b, touch c
# python3 args.py < a b c
import sys
if __name__ == '__main__':
print(sys.argv)
# ['args.py', 'b', 'c']
# if you add some lines to "a",
# you'll see them here..
for line in sys.stdin:
print(line)
```
faa92a2297 rpc: Remove mempool global from miner (MarcoFalke)
6666ef13f1 test: Properly document blockinfo size in miner_tests (MarcoFalke)
Pull request description:
The miner needs read-only access to the mempool. Instead of using the mutable global `::mempool`, keep a immutable reference to a mempool that is passed to the miner. Apart from the obvious benefits of removing a global and making things immutable, this might also simplify testing with multiple mempools.
ACKs for top commit:
promag:
ACK faa92a2297.
fjahr:
ACK faa92a2297
jnewbery:
Code review ACK faa92a2297
Tree-SHA512: c44027b5d2217a724791166f3f3112c45110ac1dbb37bdae27148a0657e0d1a1d043b0d24e49fd45465ec014224d1b7eb15c92a33069ad883fa8ffeadc24735b
4bdd68f301 Add missing typeinfo includes (Wladimir J. van der Laan)
4d88c3dcb6 net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan)
Pull request description:
Remove the forest of special exceptions based on string matching, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them.
ACKs for top commit:
MarcoFalke:
re-ACK 4bdd68f301 (only change is adding includes) 🕕
promag:
ACK 4bdd68f301, could squash.
Tree-SHA512: a005591a3202b005c75e01dfa54249db3992e2f9eefa8b3d9d435acf66130417716ed926ce4e045179cf43788f1abc7362d999750681a9c80b318373d611c366
This new approach uses a static method which takes the object as
a argument. This has the advantage that its constness can be a
template parameter, allowing a single implementation that sees the
object as const for serialization and non-const for deserialization,
without casts.
More boilerplate is included in the new macro as well.
3bd8db80d8 [validation] fix comments in CheckInputScripts() (John Newbery)
6f6465cefc scripted-diff: [validation] Rename CheckInputs to CheckInputScripts (John Newbery)
Pull request description:
CheckInputs() used to check no double spends, scripts & sigs and amounts. Since
832e074, the double spend and amount checks
have been moved to CheckTxInputs(), and CheckInputs() now just validates
input scripts. Rename the function to CheckInputScripts().
Also fix incorrect comments.
ACKs for top commit:
MarcoFalke:
re-ACK 3bd8db80d8, did the rebase myself, checked the scripted diff 👡
promag:
ACK 3bd8db80d8 :trollface:
Tree-SHA512: 7b3f8597d210492798fb784ee8ea47ea6377519111190161c7cc34a967509013f4337304f52e9bedc97b7710de7b0ff8880e08cd7f867754567f82e7b02c794c
Instead of using /16 netgroups to bucket nodes in Addrman for connection
diversification, ASN, which better represents an actor in terms
of network-layer infrastructure, is used.
For testing, asmap.raw is used. It represents a minimal
asmap needed for testing purposes.
e9fd366044 refactor: Remove null setting check in GetSetting() (Russell Yanofsky)
cba2710220 scripted-diff: Remove unused ArgsManager type flags in tests (Russell Yanofsky)
425bb30725 refactor: Add util_CheckValue test (Russell Yanofsky)
0fa54358b0 refactor: Add ArgsManager::GetSettingsList method (Russell Yanofsky)
3e185522ac refactor: Get rid of ArgsManagerHelper class (Russell Yanofsky)
dc0f148074 refactor: Replace FlagsOfKnownArg with GetArgFlags (Russell Yanofsky)
57e8b7a727 refactor: Clean up includeconf comments (Russell Yanofsky)
3f7dc9b808 refactor: Clean up long lines in settings code (Russell Yanofsky)
Pull request description:
This PR doesn't change behavior. It just implements some suggestions from #15934 and #16545 and few other small cleanups.
ACKs for top commit:
jnewbery:
Code review ACK e9fd366044
MarcoFalke:
ACK e9fd366044🚟
Tree-SHA512: 6e100d92c72f72bc39567187ab97a3547b3c06e5fcf1a1b74023358b8bca552124ca6a53c0ab53179b7f1329c03d9a73faaef6d73d2cd1a2321568a0286525e2
If after a backup, an address is issued beyond the initial
keypool range and none of the addresses in this range
is seen onchain, if a wallet is restored from backup, even in
case of rescan, funds may be loss due to the look-ahead
buffer not being incremented and so restored wallet not detecting
onchain out-of-range address as derived from its seed.
This scenario is theoretically unavoidable due to the requirement
of the keypool to have a max size. However, given the default
keypool size, this is unlikely. Document better keypool size
implications to avoid user setting a too low value.
6e77a7b65c keypool: Add comment about TopUp and when to use it (Andrew Chow)
ea50e34b28 keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow)
bb2c8ce23c keypool: Remove superfluous topup from CWallet::GetNewChangeDestination (Andrew Chow)
Pull request description:
* The `TopUp()` in `CWallet::GetNewChangeDestination` is unnecessary as currently m_spk_man calls TopUp further down the call stack inside LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination). This also lets us prepare for future changes with multiple ScriptPubKeyMans in the wallet.
* An opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::GetNewDestination` to `CWallet::GetNewDestination`.
* Another opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::ReserveKeyFromKeyPool`
Moving opportunistic TopUps ensures that ScriptPubKeyMans will always be topped up before requesting Destinations from them as we cannot always rely on future ScriptPubKeyMan implementaions topping up internally.
See also: https://github.com/bitcoin/bitcoin/pull/17373#discussion_r348598174
ACKs for top commit:
instagibbs:
utACK 6e77a7b65c only change is slight elaboration on comment
ryanofsky:
Code review ACK 6e77a7b65c. Only the comment changed since my previous review.
Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f
Remove the forest of special exceptions, and simply log a short
message to the NET logging category when an exception happens during
packet processing. It is not good to panick end users with errors
that any peer can generate (let alone writing to stderr).
78e283e656 [test] move wallet helper functions into test library (Martin Zumsande)
f613e5dfda [test] move mining helper functions into test library (Martin Zumsande)
2cb4e8bdc7 [test] move string helper functions into test library (Martin Zumsande)
Pull request description:
This disbands `test/util.h` and `test/util.cpp` and moves the content into the test utility library recently created in #17542, so that all test utility functions are in one place.
The content of the original files are split into three modules:
1) string helper functions go to `test/util/str`
2) mining helper functions go to the newly created `test/util/mining`
3) wallet helper functions go to the newly created `test/util/wallet`
ACKs for top commit:
MarcoFalke:
ACK 78e283e656🔧
Tree-SHA512: f182a61e86e76c32bcb84e37f44904d3a4a9c5a321f7a8efdda5368a6623cb8b5a5384ec4f96e67f0357b0c22099f6e3ecd0ac4cb467e3fa3f3128f8d36edfb8
7aab8d1024 [style] Code style fixups in GetWarnings() (John Newbery)
492c6dc1e7 util: change GetWarnings parameter to bool (John Newbery)
869b6314fd [qt] remove unused parameter from getWarnings() (John Newbery)
Pull request description:
`GetWarnings()` changes the format of the output warning string based on a passed-in string argument that can be set to "gui" or "statusbar".
Change the argument to a bool:
- there are only two types of behaviour, so a bool is a more natural argument type
- changing the name to `verbose` does not set any expectations for the how the calling code will use the returned string (currently, `statusbar` is used for RPC warnings, not a status bar)
- removes some error-handling code for when the passed-in string is not one of the two strings expected.
ACKs for top commit:
laanwj:
code review ACK 7aab8d1024
practicalswift:
ACK 7aab8d1024 -- diff looks correct :)
MarcoFalke:
ACK 7aab8d1024 otherwise.
promag:
Code review ACK 7aab8d1024.
Tree-SHA512: 75882c6e3e44aa9586411b803149b36ba487f4eb9cac3f5c8f07cd9f586870bba4488a51e674cf8147f05718534f482836e6a4e3f66e0d4ef6821900c7dfd04e
fa8e650b52 rest: Use mempool from node context instead of global (MarcoFalke)
fa660d65d7 node: Use mempool from node context instead of global (MarcoFalke)
facbaf092f rpc: Use mempool from node context instead of global (MarcoFalke)
Pull request description:
Currently they are identical, but in the future we might want to turn
the mempool into a unique_ptr. Replacing the global with the mempool
pointer from the node context simplifies this step.
ACKs for top commit:
jnewbery:
Code review ACK fa8e650b5
ryanofsky:
Code review ACK fa8e650b52, Only the discussed REST server changes since the last review.
Tree-SHA512: 0836f3f39cf90306455962918446e5f8612e88c32072b92afc30929aea1f17430bbda0e2b3668d36c9d6b97d63a93cf4903185194571108642b7bf5a39b89125
893aa207e8 tests: Add fuzzing harness for CheckBlock(...) and other CBlock related functions (practicalswift)
ec8dcb0199 tests: Add corpora suppression (FUZZERS_MISSING_CORPORA) for fuzzers missing in https://github.com/bitcoin-core/qa-assets/tree/master/fuzz_seed_corpus (practicalswift)
Pull request description:
Add fuzzing harness for `CheckBlock(...)` and other `CBlock` related functions.
**Testing this PR**
Run:
```
$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/block
…
# And to to quickly verify that the relevant code regions are triggered, that the
# fuzzing throughput seems reasonable, etc.
$ contrib/devtools/test_fuzzing_harnesses.sh '^block$'
```
`test_fuzzing_harnesses.sh` can be found in PR #17000.
Top commit has no ACKs.
Tree-SHA512: 275abd46d8ac970b28d8176f59124988b1e07c070173e001acd55995b830333417f301c309199fc589da08a6ac4c03aa74650d5e1638f6e3023dfbd3c9f6921d
GetWarnings() changes the format of the output warning string based on a
passed-in string argument that can be set to "gui" or "statusbar".
Change the argument to a bool:
- there are only two types of behaviour, so a bool is a more natural
argument type
- changing the name to 'verbose' does not set any expectations for the
how the calling code will use the returned string (currently,
'statusbar' is used for RPC warnings, not a status bar)
- removes some error-handling code for when the passed-in string is not
one of the two strings expected.
7d263571be rpc: require second argument only for scantxoutset start action (Andrew Chow)
Pull request description:
It was reported on [IRC](http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-11.html#l-377) that `scantxoutset`'s API was broken in 0.19.0:
```
<belcher> i think scantxoutset may have been broken in bitcoin core 0.19 ? regardless of what parameters i run it with (e.g. "scantxoutset abort", "scantxoutset status") it just returns the help doc, according to the release notes the only change was https://github.com/bitcoin/bitcoin/pull/16285/files but i dont see anything that wouldve broken it, it works fine in 0.18
<belcher> im on regtest, in case its important
<harding> I can confirm `scantxoutset abort` returns the help doc on latest master. Waiting for 0.18.1 to start now to attempt to reproduce there.
<harding> It looks like it's expecting a second parameter (even though that doesn't make sense with "abort").
<jonatack> Same for me as well
<harding> Can also confirm that `scantxoutset abort` returns the expected result on 0.18.1.
```
As noted in the conversation, previously, the second argument of `scanobjects` is only required for the `start` action. `Stop` and `abort` actions did not and could work without them.
It appears that this was broken by #16240 which enforced the size of the arguments to match the listed required arguments.
To fix this issue, this PR makes the `scanobjects` argument an optional argument. Then only in the `start` action do we check whether the `scanobjects` argument is there and throw an informative error about that. Also a test is added for this case.
ACKs for top commit:
laanwj:
ACK 7d263571be
promag:
ACK 7d263571be.
Tree-SHA512: 828bdfe47f4fffa5d00a2cf88db6cea4a2714d9c49276841ca5cbdd1603b87bb6862147b86edcf36d7b40314ddb80b1a07fd399faf288572c55cc788c5cf9526
2081442c42 test: Add test for rpc_whitelist (Emil Engler)
7414d3820c Add RPC Whitelist Feature from #12248 (Jeremy Rubin)
Pull request description:
Summary
====
This patch adds the RPC whitelisting feature requested in #12248. RPC Whitelists help enforce application policies for services being built on top of Bitcoin Core (e.g., your Lightning Node maybe shouldn't be adding new peers). The aim of this PR is not to make it advisable to connect your Bitcoin node to arbitrary services, but to reduce risk and prevent unintended access.
Using RPC Whitelists
====
The way it works is you specify (in your bitcoin.conf) configurations such as
```
rpcauth=user1:4cc74397d6e9972e5ee7671fd241$11849357f26a5be7809c68a032bc2b16ab5dcf6348ef3ed1cf30dae47b8bcc71
rpcauth=user2:181b4a25317bff60f3749adee7d6bca0$d9c331474f1322975fa170a2ffbcb176ba11644211746b27c1d317f265dd4ada
rpcauth=user3:a6c8a511b53b1edcf69c36984985e$13cfba0e626db19061c9d61fa58e712d0319c11db97ad845fa84517f454f6675
rpcwhitelist=user1:getnetworkinfo
rpcwhitelist=user2:getnetworkinfo,getwalletinfo, getbestblockhash
rpcwhitelistdefault=0
```
Now user1 can only call getnetworkinfo, user2 can only call getnetworkinfo or getwalletinfo, while user3 can still call all RPCs.
If any rpcwhitelist is set, act as if all users are subject to whitelists unless rpcwhitelistdefault is set to 0. If rpcwhitelistdefault is set to 1 and no rpcwhitelist is set, act as if all users are subject to whitelists.
Review Request
=====
In addition to normal review, would love specific review from someone working on LN (e.g., @ roasbeef) and someone working on an infrastructure team at an exchange (e.g., @ jimpo) to check that this works well with their system.
Notes
=====
The rpc list is spelling sensitive -- whitespace is stripped though. Spelling errors fail towards the RPC call being blocked, which is safer.
It was unclear to me if HTTPReq_JSONRPC is the best function to patch this functionality into, or if it would be better to place it in exec or somewhere else.
It was also unclear to me if it would be preferred to cache the whitelists on startup or parse them on every RPC as is done with multiUserAuthorized. I opted for the cached approach as I thought it was a bit cleaner.
Future Work
=====
In a future PR, I would like to add an inheritance scheme. This seemed more controversial so I didn't want to include that here. Inheritance semantics are tricky, but it would also make these whitelists easier to read.
It also might be good to add a `getrpcwhitelist` command to facilitate permission discovery.
Tests
=====
Thanks to @ emilengler for adding tests for this feature. The tests cover all cases except for where `rpcwhitelistdefault=1` is used, given difficulties around testing with the current test framework.
ACKs for top commit:
laanwj:
ACK 2081442c42
Tree-SHA512: 0dc1ac6a6f2f4b0be9c9054d495dd17752fe7b3589aeab2c6ac4e1f91cf4e7e355deedcb5d76d707cbb5a949c2f989c871b74d6bf129351f429569a701adbcbf
This separates out some logic for detecting how full the coins cache is from
FlushStateToDisk. We'll want to reuse this logic when deciding when to flush
the coins cache during UTXO snapshot activation.
034561f9cd cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice (Harris)
Pull request description:
This PR fixes#17679 by replacing BlockFilterType-vector with a set of the same type to make sure that only unique filter types get inserted.
ACKs for top commit:
MarcoFalke:
ACK 034561f9cd📖
laanwj:
ACK 034561f9cd
fanquake:
ACK 034561f9cd - Tested with `src/bitcoind --blockfilterindex=basic --blockfilterindex=basic`
Tree-SHA512: 64ccec4d23528abfbb564f2b41fb846137875260ce06ea461da12175819985964a1a7442788d5ff7282b5de0c5fd46524d9a793788ee3b876626cbdf05b28c16
7cecf10ac3 Replace LegacyScriptPubKeyMan::IsCrypted with LegacyScriptPubKeyMan::HasEncryptionKeys (Andrew Chow)
bf6417142f Remove SetCrypted() and fUseCrypto; Change IsCrypted()'s implementation (Andrew Chow)
77a777118e Rename EncryptKeys to Encrypt and pass in the encrypted batch to use (Andrew Chow)
35f962fcf0 Clear mapKeys before encrypting (Andrew Chow)
14b5efd66f Move fDecryptionThoroughlyChecked from CWallet to LegacyScriptPubKeyMan (Andrew Chow)
97c0374a46 Move Unlock implementation to LegacyScriptPubKeyMan (Andrew Chow)
e576b135d6 Replace LegacyScriptPubKeyMan::vMasterKey with GetDecryptionKey() (Andrew Chow)
fd9d6eebc1 Add GetEncryptionKey() and HasEncryptionKeys() to WalletStorage (Andrew Chow)
Pull request description:
Let wallet class handle locked/unlocked status and master key, and let keyman
handle encrypting its data and determining whether there is encrypted data.
There should be no change in behavior, but state is tracked differently. The
fUseCrypto atomic bool is eliminated and replaced with equivalent
HasEncryptionKeys checks.
Split from #17261
ACKs for top commit:
laanwj:
ACK 7cecf10ac3
Tree-SHA512: 95a997c366ca539abba0c0a7a0015f39d27b55220683d8d86344ff2d926db4724da67700d2c8ec2d82ed75d07404318c6cb81544af8aadeefab312167257e673
4341bffb6e GUI: Refactor formatServicesStr to warn when a ServicesFlag is missing (Luke Dashjr)
df77de8c21 Bugfix: GUI: Recognise NETWORK_LIMITED in formatServicesStr (Luke Dashjr)
Pull request description:
Currently, only the bottom 8 service bits are shown in the GUI peer details view.
`NODE_NETWORK_LIMITED` is the 11th bit (2^10).
The first commit expands the range to cover the full 64 bits, and properly label `"NETWORK_LIMITED"`.
The second commit refactors the code so that any future omitted service bits will trigger a compile warning.
ACKs for top commit:
jonasschnelli:
utACK 4341bffb6e
jonasschnelli:
Tested ACK 4341bffb6e
hebasto:
Concept ACK 4341bffb6e
Tree-SHA512: 8338737d03fbcd92024159aabd7e632d46e13c72436d935b504d2bf7ee92b7d124e89a5917bf64d51c87f12a64de703270c2d7b4c6711fa8ed08ea7887d817c7
Before this change, the coins simulation test uses a base view of type
CCoinsViewTest, which has no relevance outside of the unittest suite. Might as
well reuse this testcase with a more realistic configuration that has
CCoinsViewDB at the bottom of the view structure.
wallet/walletutil.cpp:77:23: error: no member named 'level' in 'boost::filesystem::recursive_directory_iterator'
} else if (it.level() == 0 && it->symlink_status().type() == fs::regular_file && IsBerkeleyBtree(it...
~~ ^
In decodepsbt if an invalid amount is seen, don't calculate the fee
but still show the invalid value in the decode.
In analyze psbt, if an invalid amount is seen, set the next step to
be the creator as the creator needs to remake the transaction so that
it is valid.
1bb5d517aa test: add unit test for non-standard bare multisig txs (Sebastian Falbesoner)
Pull request description:
Approaches another missing unit test of issue #17394: Checks that the function `IsStandardTx()` returns rejection reason `"bare-multisig"` if any one of the outputs' scriptPubKey has bare multisignature format (i.e. `M <PubKey1> <PubKey2> ... <PubKeyN> N OP_CHECKSIG`, not P2SH!) and the policy flag `fIsBareMultisigStd` is set to false.
ACKs for top commit:
instagibbs:
utACK 1bb5d517aa
Tree-SHA512: d7c95e35da16520d6dcd2b4278e2426fedd13f68d1f23c90e85e929774e123fbfcfbccc26df6ad1c0dd61780896fa4b4b3d4e8280c647bb06df2bfcf2ba572fb
773d4572a4 Mark PSBTs spending unspendable outputs as invalid in analysis (Andrew Chow)
638e40cb60 Have a PSBTAnalysis state that indicates invalid PSBT (Andrew Chow)
Pull request description:
When analyzing an unspendable PSBT, report that it is unspendable and exit analysis early.
ACKs for top commit:
Sjors:
ACK 773d457
instagibbs:
After some thought ACK 773d4572a4
Tree-SHA512: 99b0cb2fa1ea37593fc65a20effe881639d69ddeeecf5197bc87bc7f2220cbeb40f1d429d517e4d27f2e9fb563a00cd845d2b4b1ce05246a75a6cb56fb9b0ba5
`#pragma pack(1)` prevents aligning the struct and its members to their
required alignment. This can result in code that performs non-aligned
reads and writes to integers and pointers, which is problematic on some
architectures.
It also triggers UBsan — see
https://github.com/bitcoin/bitcoin/pull/17156#issuecomment-543123631
and #17510.
597d10ceb9 tests: Add fuzzing harness for various functions consuming only integrals (practicalswift)
575383b3e1 tests: Add corpora suppression (FUZZERS_MISSING_CORPORA) for fuzzers missing in https://github.com/bitcoin-core/qa-assets/tree/master/fuzz_seed_corpus (practicalswift)
Pull request description:
Add fuzzing harness for various functions consuming only integrals.
**Testing this PR**
Run:
```
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
--with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/integer
```
Top commit has no ACKs.
Tree-SHA512: f0ccbd63671636f8e661385b682e16ad287fef8f92e7f91327ee2093afc36fcd424e1646fe90279388e28a760bcc795766eb80cf6375e0f873efff37fc7e2393
d5766f223f tests: Add corpora suppression (FUZZERS_MISSING_CORPORA) for fuzzers missing in https://github.com/bitcoin-core/qa-assets/tree/master/fuzz_seed_corpus (practicalswift)
e75ecb91c7 tests: Add fuzzing harness for various CTxOut related functions (practicalswift)
ce935292c0 tests: Add fuzzing harness for various CTxIn related functions (practicalswift)
Pull request description:
Add fuzzing harness for various `CTx{In,Out}` related functions.
**Testing this PR**
Run:
```
$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/tx_in
…
$ src/test/fuzz/tx_out
…
# And to to quickly verify that the relevant code regions are triggered, that the
# fuzzing throughput seems reasonable, etc.
$ contrib/devtools/test_fuzzing_harnesses.sh '^tx_'
```
`test_fuzzing_harnesses.sh` can be found in PR #17000.
Top commit has no ACKs.
Tree-SHA512: f1374307a2581ebc3968d012ea2438061bbb84ece068e584fae9750669a6cd003723dde14db88e77c9579281ecd4eaa2a7ff0614f253d8c075e6dd16dd2e68d5
709afb2a7d tests: Test serialisation as part of deserialisation fuzzing. Test round-trip equality where possible. Avoid code repetition. (practicalswift)
Pull request description:
Test serialisation as part of deserialisation fuzzing. Test round-trip equality where possible.
ACKs for top commit:
MarcoFalke:
ACK 709afb2a7d🍲
Tree-SHA512: b8c9c24538ee516607608ac685d2e9b01eca5c15213def3fd096b16516db84bfd45516fbee43e25b28cb3481a5d4ec3f7a34713e2da35b2902081ed42b85224d
978b25528c util: Update tinyformat to upstream (Wladimir J. van der Laan)
Pull request description:
Last update was in 2017.
Updates tinyformat to upstream commit c42f/tinyformat@705e3f4e1d.
Re-apply (and mark) bitcoin core specific changes.
No changes that affect our use, as far as I can see, but this gets rid of the gcc `-Wimplicit-fallthrough` warnings, at least.
ACKs for top commit:
MarcoFalke:
ACK 978b25528c, extracted our patches based on the last update, did the update to v2.3.0 myself and re-applied the patches. Only diff is NULL/nullptr and explicit 🔝
Tree-SHA512: 2ba09e1095878d088520f379d545b40c7286ef199ecbbc17fdd5c85bca447d9b0c7a1829d4038bb6d432cd1ff92ad7bba75c0f2f96c71aeb6fa6031002f1ea1d
48a5c92f9e ui: disable 3rd-party tx-urls when wallet disabled (Harris)
Pull request description:
This PR closes#17683 by removing 3rd-party Url-Label and -TextBox from Display Options in wallet-disabled mode.
ACKs for top commit:
laanwj:
Code review ACK 48a5c92f9e
fanquake:
ACK 48a5c92f9e - tested with and without wallet (compiled out and `-disablewallet`).
Tree-SHA512: 3cc89825409fc0a3eec501c4dab5ff1caaa4ce410746a4b6ab200222fff986f4483eab90cda53a98a144be6acf1b6ca8650ab18242c39446f3335b3a9a537066
There was an issue around the time of Qt 4.6 when placeholder text was
introduced, that caused a compile failure when it was specified in the
form.
As a workaround the placeholder texts were moved to the code.
Qt 4 hasn't been relevant to us for ages. So move all (non-parametrized)
placeholder texts to the form files instead.
It's better to keep this kind of text content together. Makes sure
translate/no-translate status is kept as it is.
Removes SetCrypted() and fUseCrypto as we don't need them anymore.
SetCrypted calls in LegacyScriptPubKeyMan are replaced with mapKeys.empty()
IsCrypted() is changed to just call HasEncryptionKeys()
886f1731be Key pool: Fix omitted pre-split count in GetKeyPoolSize (Andrew Chow)
386a994b85 Key pool: Change ReturnDestination interface to take address instead of key (Andrew Chow)
ba41aa4969 Key pool: Move LearnRelated and GetDestination calls (Andrew Chow)
65833a7407 Add OutputType and CPubKey parameters to KeepDestination (Andrew Chow)
9fcf8ce7ae Rename Keep/ReturnKey to Keep/ReturnDestination and remove the wrapper (Andrew Chow)
596f6460f9 Key pool: Move CanGetAddresses call (Andrew Chow)
Pull request description:
* The `pwallet->CanGetAddresses()` call in `ReserveDestination::GetReservedDestination` to `LegacyScriptPubKeyMan::GetReservedDestination` so that the sanity check results in a failure when a `ScriptPubKeyMan` individually cannot get a destination, not when any of the `ScriptPubKeyMan`s can't.
* `ScriptPubKeyMan::GetReservedDestination` is changed to return the destination so that future `ScriptPubKeyMan`s can return destinations constructed in other ways. This is implemented for `LegacyScriptPubKeyMan` by moving key-to-destination code from `CWallet` to `LegacyScriptPubKeyMan`
* In order for `ScriptPubKeyMan` to be generic and work with future `ScriptPubKeyMan`s, `ScriptPubKeyMan::ReturnDestination` is changed to take a `CTxDestination` instead of a `CPubKey`. Since `LegacyScriptPubKeyMan` still deals with keys internally, a new map `m_reserved_key_to_index` is added in order to track the keypool indexes that have been reserved.
* A bug is fixed in how the total keypool size is calculated as it was omitting `set_pre_split_keypool` which is a bug.
Split from #17261
ACKs for top commit:
ryanofsky:
Code review ACK 886f1731be. Only change is moving earlier fix to a better commit (same end result).
promag:
Code review ACK 886f1731be.
instagibbs:
code review re-ACK 886f1731be
Sjors:
Code review re-ACK 886f1731be
Tree-SHA512: f4be290759f63fdc920d5c02bd0d09acc4b06a5f053787d4afcd3c921b2e35d2bd97617fadae015da853dc189f559fb8d2c6e58d53e4cabfac9af151cd97ad19
d8daa8f371 pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)
Pull request description:
Assert `CPubKey`'s `ECCVerifyHandle` precondition.
This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.
Related PR #17274.
ACKs for top commit:
sipa:
ACK d8daa8f371
Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
Last update was in 2017.
Updates tinyformat to upstream commit 705e3f4e1de922069bf715746d35bd2364b1f98f.
Re-apply bitcoin core specific changes.
No changes that affect our use, as far as I can see, but this gets rid
of the gcc `-Wimplicit-fallthrough` warnings, at least.
Currently they are identical, but in the future we might want to turn
the mempool into a unique_ptr. Replacing the global with the mempool
pointer from the node context simplifies this step.
55b2cb199c random: mark RandAddPeriodic and SeedPeriodic as noexcept (fanquake)
461e547877 doc: correct random.h docs after #17270 (fanquake)
Pull request description:
The usage of `MilliSleep()` in SeedPeriodic (previously SeedSleep) was
[removed](d61f2bb076) in #17270, meaning it, and its users can now be marked `noexcept`.
This also corrects the docs in random.h for some of the changes in #17270.
ACKs for top commit:
practicalswift:
ACK 55b2cb199c
laanwj:
ACK 55b2cb199c
sipa:
ACK 55b2cb199c
Tree-SHA512: 672d369796e7c4f9b4d98dc545e5454999fa1bef373871994a26041d6163c58909e2255e4f820d3ef011679aa3392754eb57477306a89f5fd3d57e2bd7f0811a
fa40e48c50 ci: Remove unparseable lines from supp file for old xenial clang tsan (MarcoFalke)
fa1bfc476c ci: ubsan report_error_type=1 and add suppressions (MarcoFalke)
fa69cef13e test: Print stderr when subprocess fails (MarcoFalke)
2222c30586 test: Use char instead of unsigned char (MarcoFalke)
faa8023ce9 ci: Bump to clang-8 for asan build to avoid segfaults on ppc64le (MarcoFalke)
Pull request description:
Use clang-8 instead of default clang (which is clang-6 on Bionic) to avoid spurious segfaults when running the ci system on ppc64le
ACKs for top commit:
practicalswift:
ACK fa40e48c50 assuming Travis is happy -- diff looks correct :)
Tree-SHA512: f4f26232d3a0ef38da245869340f723d279a3db9823befbc735fb5a00024dae041c7306d7ae55d2488e6f86aa96cdea155b007aefb561fba505141e8dbc717dc
02d8c56a18 Seed RNG with precision timestamps on receipt of net messages. (Matt Corallo)
Pull request description:
See title. Exposes a generic dead-simple "SeedEvent" interface, but currently just used for net messages.
ACKs for top commit:
sipa:
utACK 02d8c56a18
laanwj:
ACK 02d8c56a18
meshcollider:
utACK 02d8c56a18
Tree-SHA512: 28eb39a201ee2b13393c5c64dbf7c1913f3482f095969ef5141bfe549ce77dd63bb5f14738f6eedb296c686ea36014aa157b9c5e8059710a318590f30e9caa14
e7ad4a2f8c doc: rename wallet-tool references to bitcoin-wallet (Wilson Ccasihue S)
Pull request description:
Fix. text reference to executable bitcoin-wallet instead of wallet-tool, there is not a wallet-tool at bin/ folder.
ACKs for top commit:
fanquake:
ACK e7ad4a2f8c - thanks for following up.
Tree-SHA512: aed41b08947728a4ff3a97a62858ee7c86e2e5d57dcbbd0aab492dae3d8a548bb60541924e68cf3a0aa3d53d7db0012b489462b466919cd83f05b2aa88b7fff7
In accordance with #17314, Removing noisy fields from -getinfo. Fields removed: protocolversion, walletversion and keypoololdest. In addition to changing bitcoin-cli -getinfo, there is another change to test/functional/interface_bitcoin_cli.py. This change deletes tests that utilize removed -getinfo calls.
76303f65f9 test: add unit test for non-standard txs with wrong nVersion (Dominik Spicher)
Pull request description:
Takes care of one of the missing cases of #17394: nVersion must be within the allowed range.
ACKs for top commit:
instagibbs:
ACK 76303f65f9
Tree-SHA512: 94464f781cf70a5616f7cea2014ae0a97a338c34411cc989c60389de2ce00368374811db78c919bda30e0ebf341fb830998a5e97c124dd8afc8feb726cedfd3a
70ed2ab7ef Add unit test for DB creation with unicode path (Aaron Clauson)
Pull request description:
An issue arose when attempting to switch back to the main repo version of leveldb when the bitcoin data directory uses a unicode path. The leveldb windows file IO wrapper was using the *A ANSI win32 calls instead of the Unicode *W ones. This unit test will catch if the path created by leveldb doesn't match what we're expecting. For more info see https://github.com/google/leveldb/issues/755.
ACKs for top commit:
laanwj:
ACK 70ed2ab7ef
Tree-SHA512: fc6dbd3aa26a439016e63e8d4d931f218ce99094fc7887a13b54562ad4133047020288ecbcd622a8309f422ee1eda5df50bcb8c8e44442af36ed57b22c069004
02afb0c550 Fix origfee return for bumpfee with feerate arg (Gregory Sanders)
Pull request description:
fixes https://github.com/bitcoin/bitcoin/issues/17642 and adds a simple test that would have caught it
ACKs for top commit:
achow101:
ACK 02afb0c550
Tree-SHA512: 303e392e05407f204dffe360689b5bb5dc77fd462dd0e489bc0b6c8f94f89ab7fe2bd8cb47e4dc6dc5c23a619826d15f3bf6b02b2c8e96402fbb51953c462e2d
In order for ScriptPubKeyMan to be generic and work with future
ScriptPubKeyMans, ScriptPubKeyMan::ReturnDestination is changed to
take a CTxDestination instead of a CPubKey. Since LegacyScriptPubKeyMan
still deals with keys internally, a new map m_reserved_key_to_index is
added in order to track the keypool indexes that have been reserved.
The CPubKey argument of KeepDestination is also removed so that it is
more generic. Instead of taking a CPubKey or a CTxDestination, we just use
the nIndex given to find the pubkey.
Addresses are determined by LegacyScriptPubKeyMan::GetReservedDestination
instead of ReserveDestination::GetReservedDestination as other ScriptPubKeyMan
implementations may construct addresses differently
This does not change behavior.
98fbd1cdff Use correct C++11 header for std::swap() (Hennadii Stepanov)
b66861e2e5 Fix comparison function signature (Hennadii Stepanov)
Pull request description:
This PR fixes build on CentOS 7 with GCC 4.8.5:
```
...
In file included from /usr/include/c++/4.8.2/algorithm:62:0,
from ./serialize.h:11,
from ./qt/sendcoinsrecipient.h:13,
from ./qt/recentrequeststablemodel.h:8,
from qt/recentrequeststablemodel.cpp:5:
/usr/include/c++/4.8.2/bits/stl_algo.h: In instantiation of ‘_RandomAccessIterator std::__unguarded_partition(_RandomAccessIterator, _RandomAccessIterator, const _Tp&, _Compare) [with _RandomAccessIterator = QList<RecentRequestEntry>::iterator; _Tp = RecentRequestEntry; _Compare = RecentRequestEntryLessThan]’:
/usr/include/c++/4.8.2/bits/stl_algo.h:2296:78: required from ‘_RandomAccessIterator std::__unguarded_partition_pivot(_RandomAccessIterator, _RandomAccessIterator, _Compare) [with _RandomAccessIterator = QList<RecentRequestEntry>::iterator; _Compare = RecentRequestEntryLessThan]’
/usr/include/c++/4.8.2/bits/stl_algo.h:2337:62: required from ‘void std::__introsort_loop(_RandomAccessIterator, _RandomAccessIterator, _Size, _Compare) [with _RandomAccessIterator = QList<RecentRequestEntry>::iterator; _Size = int; _Compare = RecentRequestEntryLessThan]’
/usr/include/c++/4.8.2/bits/stl_algo.h:5499:44: required from ‘void std::sort(_RAIter, _RAIter, _Compare) [with _RAIter = QList<RecentRequestEntry>::iterator; _Compare = RecentRequestEntryLessThan]’
qt/recentrequeststablemodel.cpp:208:82: required from here
/usr/include/c++/4.8.2/bits/stl_algo.h:2263:35: error: no match for call to ‘(RecentRequestEntryLessThan) (RecentRequestEntry&, const RecentRequestEntry&)’
while (__comp(*__first, __pivot))
^
In file included from qt/recentrequeststablemodel.cpp:5:0:
./qt/recentrequeststablemodel.h:43:7: note: candidate is:
class RecentRequestEntryLessThan
^
qt/recentrequeststablemodel.cpp:217:6: note: bool RecentRequestEntryLessThan::operator()(RecentRequestEntry&, RecentRequestEntry&) const
bool RecentRequestEntryLessThan::operator()(RecentRequestEntry &left, RecentRequestEntry &right) const
^
qt/recentrequeststablemodel.cpp:217:6: note: no known conversion for argument 2 from ‘const RecentRequestEntry’ to ‘RecentRequestEntry&’
In file included from /usr/include/c++/4.8.2/algorithm:62:0,
from ./serialize.h:11,
from ./qt/sendcoinsrecipient.h:13,
from ./qt/recentrequeststablemodel.h:8,
from qt/recentrequeststablemodel.cpp:5:
/usr/include/c++/4.8.2/bits/stl_algo.h:2266:34: error: no match for call to ‘(RecentRequestEntryLessThan) (const RecentRequestEntry&, RecentRequestEntry&)’
while (__comp(__pivot, *__last))
^
In file included from qt/recentrequeststablemodel.cpp:5:0:
./qt/recentrequeststablemodel.h:43:7: note: candidate is:
class RecentRequestEntryLessThan
^
qt/recentrequeststablemodel.cpp:217:6: note: bool RecentRequestEntryLessThan::operator()(RecentRequestEntry&, RecentRequestEntry&) const
bool RecentRequestEntryLessThan::operator()(RecentRequestEntry &left, RecentRequestEntry &right) const
^
qt/recentrequeststablemodel.cpp:217:6: note: no known conversion for argument 1 from ‘const RecentRequestEntry’ to ‘RecentRequestEntry&’
CXX qt/qt_libbitcoinqt_a-sendcoinsentry.o
make[2]: *** [qt/qt_libbitcoinqt_a-recentrequeststablemodel.o] Error 1
```
Also for `std::swap()` header `<algorithm>` is replaced with `<utility>` one.
Refs:
- [`std::swap()`](https://en.cppreference.com/w/cpp/algorithm/swap)
- [standard library header `<utility>`](https://en.cppreference.com/w/cpp/header/utility)
ACKs for top commit:
promag:
Code review ACK 98fbd1cdff.
jonasschnelli:
utACK 98fbd1cdff
fanquake:
ACK 98fbd1cdff
Tree-SHA512: 91324490c1bdb98f186d233418e7e72ae7bee507876e94fb8c038bee031cea9e1046900f21156da4b7c33abcd726796867b124c4132d9ae3759877e90a8527db
eadd1304c8 tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs (Andrew Chow)
ff330badd4 Default to bnb_used = false as there are many cases where BnB is not used (Andrew Chow)
Pull request description:
#17290 introduced a bug where, when we had preset inputs that covered the amount being sent and subtractFeeFrromOutputs was being used, transaction funding would result in a `Fee exceeds maximum configured by -maxtxfee` error. This was happening because we weren't setting `bnb_used = false` when the preset inputs were used as it should have been. This resulted in a too high fee because the change would go to fees accidentally.
Apparently this particular case doesn't have a test, so I've added one as well.
ACKs for top commit:
Sjors:
ACK eadd130. I can't get this new test to fail on macOS (without this PR). It passes whether or not I compile with `--enable-debug`. It does fail on Ubuntu. Yay undefined behavior... Anyway, it's a useful test.
fanquake:
ACK eadd1304c8
instagibbs:
utACK eadd1304c8
Tree-SHA512: 7286c321f78666eea558cc591174630d210263594df41cab1065417510591ee514ade0e1d0cec8af09a785757da68de82592b013e8fe8d4966cec3254368706e
An issue arose when attempting to switch back to the main repo version of leveldb when the bitcoin data directory uses a unicode path. The leveldb windows file IO wrapper was using the *A ANSI win32 calls instead of the Unicode *W ones. This unit test will catch if the path created by leveldb doesn't match what we're expecting. For more info see https://github.com/google/leveldb/issues/755.
4a96e459d7 [gui] send: show watch-only balance in send screen (Sjors Provoost)
2689c8fd71 [test] qt: add send screen balance test (Sjors Provoost)
Pull request description:
Now that we can create a PSBT from a watch-only wallet (#16944), we should also display the watch-only balance on the send screen.
Before:
<img width="1008" alt="before" src="https://user-images.githubusercontent.com/10217/69533384-030e9180-0f78-11ea-9748-c32c957e822e.png">
After:
<img width="1009" alt="Schermafbeelding 2019-11-26 om 11 44 17" src="https://user-images.githubusercontent.com/10217/69622879-19811f80-1042-11ea-8279-091012f39b38.png">
I added a test to check the balance on the send screen, but it only covers regular wallets. A better would add a watch-only only wallet.
ACKs for top commit:
meshcollider:
utACK 4a96e459d7
jb55:
utACK 4a96e459d7
promag:
reACK 4a96e45, rebased and label change since last review.
instagibbs:
code review and light test ACK 4a96e459d7
Tree-SHA512: 4213549888bd309f72bdbba1453218f4a2b07e809100d786a3791897c75468f9092b06fe4b971942b1c228aa75ee7c04971f262ca9a478b42756e056eb534620
73b96c94cb net: Fix uninitialized read in ProcessMessage(...) (practicalswift)
Pull request description:
Fix an uninitialized read in `ProcessMessage(…, "tx", …)` when receiving a transaction we already have.
The uninitialized value is read and used on [L2526 in the case of `AlreadyHave(inv) == true`](d8a66626d6/src/net_processing.cpp (L2494-L2526)).
Proof of concept being run against a `bitcoind` built with MemorySanitizer (`-fsanitize=memory`):
```
$ ./p2p-uninit-read-in-conditional-poc.py
Usage: ./p2p-uninit-read-in-conditional-poc.py <dstaddr> <dstport> <net>
$ bitcoind -regtest &
$ ./p2p-uninit-read-in-conditional-poc.py 127.0.0.1 18444 regtest
SUMMARY: MemorySanitizer: use-of-uninitialized-value
[1]+ Exit 77 bitcoind -regtest
$
```
Proof of concept being run against a `bitcoind` running under Valgrind (`valgrind --exit-on-first-error`):
```
$ valgrind -q --exit-on-first-error=yes --error-exitcode=1 bitcoind -regtest &
$ ./p2p-uninit-read-in-conditional-poc.py 127.0.0.1 18444 regtest
==27351== Conditional jump or move depends on uninitialised value(s)
[1]+ Exit 1 valgrind -q --exit-on-first-error=yes --error-exitcode=1 bitcoind -regtest
$
```
Proof of concept script:
```
#!/usr/bin/env python3
import sys
from test_framework.mininode import NetworkThread
from test_framework.mininode import P2PDataStore
from test_framework.messages import CTransaction, CTxIn, CTxOut, msg_tx
def send_duplicate_tx(dstaddr="127.0.0.1", dstport=18444, net="regtest"):
network_thread = NetworkThread()
network_thread.start()
node = P2PDataStore()
node.peer_connect(dstaddr=dstaddr, dstport=dstport, net=net)()
node.wait_for_verack()
tx = CTransaction()
tx.vin.append(CTxIn())
tx.vout.append(CTxOut())
node.send_message(msg_tx(tx))
node.send_message(msg_tx(tx))
node.peer_disconnect()
network_thread.close()
if __name__ == "__main__":
if len(sys.argv) != 4:
print("Usage: {} <dstaddr> <dstport> <net>".format(sys.argv[0]))
sys.exit(0)
send_duplicate_tx(sys.argv[1], int(sys.argv[2]), sys.argv[3])
```
Note that the transaction in the proof of concept is the simplest possible, but really any transaction can be used. It does not have to be a valid transaction.
This bug was introduced in #15921 ("validation: Tidy up ValidationState interface") which was merged in to `master` 28 days ago.
Luckily this bug was caught before being part of any Bitcoin Core release :)
ACKs for top commit:
jnewbery:
utACK 73b96c94cb
laanwj:
ACK 73b96c94cb, thanks for discovering and reporting this before it ended up in a release.
Tree-SHA512: 7ce6b8f260bcdd9b2ec4ff4b941a891bbef578acf4456df33b7a8d42b248237ec4949e65e2445b24851d1639b10681c701ad500b1c0b776ff050ef8c3812c795
d2a3a5cadb util: make ScheduleBatchPriority advisory only (fanquake)
Pull request description:
ACKs for top commit:
laanwj:
ACK d2a3a5cadb
Tree-SHA512: 14e44360bc6b0c0bfd794cb8a744af7d64fb01aa5602fdb392d6c54799a721ef04426e8379b157dd40f2a33c0b6a5248b09d59c865c453ff1f6e3abbafff524e
33f5fc32e5 test: add rpc getaddressinfo labels test coverage (Jon Atack)
0f3539ac6d test: add listlabels test in wallet_labels.py (Jon Atack)
1388de8390 rpc: add getaddressinfo code documentation (Jon Atack)
2ee0cb3330 rpc: update getaddressinfo RPCExamples to bech32 (Jon Atack)
8d1ed0c263 rpc: clarify label vs labels in getaddressinfo RPCHelpman (Jon Atack)
5a0ed85070 rpc: improve getaddressinfo RPCHelpman content (Jon Atack)
70cda342cd rpc: improve getaddressinfo RPCHelpman formatting (Jon Atack)
Pull request description:
This PR is a continuation of the work in https://github.com/bitcoin/bitcoin/pull/12892.
Main motivations:
- There is currently no test coverage for the getaddressinfo `labels` response. Coverage here is a prerequisite before deprecating the `label` response or adding multiple labels per address.
- `bitcoin-cli help getaddressinfo` returns a few content errors, difficult-to-read formatting, and no explanation why it returns both `label` and `labels` and how they relate, which can be confusing for application developers.
Changes by order of commits:
- [x] improve/fix getaddressinfo RPCHelpman layout formatting
- [x] improve/fix getaddressinfo RPCHelpman content
- [x] clarify the `label` and `labels` fields in getaddressinfo RPCHelpman
- [x] update getaddressinfo RPCExamples addresses to bech32
- [x] add getaddressinfo code docs
- [x] add a `listlabels` test assertion in wallet_labels.py
- [x] add missing getaddressinfo `labels` test coverage and improve the existing `label` tests
Here are gists of the CLI help output:
[`bitcoin-cli help getaddressinfo` before this PR](https://gist.github.com/jonatack/022af5221a85c069780359a22643c810)
[`bitcoin-cli help getaddressinfo` after this PR](https://gist.github.com/jonatack/4ee5f6abc62a3d99269570206a5f90ba)
It seems we ought to begin a deprecation process for the getaddressinfo `label` field? If yes, I have a follow-up ready. _--> EDIT: Deprecation follow-ups #17578 and #17585 now build on this PR._
ACKs for top commit:
fjahr:
Re-ACK 33f5fc32e5
jnewbery:
ACK 33f5fc32e5.
Tree-SHA512: a001aa863090ec2566a31059477945b1c303ebeb430b33472f8b150e420fa5742fc33bca9d95571746395b607f43f6078dd5b53e238ac1f3fc648b51c8f79a07
27d82b63fb gui: remove macOS start on login code (fanquake)
Pull request description:
The macOS startup item code was disabled for builds targeting macOS >
`10.11` in #15208. Now that we require macOS `10.12` as a minimum (#17550),
we can remove the startup item code entirely. The API we were using, `LSSharedFileListItemCopyResolvedURL`, `LSSharedFileListCopySnapshot` etc,
was removed in macOS `10.12` SDK.
ACKs for top commit:
jonasschnelli:
utACK 27d82b63fb
jonasschnelli:
Tested ACK 27d82b63fb - successfully compiled on 10.15.1
Tree-SHA512: 7420757b91c7820e6a63280887155394547134a9cebcf3721af0284da23292627f94cd431241e033075b3fd86d79ace3ebf1b25d17763acbf71e07a742395409
93352d261f qt: Use proper class for Ui::ReceiveCoinsDialog (Hennadii Stepanov)
8781904643 qt: Fix class name of Ui::ModalOverlay (Hennadii Stepanov)
Pull request description:
Use proper classes for:
- `Ui::ModalOverlay` to remove `<customwidget>` entry
- `Ui::ReceiveCoinsDialog` to be consistent with the code base
This PR does not change behavior.
ACKs for top commit:
jonasschnelli:
Tested ACK 93352d261f - ran this on top of master and tested the modal overlay on initial mainnet sync.
laanwj:
code review ACK 93352d261f
Tree-SHA512: faeed8e86dbf5355505defcdb7e1db07d6a6005ee5eb07367b00f6aa122dd8ad34f8372d4bae7b29c0eac87b538a33157e19328be2876135e8a6376a3197f1bc
f93fc61c65 Put bounds on the number of CPUID leaves explored (Pieter Wuille)
ba2c5fe147 Fix CPUID subleaf iteration (Pieter Wuille)
Pull request description:
This fixes#17523.
The code to determine which CPUID subleaves to explore was incorrect in #17270. The new code here is based on Intel's reference documentation for CPUID (a document called "Intel® Processor Identification and the CPUID Instruction - Application Note 485", which I cannot actually find on their own website).
ACKs for top commit:
laanwj:
ACK f93fc61c65
jonatack:
ACK f93fc61c65 code review, tested rebased on current master bb862d7 with Debian 4.19 x86_64
mzumsande:
ACK f93fc61, reviewed code and compared with the intel doc, tested on an AMD and an Intel processor.
Tree-SHA512: 2790b326fa397b736c0f39f25807bea57de2752fdd58bf6693d044b8cb26df36c11cce165a334b471f8e33724f10e3b76edab5cc4e0e7776601aabda13277245
An opportunistic TopUp is moved from LegacyScriptPubKeyMan::GetNewDestination
to CWallet::GetNewDestination. Another opportunistic TopUp is moved from
LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination)
to ReserveDestination::GetReservedDestination.
Moving opportunistic TopUps ensures that ScriptPubKeyMans will always
be topped up before requesting Destinations from them as we cannot
always rely on future ScriptPubKeyMan implementaions topping up internally.
As such, it is also unnecessary to keep the TopUp calls in the
LegacyScriptPubKeyMan functions so they are moved.
This does not change behavior as TopUp calls are moved up the call stack.
This does not change behavior. This TopUp() is unnecessary as currently
m_spk_man calls TopUp further down the call stack inside
LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination)
By removing this here, we also prepare for future changes where CWallet
has multiple ScriptPubKeyMans instead of m_spk_man.
Call LegacyScriptPubKeyMan::CanGetAddresses directly instead of calling
CWallet::CanGetAddresses to only query the relevant key manager
This is a minor change in behavior: call now only happens if a new key needs to
be reserved, since if a key is already reserved it might fail unnecessarily.
This change also serves as a sanity check
https://github.com/bitcoin/bitcoin/pull/16341#discussion_r331238394
The macOS startup item code was disabled for builds targeting macOS >
10.11 in #15208. Now that we require macOS 10.12 as a minimum, #17550,
we can remove the startup item code entirely, as the API we were using
was removed in macOS 10.12.
3c2c439dcd wallet: Make -walletdir network only (João Barbosa)
Pull request description:
With this PR `bitcoind -regtest` doesn't run if bitcoin.conf has
```
walletdir=/mnt/mydisk/wallets
```
But works with
```
[regtest]
walletdir=/mnt/mydisk/wallets
```
Doesn't change mainnet behavior.
Closes#15630.
ACKs for top commit:
ryanofsky:
ACK 3c2c439dcd
MarcoFalke:
ACK 3c2c439dcd🍈
meshcollider:
Tested ACK 3c2c439dcd
Tree-SHA512: 8ab3b2db5f3f9cab78b36baaf490c80f7330372cfd8f73fe6536c8fb4c6e55e09f62296feb70617075838b3bcd7101abebbef3b228b6c3dbd42ce8c7a5c372d9
6a2e6b0600 Remove out of date comments for CalculateMaximumSignedTxSize (Gregory Sanders)
Pull request description:
These paths can be hit for probably a number of reasons, and ISMINE spendability is not a requirement to call it.
For example: During watch-only transaction creation, previous transaction in wallet, pubkey imported, but not the witnessscript associated with the prevout.
In this case I think no/minimal comment is better than specific and soon to be out of date.
ACKs for top commit:
achow101:
ACK 6a2e6b0600
darosior:
ACK 6a2e6b0600
Tree-SHA512: ad4c26fd2409eb5aed19d67c19cb5479d226bd11e9298630309c4344f6562ace2e10c2850ebe22770331d71e91320a606e79619b9fe52dd478ce1f589a740122
3958295bc8 wallet: LearnRelatedScripts only if KeepDestination (João Barbosa)
55295fba4c wallet: Lock address type in ReserveDestination (João Barbosa)
Pull request description:
Only mutates the wallet if the reserved key is kept.
First commit is a refactor that makes the address type a class member.
The second commit moves `LearnRelatedScripts` from `GetReservedDestination` to `KeepDestination` to avoid an unnecessary call to `AddCScript` - which in turn prevents multiple entries of the same script in the wallet DB.
ACKs for top commit:
achow101:
Re-ACK 3958295bc8
Sjors:
ACK 3958295bc8
ryanofsky:
Code review ACK 3958295bc8. I like this change. The new behavior makes more sense, and the change makes the code clearer, since the current LearnRelatedScripts call is hard to understand and explain. (Personally, I'd like it if this PR were merged before #17373 or that PR was rebased on top of this one so it would be less confusing.)
meshcollider:
utACK 3958295bc8
Tree-SHA512: 49a5f4b022b28042ad37ea309b28378a3983cb904e234a25795b5a360356652e0f8e60f15e3e64d85094ea63af9be01812d90ccfc08ca4f1dd927fdd8566e33f
c6dd565c88 [gui] watch-only wallet: copy PSBT to clipboard (Sjors Provoost)
39465d545d [wallet] add fillPSBT to interface (Sjors Provoost)
848f889208 [gui] send: include watch-only (Sjors Provoost)
40537f0909 [wallet] ListCoins: include watch-only for wallets without private keys (Sjors Provoost)
Pull request description:
For wallets with `WALLET_FLAG_DISABLE_PRIVATE_KEYS` this makes the watch-only balance available on the send screen (including coin selection). Instead of sending a transaction it generates a PSBT.
The user can take this PSBT and process it with [HWI](https://github.com/bitcoin-core/HWI) or put it an SD card for hardware wallets that support that.
The PSBT is copied to the clipboard. This was the easiest approach; we can add a dialog later to display it, as well as an option to save to disk.
ACKs for top commit:
instagibbs:
test and code review ACK c6dd565c88
meshcollider:
re-ACK c6dd565c88
Tree-SHA512: ebc3da0737e33b255ed926191b84569aedb6097d14868662bd5dce726ce3048e86e9a31eba987b10dffe1482b35c21ae1cd595c2caa4634bc4cf78a826a83852
d0dab897af Refactor: Require scriptPubKey to get wallet SigningProvider (Andrew Chow)
4b0c718f8f Accumulate result UniValue in SignTransaction (Andrew Chow)
Pull request description:
Easier to review ignoring whitespace:
git log -p -n1 -w
This commit does not change behavior. It passes new CScript arguments to
signing functions, but the arguments aren't currently used.
Split from #17261
ACKs for top commit:
instagibbs:
utACK d0dab897af
ryanofsky:
Code review ACK d0dab897af. Thanks for the SignTransaction update. No other changes since last review
Sjors:
Code review ACK d0dab897af
promag:
Code review ACK d0dab897af.
meshcollider:
Code review ACK d0dab897af
Tree-SHA512: c3f52df20fd9d6b3b5aa65562cf5f7dce7b7f44c148b0f988f8b578fce2a28e9b7bf010f5f04bb5bf60f5272b2899f1dbbfb8aee81579c21c9cba559d1d2bb70
b007efdf19 Allow BnB when subtract fee from outputs (Andrew Chow)
db15e71e79 Use BnB when preset inputs are selected (Andrew Chow)
Pull request description:
Currently we explicitly disable BnB when there are preset inputs selected or when the subtract fee from outputs option is enabled. This PR enables BnB for both cases.
Kind of an alternative to #17246 (implements the subtract fee from outputs part of it) and borrows a test from there too.
ACKs for top commit:
instagibbs:
reACK b007efdf19
Sjors:
re-ACK b007efdf19
Tree-SHA512: 933276b09b2fa2ab43db7f0b98762f06f6f5fa8606195f96aca9fa1cb71ae4ee7156028dd482b1cada82ddd0996a9daf12ea5c152589fdf192cd96cbc51e99df
e20c72f9f0 Fire TransactionRemovedFromMempool from mempool (251)
Pull request description:
This pull request fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code.
It also resolves the `txmempool -> validation -> validationinterface -> txmempool` circular dependency.
Ideally, `validationinterface` is a dumb component that doesn't have any knowledge of the sub-systems it sends its notifications to. The commit that aims to resolve this circular dependency by moving `txmempool` specific code out of `validationinterface` to `txmempool` where it belongs.
ACKs for top commit:
jnewbery:
ACK e20c72f9f0
Tree-SHA512: 354c3ff1113b21a0b511d80d604edfe3846dddae3355e43d1387f68906e54bf5dc01e7c029edc0b8e635b500b2ab97ee50362e2486eb4319f7347ee9a9e6cef3
a2e581de94 build: Create test utility library from src/test/util/ (Harris)
Pull request description:
This PR creates a static **test utility library** that replaces repetitive compilations of sources from *src/test/util* in **unit**, **gui** and **bench** **tests**.
The original issue is here: https://github.com/bitcoin/bitcoin/issues/17401
The changes are:
* a new *Makefile.test_util.include*
* a new entry in *Makefile.am* that includes *Makefile.test_util.include* when testing is enabled
* removal of all *src/test/util* headers & sources from unit, gui and bench Makefiles
* addition of *libtest_util.a* at LDADD's of every test
ACKs for top commit:
MarcoFalke:
ACK a2e581de94🍞
Tree-SHA512: d172127a26ee70d16625e17d7d94337a65472c57bb97f910c357c52d3dc082ea478ee586ee9074d9ebfeb05b75027e5e15f5bcd2aa35962dadfd9ac6bfd55ab9
e9a27cf338 refactor: Remove unused COINBASE_FLAGS (Neha Narula)
Pull request description:
Commit d449772cf6 stopped setting
COINBASE_FLAGS, and it looks like it hasn't been used since P2SH.
Following up on #17489, remove COINBASE_FLAGS which is unused. I verified that removing this did not change the contents of the coinbase's scriptSig.
ACKs for top commit:
laanwj:
ACK e9a27cf338
MarcoFalke:
ACK e9a27cf338💻
Tree-SHA512: f9dac124ce7e3edcae974137764bb5039387b1b123b86af44486e398aa4a8d91a9ecf640e207b364ae303acbbaee7cca300d303ea3d6869ba9cae2bf555a6334
fa538813b1 scripted-diff: Replace ::mempool with m_node.mempool in tests (MarcoFalke)
8888ad02e2 test: Replace recursive lock with locking annotations (MarcoFalke)
fac07f2038 node: Add reference to mempool in NodeContext (MarcoFalke)
Pull request description:
This is the first step toward making the mempool a global that is not initialized before main.
#### Motivation
Currently the mempool is a global that is initialized before the `main` function. This is confusing and easy to get wrong. E.g. the mempool constructor queries state that has not been initialized, like randomness (fixed), or command line arguments (not an issue last time I checked). Also without having the chainstate (chain tip) initialized first, it doesn't make conceptually sense to have a mempool, since the mempool builds txs on top of the utxo set (chain tip).
Finally, in the future someone might want to run a consensus-only full node (`-nowallet -noblockfilter -no... -nomempool` command line options) that only verifies blocks and updates the utxo set.
This is conceptually the same change that has already been done for the connection manager `CConnman`.
ACKs for top commit:
jnewbery:
utACK fa538813b1
ariard:
Tested ACK fa53881.
Tree-SHA512: 2c446a8a51476354aad7126c2b833500d36b24490caa94f847b2bdc622054de0dae28980f23e3d91b1b492dc32931656d98dbd019af9e4e58f2f8c5375aac694
The function IsStandardTx() returns rejection reason "bare-multisig" if the
transaction has a bare multisig output and the policy flag fIsBareMultisigStd
is false (set by the boolean command-line argument "-permitbaremultisig" -- for
the unit test, we simply set the global flag variable directly).
Commit d449772cf6 stopped setting
COINBASE_FLAGS, and it looks like it hasn't been used since P2SH.
Update the help string to remove "flags", which is not specified in
BIP 22.
faffa7f0dc wallet: Avoid showing GUI popups on RPC errors (take 2) (MarcoFalke)
Pull request description:
Commit 8b0d82bb42 claims "This commit does not change behavior." However, it re-introduced the bug I tried to fix in #17070
ACKs for top commit:
ryanofsky:
Code review ACK faffa7f0dc
Tree-SHA512: 99987f80c76414dca40c7d76b2fe4ea853debbe3c49e7acdeab2596c726a2935c468f4484d49212e65ecc9c8b0d861c0c2b83c1ddfc07670540699199dbfecb0
30fb598737 Fix segfault in allocator_tests/arena_tests (Jeffrey Czyz)
15c84f53f4 Define ARENA_DEBUG in Travis test runs (Jeffrey Czyz)
ad71548822 Fix compilation errors in support/lockedpool.cpp (Jeffrey Czyz)
Pull request description:
Changes in #12048 cause a compilation error in Arena::walk() when
ARENA_DEBUG is defined. Specifically, Arena's chunks_free map was
changed to have a different value type.
Additionally, missing includes cause other compilation errors when
ARENA_DEBUG is defined.
Reproduced with:
make CPPFLAGS=-DARENA_DEBUG
ACKs for top commit:
laanwj:
ACK 30fb598737
fanquake:
ACK 30fb598737 - thanks for following up jkczyz.
Tree-SHA512: 4eec368a4e9c67e4e2a27bc05608a807c2892d50c60d06ed21490cd274c0369f9671bc05b3006acc2a193316caf4896454c9c299603bfed29bd488f1987ec446
8944c1d340 Changed tooltips of receive form to highlight difference between Label and Message (dannmat)
Pull request description:
I have changed the tooltips for 'Label' & 'Message' text fields to be more clear, stating the difference between the two (#17173)
ACKs for top commit:
MarcoFalke:
ACK 8944c1d340
laanwj:
ACK 8944c1d340
Tree-SHA512: 7fbea4d3c4416264ae6c146d51d29958c418a278bdd6744133db0b684ad7a9413178c005592aa21a81d127f3f3a8583fc5de00078239db08e6f101f657a5dd3a
Also remove a needless loop in DecodeBase58 to prune zeroes in the base256
output of the conversion. The number of zeroes is implied by keeping track
explicitly of the length during the loop.
Invalid PSBTs need to be re-created, so the next role is the
Creator (new PSBTRole). Additionally, we need to know what went
wrong so an error field was added to PSBTAnalysis.
A PSBTAnalysis indicating invalid will have empty everything,
next will be set to PSBTRole::CREATOR, and an error message.
e5a0bece6e doc: add OpenSSL removal to release-notes.md (fanquake)
397dbae070 ci: remove OpenSSL installation (fanquake)
a4eb839619 doc: remove OpenSSL from build instructions and licensing info (fanquake)
648b2e3c32 depends: remove OpenSSL package (fanquake)
8983ee3e6d build: remove OpenSSL detection and libs (fanquake)
b49b6b0f70 random: Remove remaining OpenSSL calls and locking infrastructure (fanquake)
4fcfcc294e random: stop retrieving random bytes from OpenSSL (fanquake)
5624ab0b4f random: stop feeding RNG output back into OpenSSL (fanquake)
Pull request description:
Now that #17165 has been merged, removing our remaining OpenSSL usage is possible.
That remaining usage was a call to [`RAND_bytes`](https://www.openssl.org/docs/manmaster/man3/RAND_bytes.html) during the ::SLOW path of [ProcRand](https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L616). As well as feeding output from our RNG back into OpenSSL via [`RAND_add`](https://www.openssl.org/docs/manmaster/man3/RAND_add.html) during the ::SLOW and ::SLEEP paths.
Optimistically tagged for `0.20.0`. Needs discussion, potentially in an upcoming weekly meeting?
Closes#12530.
ACKs for top commit:
MarcoFalke:
ACK e5a0bece6e
laanwj:
ACK e5a0bece6e
Tree-SHA512: 02fce08ec91d20e0da51e9314eec53dcf8699cded02f0a005417d627520c20b826332cb42bdae132af283d4903aa3088a9f613f3aea915d655a51532a4d4796c
Easier to review ignoring whitespace:
git log -p -n1 -w
This commit does not change behavior. It passes new CScript arguments to
signing functions, but the arguments aren't currently used.
SignTransaction will be called multiple times in the future. Pass
it a result UniValue so that it can accumulate the results of multiple
SignTransaction passes.
49f4c7f069 tests: Add fuzzing harness for various PSBT related functions (practicalswift)
Pull request description:
Add fuzzing harness for various PSBT related functions.
**Testing this PR**
Run:
```
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
--with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/psbt
```
ACKs for top commit:
MarcoFalke:
re-ACK 49f4c7f069🐟
Tree-SHA512: 4cebe62bd8c244ee40a43e829f5bd175ab40e1dfbbab1affb1529374858225820d6c9fa9ba45862bf56c1522845422fd96d620cedbdec52a67ac1449dec4e1b2
e161bc74d2 doc: Remove bitness from bitcoin-qt help message and manpage (Wladimir J. van der Laan)
Pull request description:
Remove the `(64-bit)` from the bitcoin-qt help message.
Since removing the Windows 32-bit builds, it is no longer information that is often useful for troubleshooting. This never worked for other architectures than x86, and the only 32-bit x86 build left is the Linux one. Linux users tend to know what architecture they are using.
It also accidentally ends up in the bitcoin-qt manpage (if you happen to be generating them on a x86 machine), which gets checked in. See for example 1bc9988993 (diff-e4b84be382c8ea33b83203ceb8c85296)
ACKs for top commit:
practicalswift:
ACK e161bc74d2 -- rationale makes sense and diff looks correct :)
MarcoFalke:
Tested ACK e161bc74d2🔮
Tree-SHA512: d38754903252896dc86fac6c12ad6615d322c2744db7c02b18574a08c69e8876b2c905e1f09b324002236b111ee93479f89769c562e7b3b2e6eb2992d76464ef
On the ::SLOW path we would use OpenSSL as an additional source of
random bytes. This commit removes that functionality. Note that this was
always only an additional source, and that we never checked the return
value
RAND_bytes(): https://www.openssl.org/docs/manmaster/man3/RAND_bytes.html
RAND_bytes() puts num cryptographically strong pseudo-random bytes into buf.
On the ::SLOW or ::SLEEP paths, we would feed our RNG output back into
OpenSSL using RAND_add. This commit removes that functionality.
RAND_add(): https://www.openssl.org/docs/manmaster/man3/RAND_add.html
RAND_add() mixes the num bytes at buf into the internal state of the
random generator. This function will not normally be needed, as
mentioned above. The randomness argument is an estimate of how much
randomness is contained in buf, in bytes, and should be a number
between zero and num.
2f5f7d6b13 GuessVerificationProgress: cap the ratio to 1 (darosior)
Pull request description:
Noticed `getblockchaininfo` would return a `verificationprogress` > 1, especially while generating. This caps the verification progress to `1`.
Tried to append a check to functional tests but this would pass even without the patch, so it seems better to not add a superfluous check (but this can easily be reproduced by trying to generate blocks in the background and `watch`ing `getblockchainfo`).
ACKs for top commit:
laanwj:
ACK 2f5f7d6b13
promag:
ACK 2f5f7d6b13.
Tree-SHA512: fa3aca12acab9c14dab3b2cc94351082f548ea6e6c588987cd86e928a00feb023e8112433658a0e85084e294bfd940eaafa33fb46c4add94146a0901bc1c4f80
d1c02775aa Report amount of data gathered from environment (Pieter Wuille)
64e1e022ce Use thread-safe atomic in perfmon seeder (Pieter Wuille)
d61f2bb076 Run background seeding periodically instead of unpredictably (Pieter Wuille)
483b94292e Add information gathered through getauxval() (Pieter Wuille)
11793ea22e Feed CPUID data into RNG (Pieter Wuille)
a81c494b4c Use sysctl for seeding on MacOS/BSD (Pieter Wuille)
2554c1b81b Gather additional entropy from the environment (Pieter Wuille)
c2a262a78c Seed randomness with process id / thread id / various clocks (Pieter Wuille)
723c796667 [MOVEONLY] Move cpuid code from random & sha256 to compat/cpuid (Pieter Wuille)
cea3902015 [MOVEONLY] Move perfmon data gathering to new randomenv module (Pieter Wuille)
b51bae1a5a doc: minor corrections in random.cpp (fanquake)
Pull request description:
This introduces a new `randomenv` module that queries varies non-cryptographic (and non-RNG) sources of entropy available on the system; things like user IDs, system configuration, time, statistics, CPUID data.
The idea is that these provide a fallback in scenarios where system entropy is somehow broken (note that if system entropy *fails* we will abort regardless; this is only meant to function as a last resort against undetected failure). It includes some data sources OpenSSL currently uses, and more.
The separation between random and randomenv is a bit arbitrary, but I felt that all this "non-essential" functionality deserved to be separated from the core random module.
ACKs for top commit:
TheBlueMatt:
utACK d1c02775aa. Certainly no longer measuring the time elapsed between a 1ms sleep (which got removed in the latest change) is a fair tradeoff for adding about 2 million other actually-higher-entropy bits :).
laanwj:
ACK d1c02775aa
Tree-SHA512: d290a8db6538a164348118ee02079e4f4c8551749ea78fa44b2aad57f5df2ccbc2a12dc7d80d8f3e916d68cdd8e204faf9e1bcbec15f9054eba6b22f17c66ae3
Remove the `(64-bit)` from the bitcoin-qt help message.
Since removing the Windows 32-bit builds, it is no longer information
that is often useful for troubleshooting. This never worked for other
architectures than x86, and the only 32-bit x86 build left is the Linux
one. Linux users tend to know what architecture they are using.
It also accidentally ends up in the bitcoin-qt manpage.
The test uses reinterpret_cast<void*> on unallocated memory. Using this
memory in printchunk as char* causes a segfault, so have printchunk take
void* instead.
Changes in #12048 cause a compilation error in Arena::walk() when
ARENA_DEBUG is defined. Specifically, Arena's chunks_free map was
changed to have a different value type.
Additionally, missing includes cause other compilation errors when
ARENA_DEBUG is defined.
Reproduced with:
make CPPFLAGS=-DARENA_DEBUG
5e8a56348b test: add unit test for non-standard txs with too large scriptSig (Sebastian Falbesoner)
Pull request description:
Approaches the first missing test of issue #17394: Checks that the function `IsStandardTx()` returns rejection reason `"scriptsig-size"` if any one the inputs' scriptSig is larger than 1650 bytes.
ACKs for top commit:
MarcoFalke:
ACK 5e8a56348b
instagibbs:
ACK 5e8a56348b
Tree-SHA512: 79977b12ddea9438a37cefdbb48cc551e4ad02a8ccfaa2d2837ced9f3a185e2e07cc366c243b9e3c7736245e90e315d7b4110efc6b440c63dbef7ee2c9d78a73
-BEGIN VERIFY SCRIPT-
# tx pool member access (mempool followed by dot)
sed --regexp-extended -i -e 's/(::)?\<mempool\>\.([a-zA-Z])/m_node.mempool->\2/g' $(git grep -l mempool ./src/test)
# plain global (mempool not preceeded by dot, but followed by comma)
sed --regexp-extended -i -e 's/([^\.])(::)?\<mempool\>,/\1*m_node.mempool,/g' $(git grep -l mempool ./src/test)
-END VERIFY SCRIPT-
Currently it is an alias to the global ::mempool and should be used as
follows.
* Node code (validation and transaction relay) can use either ::mempool
or node.mempool, whichever seems a better fit.
* RPC code should use the added convenience getter EnsureMempool, which
makes sure the mempool exists before use. This prepares the RPC code
to a future where the mempool might be disabled at runtime or compile
time.
* Test code should use m_node.mempool directly, as the mempool is always
initialized for tests.
edb6b768a4 fix uninitialized variable nMinerConfirmationWindow (NullFunctor)
Pull request description:
It is used for the computation of `BIP9WarningHeight`, and by that time it isn't initialized.
ACKs for top commit:
jnewbery:
utACK edb6b768a
promag:
ACK edb6b768a4, commit description could be cleaned up though.
MarcoFalke:
ACK edb6b768a4, used python3 to do the addition locally 📍
practicalswift:
ACK edb6b768a4, used `clang++ -O2` on the previous version^W^W^W^W^W^W`bc` to verify the addition locally 🏓
Sjors:
Code review ACK edb6b76. Nit: commit description has duplicate text.
Tree-SHA512: 6fa0be0ecfbfd5d537f2c5b4a9333c76530c1f3182f777330cc7939b0496e37b75d8f8810cdaf471a9bd3247b425f2e239578300dfa0d5a87cd14a6ccfafa619
Also rename the "result_complete" variable in GetSettingsList() to "done" to be
more consistent with GetSetting().
This change doesn't affect current behavior but could be useful in the future
to support dynamically changing settings at runtime and adding new settings
sources, because it lets high priority sources reset settings back to default
(see test).
By removing a special case for null, this change also helps merge code treat
settings values more like black boxes, and interfere less with settings parsing
and retrieval.
The bool/int/string flags were added speculatively in #16097 and trigger errors
when type checking is actually implemented in
https://github.com/bitcoin/bitcoin/pull/16545
-BEGIN VERIFY SCRIPT-
sed -i 's/ALLOW_\(BOOL\|INT\|STRING\)/ALLOW_ANY/g' src/test/util_tests.cpp src/test/getarg_tests.cpp
-END VERIFY SCRIPT-
This commit does not change behavior.
Test GetSetting and GetArg type coercion, negation, and default value handling.
Test is expanded later to cover other flags besides ALLOW_ANY when they are
implemented in https://github.com/bitcoin/bitcoin/pull/16545
This commit does not change behavior.
Add for consistency with ArgsManager::GetSetting method and to make setting
types accessible to ArgsManager callers and tests (test added next commit).
This commit does not change behavior.
* Instead of calling RandAddSeedSleep anytime the scheduler goes
idle, call its replacement (RandAddSeedPeriodic) just once per
minute. This has better guarantees of actually being run, and
helps limit how frequently the dynamic env data is gathered.
* Since this code runs once per minute regardless now, we no
longer need to keep track of the last time strengthening was
run; just do it always.
* Make strengthening time context dependent (100 ms at startup,
10 ms once per minute afterwards).
e2c03c1156 doc: Add relase note for db→walletdb rename (Wladimir J. van der Laan)
4c1d263d93 scripted-diff: Change `BCLog::DB` to `BCLog::WALLETDB` (Wladimir J. van der Laan)
6b42b3ba90 Rename `db` log category to `walletdb` (like `coindb`) (Wladimir J. van der Laan)
Pull request description:
Rename the `db` log category to `walletdb` (in the style of, and to distinguish from `coindb`). Deprecate (but still accept) '-debug=db'.
Second commit is a scripted commit that changes the enum item name.
ACKs for top commit:
hebasto:
ACK e2c03c1156, tested on Linux Mint 19.2:
Tree-SHA512: a044de6f9a70e735cbb1caa4ed6bf75bc2269b2d5bc3241a25b6a6d69c1fc1d83456e252b431388ae61f4821e4fc06ecc1b634816ceadbe9a3c0e494bee6c11e
083c954b02 Add settings_tests (Russell Yanofsky)
7f40528cd5 Deduplicate settings merge code (Russell Yanofsky)
9dcb952fe5 Add util::Settings struct and helper functions. (Russell Yanofsky)
e2e37cfe8a Remove includeconf nested scope (Russell Yanofsky)
5a84aa880f Rename includeconf variables for clarity (Russell Yanofsky)
dc8e1e7548 Clarify emptyIncludeConf logic (Russell Yanofsky)
Pull request description:
This is a refactoring-only change that makes it easier to add a new settings source.
This PR doesn't change behavior. The [`util_ArgsMerge`](deb2327b43/src/test/util_tests.cpp (L626-L822)) and [`util_ChainMerge`](deb2327b43/src/test/util_tests.cpp (L843-L924)) tests added in #15869 and #15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change.
This change:
- Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3e4975203ad6222ba2b00c83b4e4213793 from #15935).
- Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](c459c5f701/src/util/system.cpp (L221-L244)), [GetNetBoolArg](c459c5f701/src/util/system.cpp (L255-L261)), [GetArgs](c459c5f701/src/util/system.cpp (L460-L467)), [IsArgNegated](c459c5f701/src/util/system.cpp (L482-L491)), [GetUnsuitableSectionOnlyArgs](c459c5f701/src/util/system.cpp (L343-L352))) in inconsistent ways.
- Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](69d44f3cc7/src/util/system.cpp (L323-L326)) and [more ignored arguments](69d44f3cc7/src/util/settings.cpp (L67-L72)), and config file [reverse precedence](69d44f3cc7/src/util/settings.cpp (L61-L65)), [inconsistently applied top-level settings](69d44f3cc7/src/util/settings.cpp (L55-L59)), and [zombie values](69d44f3cc7/src/util/settings.cpp (L101-L108)).
The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended:
* 70675c3e4975203ad6222ba2b00c83b4e4213793 from #15935 – _Add \<datadir>/settings.json persistent settings storage_
* 04c80c40df9fc6f4734ba238ea7f65607cf88089 from #15937 – _Add loadwallet and createwallet RPC load_on_startup options_
ACKs for top commit:
ariard:
ACK 083c954
jnewbery:
ACK 083c954b02
jamesob:
ACK 083c954b02
Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659
fad1de66a2 wallet: Remove unused boost::this_thread::interruption_point (MarcoFalke)
Pull request description:
`BerkeleyEnvironment::Open` is only called from the main thread (init) or an http rpc thread, neither of which can be interrupted, so remove the useless interruption point.
`BerkeleyEnvironment{}` is only used in tests, which run in a single process/thread, so remove the useless interruption point.
ACKs for top commit:
laanwj:
ACK fad1de66a2
fanquake:
ACK fad1de66a2
Tree-SHA512: dacd8398e966e4a6ce5cf7d3ed821c9c267eff40b14c0635085441647cdb72d1642807f89355419f1710f814c7963e35a10d102d0b985c7198261dfc736256f8
0b75a7f068 wallet: Reuse existing batch in CWallet::SetUsedDestinationState (João Barbosa)
01f45dd00e wallet: Avoid recursive lock in CWallet::SetUsedDestinationState (João Barbosa)
Pull request description:
This PR makes 2 distinct changes around `CWallet::SetUsedDestinationState`:
- 1st the recursive lock is removed and now it requires the lock to be held;
- 2nd change is to support, in the best case, just a wallet database flush when transaction is added to the wallet.
ACKs for top commit:
achow101:
ACK 0b75a7f068
MarcoFalke:
ACK 0b75a7f068
ryanofsky:
Code review ACK 0b75a7f068. Code changes looks fine but PR description should be updated to say what benefits of the change are. I might have missed something, but I didn't see a place where multiple batches were used previously and a single batch was used now. So the main benefit of this change appears to be removing a recursive lock? And maybe moving toward a consistent convention for passing batch instances?
Tree-SHA512: abcf23a5850d29990668db20d6f624cca3e89629cc9ed003e0d05cde1b58ab2ff365034f156684ad13e55764b54c6c0c2bc7d5f96b8af7dc5e45a3be955d6b15
36b68de5b2 Remove getBlockDepth method from Chain::interface (Antoine Riard)
b66c429c56 Remove locked_chain from GetDepthInMainChain and its callers (Antoine Riard)
0ff03871ad Use CWallet::m_last_block_processed_height in GetDepthInMainChain (Antoine Riard)
f77b1de16f Only return early from BlockUntilSyncedToCurrentChain if current tip is exact match (Antoine Riard)
769ff05e48 Refactor some importprunedfunds checks with guard clause (Antoine Riard)
5971d3848e Add block_height field in struct Confirmation (Antoine Riard)
9700fcb47f Replace CWalletTx::SetConf by Confirmation initialization list (Antoine Riard)
5aacc3eff1 Add m_last_block_processed_height field in CWallet (Antoine Riard)
10b4729e33 Pass block height in Chain::BlockConnected/Chain::BlockDisconnected (Antoine Riard)
Pull request description:
Work starter to remove Chain::Lock interface by adding m_last_block_processed_height in CWallet and m_block_height in CMerkleTx to avoid GetDepthInMainChain having to keep a lock . Once this one done, it should ease work to wipe out more cs_main locks from wallet code.
I think it's ready for a first round of review before to get further.
- `BlockUntilSyncedToCurrent` : restrain isPotentialTip to isTip because we want to be sure that wallet see BlockDisconnected callbacks if its height differs from the Chain one. It means during a reorg, an RPC could return before the BlockDisconnected callback had been triggered. This could cause a tx that had been included in the disconnected block to be displayed as confirmed, for example.
~~- `AbandonTransaction` : in case of conflicted tx (nIndex = -1), we set its m_block_height to the one of conflicting blocks, but if this height is superior to CWallet::m_last_block_processed_height, that means tx isn't conflicted anymore so we return 0 as tx is again unconfirmed~~ After #16624, we instead rely on Confirmation.
~~- `AddToWalletIfInvolvingMe`: in case of block disconnected, transactions are added to mempool again, so we need to replace old txn in `mapWallet` with a height set to zero so we remove check on block_hash.IsNull~~ Already done in #16624
ACKs for top commit:
jnewbery:
@jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch (36b68de5b2).
jkczyz:
> @jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch ([36b68de](36b68de5b2)).
meshcollider:
utACK 36b68de5b2
ryanofsky:
Code review ACK 36b68de5b2. Changes since last review: new jkczyz refactor importprunedfunds commit, changed BlockUntilSyncedToCurrentChainChanges commit title and description, changed Confirmation struct field order and line-wrapped comment
jnewbery:
utACK 36b68de5b2
promag:
Code review ACK 36b68de5b2.
Tree-SHA512: 08b89a0bcc39f67c82a6cb6aee195e6a11697770c788ba737b90986b4893f44e90d1ab9ef87239ea3766508b7e24ea882b7199df41173ab27a3d000328c14644
Get rid of settings merging code in util/system.cpp repeated 5 places,
inconsistently:
- ArgsManagerHelper::GetArg
- ArgsManagerHelper::GetNetBoolArg
- ArgsManager::GetArgs
- ArgsManager::IsArgNegated
- ArgsManager::GetUnsuitableSectionOnlyArgs
Having settings merging code separated from parsing simplifies parsing somewhat
(for example negated values can simply be represented as false values instead
of partially cleared or emply placeholder lists).
Having settings merge happen one place instead of 5 makes it easier to add new
settings sources and harder to introduce new inconsistencies in the way
settings are merged.
This commit does not change behavior in any way.
Implement merging of settings from different sources (command line and config
file) separately from parsing code in system.cpp, so it is easier to add new
sources.
Document current inconsistent merging behavior without changing it.
This commit only adds new settings code without using it. The next commit calls
the new code to replace existing code in system.cpp.
Co-authored-by: John Newbery <john@johnnewbery.com>
CheckInputs() used to check no double spends, scripts & sigs and amounts. Since
832e0744cb, the double spend and amount checks
have been moved to CheckTxInputs(), and CheckInputs() now just validates
input scripts. Rename the function to CheckInputScripts().
-BEGIN VERIFY SCRIPT-
sed -i -E -e 's/CheckInputs\b/CheckInputScripts/g' $(git grep -l CheckInputs | grep -v doc/)
-END VERIFY SCRIPT-
fae43a97ca test: Seed test RNG context for each test case, print seed (MarcoFalke)
Pull request description:
Debugging failing unit tests is hard if the failure is non-deterministic and the seed is not known.
Fix that by printing the seed and making it possible to set the seed from outside.
ACKs for top commit:
davereikher:
Tested ACK fae43a97ca
Tree-SHA512: 33d848dd1f4180d3664ecf60e9810c2a93590c05276b2c46b1e4fe6e376b45916a46b90c803bb602750ab666da3a05ce499e550024685a90b8cc38fab6667cb8
5506ecfe7a [refactor] Replace global int nScriptCheckThreads with bool (John Newbery)
d9957623b4 [tests] Don't use TestingSetup in the checkqueue_tests (John Newbery)
Pull request description:
The meaning of this value is confusing. Refactor it and add comments.
ACKs for top commit:
sipa:
ACK 5506ecfe7a
promag:
ACK 5506ecfe7a, only change was addressing my nits.
laanwj:
Code review ACK 5506ecfe7a
MarcoFalke:
ACK 5506ecfe7a🥐
Tree-SHA512: 78536727c98d2c23f3c0f3f169131474fef9a4486ae65029011caf06eab30f6f70ff73a65b2fb04a5d969fc1150858d1c6ea4767f04d48c1eea6b829316d0e63
fa4c6fa9b1 doc: Add documentation for new test/lib (MarcoFalke)
faec28252c scripted-diff: test: Move setup_common to test library (MarcoFalke)
Pull request description:
Sorry for clickbait, this is only a move-only scripted-diff commit and one documentation commit.
Longer term, someone who knows something about build systems can make this an actual library. Motivation for this is that each module gets compiled for each target that includes it. For example, setup_common is compiled 27 times (for the fuzz suite) and another 3 times for the other tests (bench, unit test, gui)
ACKs for top commit:
practicalswift:
ACK fa4c6fa9b1 -- diff looks correct and Travis is happy
jonatack:
ACK fa4c6fa9b1 with the reserve that the commit messages (and PR description) contain the motivation for this change. Built, ran tests, light code review.
ryanofsky:
Code review ACK fa4c6fa9b1. I didn't realize `lib` was actually name of existing directory, not a new name. But in any case this looks good and nice to have one scripted diff instead of two.
Tree-SHA512: 2e176df90c60578276e4a6dc83ff57ff59d8e666ecf30c5ceacb8c326725da91baa4cac3dfa7a2e1605f58122a3e3e27e4938ff33e3a0ce7ea53afffebbf57a4
The global nScriptCheckThreads int is confusing and is only needed for
its int-ness in AppInitMain. Move all `-par` parsing logic there and
replace the int nScriptCheckThreads with a bool
g_parallel_script_checks.
Also tidy up logic and improve comments.
We don't remove yet Chain locks as we need to preserve lock
order with CWallet one until swapping at once to avoid
deadlock failures (spotted by --enable-debug)
is exact match
In the next commit, we start using BlockConnected/BlockDisconnected
callbacks to establish tx depth, rather than querying the chain
directly.
Currently, BlockUntilSyncedToCurrentChain will return early if
the best block processed by the wallet is a descendant of the node'tip.
That means that in the case of a re-org, it won't wait for the
BlockDisconnected callbacks that have been enqueued during the re-org
but have not yet been triggered in the wallet.
Change BlockUntilSyncedToCurrentChain to only return early if the
wallet's m_last_block_processed matches the tip exactly. This ensures
that there are no BlockDisconnected or BlockConnected callbacks
in-flight.
At wallet loading, we rely on chain state querying to retrieve
height of txn, to do so we ensure that lock order is respected
between cs_main and cs_wallet.
If wallet loaded is the wallet-tool one, all wallet txn will
show up with a height of zero. It doesn't matter as confirmation
height is not used by wallet-tool.
Reorder arguments and document Confirmation calls to avoid
ambiguity.
Fixes nits left from #16624
286f197704 Add util_ArgParsing test (Russell Yanofsky)
Pull request description:
ArgsManager test coverage for parsing of integer and boolean values is
currently very poor and doesn't give us a way of knowing whether changes to
ArgsManager may unintentionally break backwards compatibility, so this adds a
new test to catch regressions.
ACKs for top commit:
promag:
ACK 286f197, more surprising results 😱
laanwj:
ACK 286f197704
Tree-SHA512: 9e1db3ef87e55abbc280af60c088f35765a1f9e2ec20507ad0c1992027b875490016868dcb8cc287e6df279dd0e00f10550901af3de3d36287867249e0bd8207
3d05d33269 cli: fix -getinfo output when compiled with no wallet (fanquake)
Pull request description:
master (33b155f287):
```bash
src/bitcoin-cli -getinfo
{
"version": 199900,
"protocolversion": 70015,
"blocks": 602348,
"headers": 602348,
"verificationprogress": 0.9999995592310106,
"timeoffset": 0,
"connections": 10,
"proxy": "",
"difficulty": 13691480038694.45,
"chain": "main",
"walletversion": null,
"balance": null,
"keypoololdest": null,
"keypoolsize": null,
"paytxfee": null,
"relayfee": 0.00001000,
"warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
}
```
This PR (3d05d33269):
```bash
{
"version": 199900,
"protocolversion": 70015,
"blocks": 602348,
"headers": 602348,
"verificationprogress": 0.9999996313568186,
"timeoffset": 0,
"connections": 10,
"proxy": "",
"difficulty": 13691480038694.45,
"chain": "main",
"relayfee": 0.00001000,
"warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
}
```
ACKs for top commit:
MarcoFalke:
ouch ACK 3d05d33269
laanwj:
ACK 3d05d33269
darosior:
ACK 3d05d33269
Tree-SHA512: 055424e122a082cbfea410da287d9ceb7ed405fd68d53e2f5bef62beea80bc374a7d00366de0479d23faecb7f063b232aca52e9fdbdb97c58ddf46e7749136a9
3645e4ca00 Add missing newline in util_ChainMerge test (Russell Yanofsky)
Pull request description:
This was causing a lot of test cases not not be very meaningful because
multiple configuration options were combined into one line.
The changes in test output with this fix make sense and look like:
```diff
- testnet=1 regtest=1 || test
+ testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one.
```
Issue was reported and debugged by
Wladimir J. van der Laan <laanwj@protonmail.com> in
https://github.com/bitcoin/bitcoin/pull/17385#issuecomment-550033222
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
laanwj:
ACK 3645e4ca00
practicalswift:
ACK 3645e4ca00 -- diff looks correct
Tree-SHA512: ca5bde9b9f553811d4827113f4880d15d7b8f4f1455b95bbf34c9a1512fdd53062f1a2133c50d9b54f94160a1ee77a54bc82681a5f3bf25d2b0d01f8a8e95165
ArgsManager test coverage for parsing of integer and boolean values is
currently very poor and doesn't give us a way of knowing whether changes to
ArgsManager may unintentionally break backwards compatibility, so this adds a
new test to catch regressions.
dcef9a2922 logs: add timing information to FlushStateToDisk() (James O'Beirne)
41edaf227a logs: add BCLog::Timer and related macros (James O'Beirne)
Pull request description:
It's currently annoying to detect FlushStateToDisk() calls when benchmarking since they have to be inferred from a drop in coins count from the `UpdateTip: ` log messages. This adds a new logging utility, `BCLog::Timer`, and some related macros that are generally useful for printing timing-related logging messages, and a message that is unconditionally written when the coins cache is flushed to disk.
```
2019-09-04T20:17:51Z FlushStateToDisk: write block and undo data to disk completed (3ms)
2019-09-04T20:17:51Z FlushStateToDisk: write block index to disk completed (370ms)
2019-09-04T20:17:51Z FlushStateToDisk: write coins cache to disk (2068451 coins, 294967kB) completed (21481ms)
```
ACKs for top commit:
laanwj:
Thanks, ACK dcef9a2922
ryanofsky:
Code review ACK dcef9a2922. No changes since last review other than moving code to new timer.h header
Tree-SHA512: 6d61e48a062d3edb48d0e056a6f0b1f8031773cc99289ee4544f8349d24526b88519e1e304009d56e428f1eaf76c857bf8e7e1c0b6873a6f270306accb5edc3d
This was causing a lot of test cases not not be very meaningful because
multiple configuration options were combined into one line.
The changes in test output with this fix make sense and look like:
```diff
- testnet=1 regtest=1 || test
+ testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one.
```
Issue was reported and debugged by
Wladimir J. van der Laan <laanwj@protonmail.com> in
https://github.com/bitcoin/bitcoin/pull/17385#issuecomment-550033222
1c26c16065 Improve "Hide" button tool-tip message (Danny-Scott)
Pull request description:
Cleaned up the tool tip text, it looks as though it just got included back in 2014 when the whole section was added.
Changed hide button tool tip within transaction fee settings area from "collapse fee-settings" to "Hide transaction fee settings" to be more user friendly and fit with other tool tips.
ACKs for top commit:
laanwj:
ACK 1c26c16065
Tree-SHA512: e2c83271c273f785ac625da9f88e095076043e21a9c59792049c271747837d19483e0cae5466c26ef3231947b6245680c4c136a530ba6f1885f9ddc18f2560d6
fa2c44c3cc test: Add ASSERT_DEBUG_LOG to unit test framework (MarcoFalke)
fa1936f57b logging: Add member for arbitrary print callbacks (MarcoFalke)
Pull request description:
Similar to `assert_debug_log` in the functional test framework
Top commit has no ACKs.
Tree-SHA512: aa9eaeca386b61d806867c04a33275f6eb4624fa5bf50f2928d16c83f5634bac96bcac46f9e8eda3b00b4251c5f12d7b01d6ffd84ba8e05c09eeec810cc31251
92b2f5306b test: add dumptxoutset RPC test (James O'Beirne)
c1ccbc3dde devtools: add utxo_snapshot.sh (James O'Beirne)
57cf74c991 rpc: add dumptxoutset (James O'Beirne)
92fafb3a7d coinstats: add coins_count (James O'Beirne)
707fde7b9b add unused SnapshotMetadata class (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
This changeset defines the serialization format for UTXO snapshots and adds an RPC command for creating them, `dumptxoutset`. It also adds a convenience script for generating and verifying snapshots at a certain height, since that requires doing a hacky rewind of the chain via `invalidateblock`.
All of this is unused at the moment.
ACKs for top commit:
laanwj:
ACK 92b2f5306b
Tree-SHA512: 200dff87767f157d627e99506ec543465d9329860a6cd49363081619c437163a640a46d008faa92b1f44fd403bfc7a7c9e851c658b5a4849efa9a34ca976bf31
104f7de593 remove old bootstrap relevant code (tryphe)
Pull request description:
This picks up #15954
I fixed the code and added at a functional test utilizing the scripts in `contrib/linearize` as suggested by @MarcoFalke .
ACKs for top commit:
laanwj:
ACK 104f7de593
Tree-SHA512: acac9f285f9785fcbc3afc78118461e45bec2962f90ab90e9f82f3ad28adc90a44f0443b712458ccf486e46d891eb8a67f53e7bee5fa6d89e4387814fe03f117
b2ff500fb3 test: add "diamond" unit test to MempoolAncestryTests (Sebastian Falbesoner)
Pull request description:
Approaches #17271 (_Missing Unit Test for Ancestors "diamond"_).
If ancestors are represented more than once (in this case `ta` and `tb`), check that those are not overcounted.
ACKs for top commit:
laanwj:
ACK b2ff500fb3
Tree-SHA512: 82a6573cc7f0e82bf6fcfe207d7ddecbf297d2a203d22e95b73d887e3cb280f45a3c5f649161561c1be1eb560ff81b9b385868f205d1c12284211c2377e5ad99
To do so we update CValidationInterface::BlockDisconnect to take a
CBlockIndex pointing to the block being disconnected.
This new parameter will be use in the following commit to establish
wallet height.
1a8f0d5a74 [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
4de630354f [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)
Pull request description:
Introduce a Poisson helper method that wraps the existing method to return `std::chrono::duration` type, which is mockable.
Needed for https://github.com/bitcoin/bitcoin/pull/16698.
ACKs for top commit:
ajtowns:
ACK 1a8f0d5a74
MarcoFalke:
re-ACK 1a8f0d5a74
naumenkogs:
ACK 1a8f0d5, and let's merge it and come back to it later.
Tree-SHA512: 7e2325d7c55fc0b4357cb86b83e0c218ba269f678c1786342d8bc380bfd9696373bc24ff124b9ff17a6e761c62b2b44ff5247c3911e2afdc7cc5c20417e8290b
Suggested https://github.com/bitcoin/bitcoin/pull/17304#discussion_r341194391
by Gregory Sanders <gsanders87@gmail.com>
Reason for keeping the `return true` `return false` verbosity is that more code
will be added after the ReserveKeyFromKeyPool() call before returning.
4671fc3d9e Expand on wallet_balance.py comment from https://github.com/bitcoin/bitcoin/pull/16766\#issuecomment-527563982 (Jeremy Rubin)
91f3073f08 Update release notes to mention changes to IsTrusted and impact on wallet (Jeremy Rubin)
8f174ef112 Systematize style of IsTrusted single line if (Jeremy Rubin)
b49dcbedf7 update variable naming conventions for IsTrusted (Jeremy Rubin)
5ffe0d1449 Update comment in test/functional/wallet_balance.py (Jeremy Rubin)
a550c58267 Update wallet_balance.py test to reflect new behavior (Jeremy Rubin)
5dd7da4ccd Reuse trustedParents in looped calls to IsTrusted (Jeremy Rubin)
595f09d6de Cache tx Trust per-call to avoid DoS (Jeremy Rubin)
dce032ce29 Make IsTrusted scan parents recursively (Jeremy Rubin)
Pull request description:
This slightly modifies the behavior of IsTrusted to recursively check the parents of a transaction. Otherwise, it's possible that a parent is not IsTrusted but a child is. If a parent is not trusted, then a child should not be either.
This recursive scan can be a little expensive, so ~it might be beneficial to have a way of caching IsTrusted state, but this is a little complex because various conditions can change between calls to IsTrusted (e.g., re-org).~ I added a cache which works per call/across calls, but does not store the results semi-permanently. Which reduces DoS risk of this change. There is no risk of untrusted parents causing a resource exploitation, as we immediately return once that is detected.
This is a change that came up as a bug-fix esque change while working on OP_SECURETHEBAG. You can see the branch where this change is important here: https://github.com/bitcoin/bitcoin/compare/master...JeremyRubin:stb-with-rpc?expand=1. Essentially, without this change, we can be tricked into accepting an OP_SECURETHEBAG output because we don't properly check the parents. As this was a change which, on its own, was not dependent on OP_SECURETHEBAG, I broke it out as I felt the change stands on its own by fixing a long standing wallet bug.
The test wallet_balance.py has been corrected to meet the new behavior. The below comment, reproduced, explains what the issue is and the edge cases that can arise before this change.
# Before `test_balance()`, we have had two nodes with a balance of 50
# each and then we:
#
# 1) Sent 40 from node A to node B with fee 0.01
# 2) Sent 60 from node B to node A with fee 0.01
#
# Then we check the balances:
#
# 1) As is
# 2) With transaction 2 from above with 2x the fee
#
# Prior to #16766, in this situation, the node would immediately report
# a balance of 30 on node B as unconfirmed and trusted.
#
# After #16766, we show that balance as unconfirmed.
#
# The balance is indeed "trusted" and "confirmed" insofar as removing
# the mempool transactions would return at least that much money. But
# the algorithm after #16766 marks it as unconfirmed because the 'taint'
# tracking of transaction trust for summing balances doesn't consider
# which inputs belong to a user. In this case, the change output in
# question could be "destroyed" by replace the 1st transaction above.
#
# The post #16766 behavior is correct; we shouldn't be treating those
# funds as confirmed. If you want to rely on that specific UTXO existing
# which has given you that balance, you cannot, as a third party
# spending the other input would destroy that unconfirmed.
#
# For example, if the test transactions were:
#
# 1) Sent 40 from node A to node B with fee 0.01
# 2) Sent 10 from node B to node A with fee 0.01
#
# Then our node would report a confirmed balance of 40 + 50 - 10 = 80
# BTC, which is more than would be available if transaction 1 were
# replaced.
The release notes have been updated to note the new behavior.
ACKs for top commit:
ariard:
Code Review ACK 4671fc3, maybe extend DoS protection in a follow-up PR.
fjahr:
Code review ACK 4671fc3d9e
ryanofsky:
Code review ACK 4671fc3d9e. Changes since last review: 2 new commits adding suggested release note and python test comment, also a clean rebase with no changes to the earlier commits. The PR description is more comprehensive now, too. Looks good!
promag:
Code review ACK 4671fc3d9e.
Tree-SHA512: 6b183ff425304fef49724290053514cb2770f4a2350dcb83660ef24af5c54f7c4c2c345b0f62bba60eb2d2f70625ee61a7fab76a7f491bb5a84be5c4cc86b92f
436ad43643 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas)
Pull request description:
Closes#8752 by bringing back abandoned #10470.
This now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash and add a functional test to prevent this in the future.
For more context, #8757 was closed in favor of #10470.
ACKs for top commit:
instagibbs:
utACK 436ad43643
kallewoof:
utACK 436ad43643
jonatack:
I'm not qualifed to give an ACK here but 436ad43643 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp:
Tree-SHA512: 63d75cd3d3f19fc84dc38899b200c96179b82b24db263cd0116ee5b715265be647157855c2e35912d2fbc49c7b37db9375d6aab0ac672f0f09bece8431de5ea9
fa07b8beb5 test: Reset global args between test suites (MarcoFalke)
Pull request description:
Ideally there wouldn't be any globals in Bitcoin Core. However, as we still have globals, they need to be reset between runs of test cases. One way to do this is to run each suite in a different process. `make check` does that. However, `./src/test/test_bitcoin` when run manually or on appveyor is a single process, where all globals are preserved between test cases.
This leads to hard to debug issues such as https://github.com/bitcoin/bitcoin/pull/15845#pullrequestreview-310852164.
Fix that by resetting the global arg for each test suite. Note that this wont reset the arg between test cases, as the constructor/destructor is not called for them.
Addendum: This is not a general fix, only for `-segwitheight`. I don't know if clearing all args can be done with today's argsmanager. Nor do I know if it makes sense. Maybe we want datadir set to a temp path to not risk accidentally corrupting the default data dir?
ACKs for top commit:
laanwj:
ACK fa07b8beb5
practicalswift:
ACK fa07b8beb5
mzumsande:
ACK fa07b8beb5, I also tested that this fixes the issue in #15845.
Tree-SHA512: 1e30b06f0d2829144a61cc1bc9bdd6a694cbd911afff83dd3ad2a3f15b577fd30acdf9f1469f8cb724d0642ad5d297364fd5a8a2a9c8619a7a71fa9ae2837cdc
c98bd13e67 replace asserts in RPC code with CHECK_NONFATAL and add linter (Adam Jonas)
Pull request description:
- Replace instances of assert in /rpc files and rpcwallet with CHECK_NONFATAL(condition)
- Add a linter to prevent future usage of assert being used in RPC code
ref https://github.com/bitcoin/bitcoin/pull/17192
ACKs for top commit:
practicalswift:
ACK c98bd13e67 -- diff looks correct
Tree-SHA512: a16036b6bbcca73a5334665f66e17e1756377d582317568291da1d727fc9cf8c84bac9d9bd099534e1be315345336e5f7b66b93793135155f320dc5862a2d875
b6d2183858 Minor refactoring to remove implied m_addr_relay_peer. (User)
a552e8477c added asserts to check m_addr_known when it's used (User)
090b75c14b p2p: Avoid allocating memory for addrKnown where we don't need it (User)
Pull request description:
We should allocate memory for addrKnown filter only for those peers which are expected to participate in address relay.
Currently, we do it for all peers (including SPV and block-relay-only), which results in extra RAM where it's not needed.
Upd:
In future, we would still allow SPVs to ask for addrs, so allocation still will be done by default.
However, they will be able to opt-out via [this proposal](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-October/017428.html) and then we could save some more memory.
This PR still saves memory for block-relay-only peers immediately after merging.
Top commit has no ACKs.
Tree-SHA512: e84d93b2615556d466f5ca0e543580fde763911a3bfea3127c493ddfaba8f05c8605cb94ff795d165af542b594400995a2c51338185c298581408687e7812463
152b0a00d8 Refactor: Move nTimeFirstKey accesses out of CWallet (Andrew Chow)
7ef47b88e6 Refactor: Move GetKeypoolSize code out of CWallet (Andrew Chow)
089e17d45c Refactor: Move RewriteDB code out of CWallet (Andrew Chow)
0eac7088ab Refactor: Move SetupGeneration code out of CWallet (Andrew Chow)
f45d12b36c Refactor: Move HavePrivateKeys code out of CWallet::CreateWalletFromFile (Andrew Chow)
8b0d82bb42 Refactor: Move Upgrade code out of CWallet::CreateWalletFromFile (Andrew Chow)
46865ec958 Refactor: Move MarkUnusedAddresses code out of CWallet::AddToWalletIfInvolvingMe (Andrew Chow)
a18edd7b38 Refactor: Move GetMetadata code out of getaddressinfo (Andrew Chow)
9716bbe0f8 Refactor: Move LoadKey LegacyScriptPubKeyMan method definition (Andrew Chow)
67be6b9e21 Refactor: Move SetAddressBookWithDB call out of LegacyScriptPubKeyMan::ImportScriptPubKeys (Andrew Chow)
fc2867fdf5 refactor: Replace UnsetWalletFlagWithDB with UnsetBlankWalletFlag in ScriptPubKeyMan (Andrew Chow)
78e7cbc7ba Refactor: Remove UnsetWalletFlag call from LegacyScriptPubKeyMan::SetHDSeed (Andrew Chow)
0391aba52d Remove SetWalletFlag from WalletStorage (Andrew Chow)
4c5491f99c Refactor: Move SetWalletFlag out of LegacyScriptPubKeyMan::UpgradeKeyMetadata (Andrew Chow)
769acef857 Refactor: Move SetAddressBook call out of LegacyScriptPubKeyMan::GetNewDestination (Andrew Chow)
acedc5b823 Refactor: Add new ScriptPubKeyMan virtual methods (Andrew Chow)
533d8b364f Refactor: Declare LegacyScriptPubKeyMan methods as virtual (Andrew Chow)
b4cb18bce3 MOVEONLY: Reorder LegacyScriptPubKeyMan methods (Andrew Chow)
Pull request description:
Moves several more key management and metadata functions into LegacyScriptPubKeyMan from CWallet to further separate the two.
Note to reviewers: All of the `if (auto spk_man = walletInstance->m_spk_man.get()) {` blocks will be replaced with for loops in the next PR so you may see some things in those blocks that don't necessarily make sense with an `if` but will with a `for`.
ACKs for top commit:
laanwj:
code review ACK 152b0a00d8
Sjors:
re-ACK 152b0a00d8
promag:
Code review ACK 152b0a00d8.
Tree-SHA512: ff9872a3ef818922166cb15d72363004ec184e1015a3928a66091bddf48995423602ccd7e55b814de85d25ad7c69058280b1fde2e633570c680dc7d6084b3122
fa8919889f bench: Remove redundant copy constructor in mempool_stress (MarcoFalke)
29f8434368 refactor: Remove redundant PSBT copy constructor (Hennadii Stepanov)
Pull request description:
I fail to see why people add these copy constructors manually without explanation, when the compiler can generate them at least as good automatically with less code.
ACKs for top commit:
promag:
ACK fa8919889f.
hebasto:
ACK fa8919889f, nit s/constructor/operator/ in commit fa8919889f message, as @promag [mentioned](https://github.com/bitcoin/bitcoin/pull/17349#discussion_r341776389) above.
jonatack:
ACK fa8919889f
Tree-SHA512: ce024fdb894328f41037420b881169b8b1b48c87fbae5f432edf371a35c82e77e21468ef97cda6f54d34f1cf9bb010235d62904bb0669793457ed1c3b2a89723
fa0a731d00 test: Add RegTestingSetup to setup_common (MarcoFalke)
fa54b3e248 test: move-only ComputeFilter to src/test/lib/blockfilter (MarcoFalke)
Pull request description:
The default chain for `TestingSetup` is the main chain. However, any test that wants to mine blocks on demand needs to switch to regtest. This is done manually and in-line right now.
Fix that by creating an explicit `RegTestingSetup` and use it where appropriate.
Also, add a move-only commit to move `ComputeFilter` into the newly created unit test library.
Both commits are part of #15845, but split up because they are useful on their own.
ACKs for top commit:
practicalswift:
ACK fa0a731d00 -- diff looks correct
Tree-SHA512: 02b9765580b355ed8d1be555f8ae11fa6e3d575f5cb177bbdda0319378837e29de5555c126c477dc8a1e8a5be47335afdcff152cf2dea2fbdd1a988ddde3689b
ac831339cb doc: Fix some misspellings (randymcmillan)
Pull request description:
Here is a more thorough lint-spelling update.
This PR takes care of easy to fix spelling errors to clean up the linting stages.
There are misspellings coded into the functional tests.
That is a whole separate job within itself.
ACKs for top commit:
practicalswift:
ACK ac831339cb -- diff looks correct
Tree-SHA512: d8fad83fed083715655f148263ddeffc6752c8007d568fcf3dc2c418ccd5db70089ce3ccfd3994fcbd78043171402eb9cca5bdd5125287e22c42ea305aaa6e9d
f44abe4bed refactor: Remove addrdb.h dependency from node.h (Hennadii Stepanov)
Pull request description:
`node.h` includes `addrdb.h` just for the sake of `banmap_t` type.
This PR makes dependencies simpler and explicit.
~Also needless `typedef` has been removed from `enum BanReason`.~
ACKs for top commit:
laanwj:
ACK f44abe4bed
practicalswift:
ACK f44abe4bed
Tree-SHA512: 33a1be20e5c629daf4a61ebbf93ea6494b9256887cebd4974de4782f6d324404b6cc84909533d9502b2cc19902083f1f9307d4fb7231e67db5b412b842d13072
3ed8e3d079 doc: Remove explicit network name references (Fabian Jahr)
d6e493f0c2 wallet: Remove left-over BIP70 comment (Fabian Jahr)
Pull request description:
A small follow-up to #17165 which removed BIP70 support.
1. Removes one leftover mention of BIP70 in a comment.
2. Removes BIP70 reference in comments on network/chain name strings. These can be removed as they are not really helpful and also incorrect: BIP70 only defines "main" and "test" but not "regtest". If/When signet gets merged we will add another name to the list that is not defined in BIP70. Mostly there is also an exhaustive list of the options included in the comment anyway.
If we would like to keep an identifier for this naming scheme, I would suggest switching to something more generic, like 'short chain name'. Happy to implement that if that is preferred. Alternatively, we could add a reference to `CBaseChainParams`. That would also mean we don't have to change these lines again for signet.
ACKs for top commit:
MarcoFalke:
ACK 3ed8e3d079
Tree-SHA512: 9a7c0b9cacbb67bd31a089ffdc6f1ebc7f336493e2c8266eb697da34dce2b505a431d5639a3e4fc34f9287361343e861b55dc2662e0a1d2095cc1046db77d6ee
a35b6824f3 Add assertion to randrange that input is not 0 (Jeremy Rubin)
Pull request description:
From the comment in randrange, their is an implicit argument that randrange cannot accept an argument of 0. If the argument is 0, then we have to return {}, which is not possible in a uint64_t.
The current code takes a very interesting approach, which is to return [0..std::numeric_limits<uint64_t>]. This can cause all sorts of fun problems, like allocating a lot of memory, accessing random memory (maybe with your private keys), and crashing the computer entirely.
This gives us three choices of how to make it "safe":
1) return Optional<uint64_t>
2) Change the return type to [0..range]
3) Return 0 if 0
4) Assert(range)
So which solution is best?
1) seems a bit overkill, as it makes any code using randrange worse.
2) Changing the return type as in 2 could be acceptable, but it imposes the potential overflow checking on the caller (which is what we want).
3) An interesting option -- effective makes the return type in {0} U [0..range]. But this is a bad choice, because it leads to code like `vec[randrange(vec.size())]`, which is incorrect for an empty vector. Null set should mean null set.
4) Assert(range) stands out as the best mitigation for now, with perhaps a future change to solution 2. It prevents the error from propagating at the earliest possible time, so the program crashes cleanly rather than by freezing the computer or accessing random memory.
ACKs for top commit:
instagibbs:
Seems reasonable for now, ACK a35b6824f3
laanwj:
ACK a35b6824f3
promag:
ACK a35b6824f3.
Tree-SHA512: 8fc626cde4b04b918100cb7af28753f25ec697bd077ce0e0c640be0357626322aeea233e3c8fd964ba1564b0fda830b7f5188310ebbb119c113513a4b89952dc
ScriptPubKeyMan is only using UnsetWalletFlagWithDB to unset the blank
wallet flag. Just make that it's own function and not expose the flag
writing directly.
This does not change behavior.
The default (i.e., generated by a compiler) copy constructor does the
same things.
Also this prevents -Wdeprecated-copy warning for implicitly declared
operator= in GCC 9.
b0c774b48a Add new mempool benchmarks for a complex pool (Jeremy Rubin)
Pull request description:
This PR is related to #17268.
It adds a mempool stress test which makes a really big complicated tx graph, and then, similar to mempool_eviction test, trims the size.
The test setup is to make 100 original transactions with Rand(10)+2 outputs each.
Then, 800 times:
we create a new transaction with Rand(10) + 1 parents that are randomly sampled from all existing transactions (with unspent outputs). From each such parent, we then select Rand(remaining outputs) +1 50% of the time, or 1 outputs 50% of the time.
Then, we trim the size to 3/4. Then we trim it to just a single transaction.
This creates, hopefully, a big bundle of transactions with lots of complex structure, that should really put a strain on the mempool graph algorithms.
This ends up testing both the descendant and ancestor tracking.
I don't love that the test is "unstable". That is, in order to compare this test to another, you really can't modify any of the internal state because it will have a different order of invocations of the deterministic randomness. However, it certainly suffices for comparing branches.
Top commit has no ACKs.
Tree-SHA512: cabe96b849b9885878e20eec558915e921d49e6ed1e4b011b22ca191b4c99aa28930a8b963784c9adf78cc8b034a655513f7a0da865e280a1214ae15ebb1d574
5710dadf9b test: fix script_p2sh_tests OP_PUSHBACK2/4 missing (kodslav)
Pull request description:
Cleans up #15140 which fixes commit 6b25f29a91 where opcodes were lost in translation.
ACKs for top commit:
laanwj:
code review ACK 5710dadf9b
Tree-SHA512: 3f7fbcaf0dd199626d9ec9fdf3c5b5c5c2a91c4cfe81fae5b1d5662a48e52cf4bd27c94f8f42ebdfe7a076c5d600ada5661a6902b03eb5dc3dc953f4524345ac
8734c856f8 Replace the LogPrint function with a macro (Jeffrey Czyz)
Pull request description:
Calling `LogPrint` with a category that is not enabled results in
evaluating the remaining function arguments, which may be arbitrarily
complex (and possibly expensive) expressions. Defining `LogPrint` as a
macro prevents this unnecessary expression evaluation.
This is a partial revert of #14209. The decision to revert is discussed
in #16688, which adds verbose logging for validation event notification.
ACKs for top commit:
jnewbery:
ACK 8734c856f8
Tree-SHA512: 19e995eaef0ff008a9f8c1fd6f3882f1fbf6794dd7e2dcf5c68056be787eee198d2956037d4ffba2b01e7658b47eba276cd7132feede78832373b3304203961e
a8b82867d5 Fix incorrect help-debug for -checkpoints (Antoine Riard)
Pull request description:
ACKs for top commit:
jnewbery:
ACK a8b82867d5 for improving the `-prune` help text.
MarcoFalke:
ACK a8b82867d5
Tree-SHA512: 973fa97436be09a9939386dc00023420a7296a9e268356bf26aa06468f9f0d2c822205a4f1ce8f44a0562aa64ad90a43dec5697af656ef28ba6829e4e4360e94
9cae3d5e94 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)
Pull request description:
The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.
This is a follow-up to #17235 which accidentally broke those two fuzzers.
Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.
Top commit has no ACKs.
Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
d314e8a818 refactor: Replace all uses of boost::optional with our own Optional type (Wladimir J. van der Laan)
Pull request description:
Replace all uses of boost::optional with our own Optional type. Luckily, there aren't so many.
After this:
- `boost::optional` is no longer used directly (only through `Optional` which is an alias for it)
- `boost/optional.hpp` is only included in one place
ACKs for top commit:
MarcoFalke:
ACK d314e8a818
practicalswift:
ACK d314e8a818 -- diff looks correct + satisfying to see incremental progress towards the goal of a Boost free future :)
jtimon:
ACK d314e8a818
fanquake:
ACK d314e8a818
Tree-SHA512: b43e0017af81b07b5851377cd09624f114510ac5b9018d037664b58ad0fc8e893e30946b61f8f5e21e39125925bf9998a81f2226b468aab2df653ee57ed3213d
Instead of keeping pindexBestHeader set to the best header we've
ever seen, reset it back to our validated tip if we find an ancestor
of it turns out to be invalid. While the name is now a bit confusing,
this matches much better with how it is used in practice, see below.
Further, this opens up more use-cases for it in the future, namely
aggressively searching for new peers in case we have discovered
(possibly via some covert channel) headers which we do not know to be
invalid, but which we cannot find block data for.
Places pindexBestHeader is used:
* Various GUI displays of the best header and getblockchaininfo["headers"],
I don't think changing this is bad, and if anything this is less confusing
in the presence of an invalid block.
* IsCurrentForFeeEstimation(): If anything I think ensuring pindexBestHeader
isn't some crazy invalid chain is better than the alternative, even in the
case where you are rejecting the current chain due to hardware error (since
hopefully in that case you won't get any new blocks anyway).
* ConnectBlock assumevalid checks: We use pindexBestHeader to check that the
block we're connecting leads to something with nMinimumChainWork (preventing
a user-set assumevalid from having bogus work) and that the block we're
connecting leads to pindexBestHeader (I'm not too worried about this one -
it's nice to "disable" assumevalid if we have a long invalid headers chain,
but I don't see it as a critical protection).
* BlockRequestAllowed() uses pindexBestHeader as its target to ensure the
requested block is within a month of the "current chain". I don't think this
is a meaningful difference, if we're rejecting the current tip we're
trivially fingerprintable anyway, and if the chain really does have a bunch
of invalid crap near the tip, using the best not-invalid header is likely a
better criteria.
* ProcessGetBlockData uses pindexBestHeader as the "current chain" definition
of whether a block request is "historical" for the purpose of bandwidth
limiting. Similarly, I don't see why this is a meaningful change.
* We use pindexBestHeader for requesting missing headers on receipt of a
headers/compact block message or block inv as well as for initial getheaders.
I think this is definitely wrong, using the best not-invalid header for such
requests is much better.
* We use pindexBestHeader to define the "current chain" for deciding when
we're close to done with initial headers sync. I don't think this is a
meaningful change.
* We use pindexBestHeader to decide if initial headers sync has timed out. If
we're rejecting the chain due to hardware error this may result in
additional cases where we ban a peer, but this is already true, so I think
its fine.
3004d5a12d [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5b [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b31 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e492 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed [validation] Add CValidationState subclasses (John Newbery)
Pull request description:
Carries out some remaining tidy-ups remaining after PR 15141:
- split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
- various minor code style tidy-ups to the ValidationState class
- remove the useless `ret` parameter from `ValidationState::Invalid()`
- remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()`
- remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object.
Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:
Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below.
```sh
git checkout <CommitHash>
git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
git diff HEAD^
```
After that it's possible to easily see the mechanical changes with:
```sh
git log -p -n1 -U0 --word-diff-regex=. <CommitHash>
```
ACKs for top commit:
laanwj:
ACK 3004d5a12d
amitiuttarwar:
code review ACK 3004d5a12d. Also built & ran tests locally.
fjahr:
Code review ACK 3004d5a12d . Only nit style change and pure virtual destructor added since my last review.
ryanofsky:
Code review ACK 3004d5a12d. Just whitespace change and pure virtual destructor added since last review.
Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
fa144e6fde rpc: Add generatetodescriptor (MarcoFalke)
Pull request description:
The existing `generatetoaddress` RPC can only generate to scriptPubKeys that can be represented by an address. However, raw scripts (such as `OP_TRUE`) or P2PK can not be represented by an address, which complicates testing.
ACKs for top commit:
laanwj:
ACK fa144e6fde
Tree-SHA512: aee934ab7e33f07c81f3b4c8ec23e7b6ddf63a1f4b86051af0bd76b75d8da1f51627cc682e5c6e42582340ca576bbf8ff724bdd43f87128ccecfa91e52d30ae7
After this:
- `boost::optional` is no longer used directly (only through `Optional`
which is an alias for it)
- `boost/optional.hpp` is only included in one place
31879345ee cli: Add "headers" and "verificationprogress" to -getinfo (Wladimir J. van der Laan)
Pull request description:
These values are useful to know the current progress of initial sync, or of catching up, which is arguably the use of a quick `-getinfo` command.
ACKs for top commit:
MarcoFalke:
unsigned ACK 31879345ee
jonasschnelli:
utACK 31879345ee
jonatack:
Tested ACK 31879345ee on Debian 4.19.37-5+deb10u2 (2019-08-08) x86_64 GNU/Linux
Tree-SHA512: 185180ab426b4db5d99eb208ee88d1606f585361875ba3a92b6c28a74fe181d72ed710c8859b969ba49b1ca7d2385695932b79ff621c7a2a7cedd0df717a99ed
362ded410b Avoid using g_rpc_node global in wallet code (Russell Yanofsky)
8922d7f6b7 scripted-diff: Remove g_connman, g_banman globals (Russell Yanofsky)
e6f4f895d5 Pass NodeContext, ConnMan, BanMan references more places (Russell Yanofsky)
4d5448c76b MOVEONLY: Move NodeContext struct to node/context.h (Russell Yanofsky)
301bd41a2e scripted-diff: Rename InitInterfaces to NodeContext (Russell Yanofsky)
Pull request description:
This change is mainly a naming / organization change intended to simplify #10102. It:
- Renames struct InitInterfaces to struct NodeContext and moves it from
src/init.h to src/node/context.h. This is a cosmetic change intended to make
the point of the struct more obvious.
- Gets rid of BanMan and ConnMan globals making them NodeContext members
instead. Getting rid of these globals has been talked about in past as a way
to implement testing and simulations. Making them NodeContext members is a
way of keeping them accessible without the globals.
- Splits g_rpc_interfaces global into g_rpc_node and g_rpc_chain globals. This
better separates node and wallet rpc methods. Node RPC methods should have
access NodeContext, while wallet RPC methods should only have indirect access
to node functionality via interfaces::Chain.
- Adds NodeContext& references to interfaces::Chain class and the
interfaces::MakeChain() function. This is needed to access ConnMan and BanMan
instances without the globals.
- Gets rid of redundant Node and Chain instances in Qt tests. This is
needed due to the previous MakeChain change, and also makes test setup a
little more straightforward. More cleanup could be done in the future, but it
will require deduplication of bitcoind, bitcoin-qt, and TestingSetup init
code.
ACKs for top commit:
laanwj:
ACK 362ded410b
Tree-SHA512: 9ae6ff1e33423291d1e52056bac95e0874538390892a6e83c4c115b3c73155a8827c0191b46eb3d14e3b3f6c23ccb08095490880fbc3188026319c71739f7db2
37b8475dcf Chainparams: Use name constants in chainparams initialization (Jorge Timón)
Pull request description:
I thought this wouldn't work for some reason, but it seems it does.
Just a little bit more consistency. I'm still not able to use them in qt/networkstyle.cpp though, not sure why.
ACKs for top commit:
MarcoFalke:
ACK 37b8475dcf
laanwj:
ACK 37b8475dcf
hebasto:
ACK 37b8475dcf, I have reviewed the code and it looks OK, I agree it can be merged.
fjahr:
ACK 37b8475
Tree-SHA512: d9fa5df5650e10c645ac1f3afe831674a47f35d4a649e18a3d2aee1d04b08e6896aff6f1bbed0630d28775c51f989f9daaa9e405c9f3d7dca30e639a6f9008f0
f3b51eb935 Fix occurences of c_str() used with size() to data() (Wladimir J. van der Laan)
Pull request description:
Using `data()` better communicates the intent here.
~~Also, depending on how `c_str()` is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents (or even be inaccessible, worst case).~~ Apparently [this is no longer an issue with C++11](https://github.com/bitcoin/bitcoin/pull/17281#discussion_r339742128).
ACKs for top commit:
fjahr:
Code review ACK f3b51eb
practicalswift:
ACK f3b51eb935 -- diff looks correct, `data()` more idiomatic
ryanofsky:
Code review ACK f3b51eb935. Most of these calls (including one in crypter.cpp) are passing text strings, not binary strings likely to contain `\0` and were probably safe before, but much better to avoid the possibility of bugs like this.
Tree-SHA512: 842e1bdd37efc4ece2ecb87ca34962aafef0a192180051def630607e349dc9c8b4e562481fff3de474515f493b4ee3ea53b00269a801a66e625326a38dfce5b8
c1c6c410a6 test: add reason checks for non-standard txs in test_IsStandard (Sebastian Falbesoner)
Pull request description:
While taking a look at #17272 I noticed that for some reason the unit test `test_IsStandard` (which was not adapted to the policy change in the referenced PR commits) didn't fail as expected:
6a97e8a060/src/test/transaction_tests.cpp (L758-L762)
It turned out that `IsStandardTx()` returned `"dust"` as rejection reason (instead of the expected `"multi-op-return"`), leading to the conclusion that 5fe6f052bd erroneously performs the `IsDust()` check also for TX_NULL_DATA transactions. To avoid cases like this in the future, this PR makes the unit test `test_IsStandard` more strict by also checking for the concrete reason after each occurence of `IsStandardTx()` returning false.
ACKs for top commit:
instagibbs:
utACK c1c6c410a6
Tree-SHA512: c7419884cc52977c73f8f8c476eaebed80ba7bda4d03509d3f46dd977be911389f7b53daefa5ef31d2f7df9402243152e01e83f1b8a9fb300c19d1a0f69a89a9
ValidationState::Invalid() takes a parameter `ret` which is returned to
the caller. All call sites set this to false. Remove the `ret` parameter
and just return false always.
This is in preparation for the next commit, which removes the useless
`ret` parameter from ValidationState::Invalid().
error() is simply a convenience wrapper that calls LogPrintf and returns
false. Call LogPrintf explicitly and substitute the error() call for a
false bool literal.
