7e3ee4cdd0f60a2f549ba030fe96b90d61c036c5 GUI: Ask user to unlock wallet before signing psbt (Samuel Dobson)
0f3acecf3372f9da143590bb17d8444564e083f4 Add test that walletprocesspsbt requires unlocked wallet when signing (Samuel Dobson)
0e895212bb571ae0de5580adfd8ee9b3c2137e24 Ensure wallet is unlocked before signing in walletprocesspsbt (Samuel Dobson)
Pull request description:
If signing a PSBT, we need to ensure the wallet is unlocked.
Fixes#22874, fixesbitcoin-core/gui#312
ACKs for top commit:
achow101:
ACK 7e3ee4cdd0f60a2f549ba030fe96b90d61c036c5
lsilva01:
Code Review ACK 7e3ee4cdd0
benthecarman:
ACK 7e3ee4cdd0f60a2f549ba030fe96b90d61c036c5
Tree-SHA512: 6726a873582747900ab454ea21153a92be86808a4c1517dc2856b389876a2da9e8df1ffa9b567b6bd017038342c3544ecf5ca3c97744e7debe0a5ee65563687d
efcaefc7b5ffe0495e7c809032342ee5ca4841be test: Add remaining scenarios of 0 waste (rajarshimaitra)
Pull request description:
As per the [review club](https://bitcoincore.reviews/22009) discussion on #22009 , it was observed that there were other two fee scenarios in which selection waste could be zero.
These are:
- (LTF - Fee) == Change Cost
- (LTF - Fee) == Excess
Even though these are obvious by the definition of waste metric, adding tests for them can be helpful in explaining its behavior
to new readers of the code base, along with pinning the behavior for future.
This PR adds those two cases to waste calculation unit test.
Also let me know if I am missing more scenarios.
ACKs for top commit:
jonatack:
Tested re-ACK efcaefc7b5ffe0495e7c809032342ee5ca4841be
achow101:
ACK efcaefc7b5ffe0495e7c809032342ee5ca4841be
meshcollider:
ACK efcaefc7b5ffe0495e7c809032342ee5ca4841be
Tree-SHA512: 13fe3e2c0ea7bb58d34e16c32908b84705130dec16382ff941e5e60ca5b379f9c5811b33f36c4c72d7a98cfbb6af2f196d0a69e96989afa4b9e49893eaadd7cb
182de7ba10811ec39e24ec5bec7cd2119f776f2f ci: update minimum compiler requirements for std::filesystem (fanquake)
04f5bafb7b7d3f86a5db3047a77591e0ec272e68 doc: update minimum compiler requirements for std::filesystem (fanquake)
Pull request description:
This increases the minimum required compiler versions to Clang 7 and GCC 8.1. This has been split out of #20744 (migration to `std::filesystem`), as it's also a requirement for some other changes, such as #20452 or #20457 which want to make use of `std::from_chars`. As well as #20435, which is also `std::filesystem` related. Given that the `std::filesystem` changes are moving ahead, splitting out this change to let other PRs take advantage of the new requirements seems worthwhile.
Clang 7 has been available in Debian since [Stretch (oldoldstable)](https://packages.debian.org/stretch/clang-7) and in Ubuntu since [Bionic (18.04)](https://packages.ubuntu.com/bionic-updates/clang-7). GCC 8 has been available in Debian since [Buster (oldstable)](https://packages.debian.org/buster/gcc) and in Ubuntu since [Bionic (18.04)](https://packages.ubuntu.com/bionic/gcc-8). CentOS 8 also packages GCC 8.
The CI changes here give us one build with GCC 8, and another using Clang 7 on top of libc++.
Note that the minimum required libc++ in dependencies.md is unchanged as, at least for `<filesystem>`, and the `*_chars` use cases, libc++ 7 [should be sufficient](https://en.cppreference.com/w/cpp/compiler_support/17).
I've tested that building `<filesystem>` code using Clang 7 & libc++ works. i.e `clang++-7 -std=c++17 fs.cpp -stdlib=libc++ -lc++fs`. Also that building `<filesystem>` code with Clang 7 and libstdc++ 8 works. i.e `clang++-7 -std=c++17 fs.cpp -lstdc++fs`.
ACKs for top commit:
MarcoFalke:
review ACK 182de7ba10811ec39e24ec5bec7cd2119f776f2f
Tree-SHA512: 5bc151c4be58005711eed6bd8a091f3417f75a0218c11c08cffff9d749edadd965726bb7856a8e693e96e69ed0596989cda1aac4b29fb6d30705b1687a5b3363
d96b000e94d72d041689c5c47e374df2ebc0e011 Make GUI UTXO lock/unlock persistent (Samuel Dobson)
077154fe698f5556ad6e26ef49c9024c2f07ff68 Add release note for lockunspent change (Samuel Dobson)
719ae927dcdb60c0f9902fa79796256035228c4e Update lockunspent tests for lock persistence (Samuel Dobson)
f13fc16295c19a156f2974d2d73fba56d52fc161 Allow lockunspent to store the lock in the wallet DB (Samuel Dobson)
c52789365e5dbcb25aa5f1775de4d318da79e5a7 Allow locked UTXOs to be store in the wallet database (Samuel Dobson)
Pull request description:
Addresses and closes#22368
As per that issue (and its predecessor #14907), there seems to be some interest in allowing unspent outputs to be locked persistently. This PR does so by adding a flag to lockunspent to store the change in the wallet database. Defaults to false, so there is no change in default behaviour.
Edit: GUI commit changes default behaviour. UTXOs locked/unlocked via the GUI are now persistent.
ACKs for top commit:
achow101:
ACK d96b000e94d72d041689c5c47e374df2ebc0e011
kristapsk:
ACK d96b000e94d72d041689c5c47e374df2ebc0e011
lsilva01:
Tested ACK d96b000e94 on Ubuntu 20.04
prayank23:
ACK d96b000e94
Tree-SHA512: 957a5bbfe7f763036796906ccb1598feb6c14c5975838be1ba24a198840bf59e83233165cb112cebae909b6b25bf27275a4d7fa425923ef6c788ff671d7a89a8
097ac74fd22f706fb7d8d652dadea844dc8198b9 ci: Add more functional tests to the native Windows task (Hennadii Stepanov)
8e08a4b6ce13dcf20c026c832dc5810a54c8390b ci: Increase the dynamic port range to the maximum on native Windows (Hennadii Stepanov)
Pull request description:
Fixes#18548.
The solution suggested in https://github.com/bitcoin/bitcoin/issues/18548#issuecomment-923944390.
Also more functional tests added to the native Windows task.
ACKs for top commit:
MarcoFalke:
concept ACK 097ac74fd22f706fb7d8d652dadea844dc8198b9
Tree-SHA512: 51f3cb5b4e707dd2e4fd6b3be7e3b9615ec4a2760f4e0de9312443edadedf148e94964c43352fd2c8f6842a2baf70084b559760e7b46fc654ea882b0bba21443
fa04f26aa77d2cf746db4a8a43068b7c5c9cd02b test: Avoid race after connect_nodes (MarcoFalke)
Pull request description:
Wait until the connection is fully established on both sides (verack). Fixes#22714
ACKs for top commit:
kiminuo:
utACK fa04f26aa77d2cf746db4a8a43068b7c5c9cd02b
Tree-SHA512: bc2c44b44b688086ff84046924cf5251dd625584e93ce8fa17de27023855b32f3bb55109b846abbcec775e2836c7f3c5a81d6b4aff7c4ac065b9aefa044c1883
e148a5233292d156cda76cb20afb6641fc20f25e bench: fixed ubsan implicit conversion (Martin Ankerl)
da4e2f1da0388d424659fa8c853fcaf37b4b5959 bench: various args improvements (Jon Atack)
d312fd94a1083cdbf071f2888aab43c62d358151 bench: clean up includes (Jon Atack)
1f10f1663e53474038b9111c4264a250cffe7501 bench: add usage description and documentation (Martin Ankerl)
d3c6f8bfa12f78635752878b28e66cec0c85d4a9 bench: introduce -min_time argument (Martin Ankerl)
9fef8329322277d9c14c8df1867cb3c61477c431 bench: make EvictionProtection.* work with any number of iterations (Martin Ankerl)
153e6860e84df0a3d52e5a3b2fe9c37b5e0b029a bench: change AddrManGood to AddrManAddThenGood (Martin Ankerl)
468b232f71562280aae16876bc257ec24f5fcccb bench: remove unnecessary & incorrect multiplication in MuHashDiv (Martin Ankerl)
eed99cf272426e5957bee35dc8e7d0798aec8ec0 bench: update nanobench from 4.3.4 to 4.3.6 (Martin Ankerl)
Pull request description:
This PR updates the nanobench with the latest release from upstream, v4.3.6. It fixes the missing performance counters.
Due to discussions on #22999 I have done some work that should make the benchmark results more reliable. It introduces a new flag `-min_time` that allows to run a benchmark for much longer then the default. When results are unreliable, choosing a large timeframe here should usually get repeatable results even when frequency scaling cannot be disabled. The default is now 10ms. For this to work I have changed the `AddrManGood` and `EvictionProtection` benchmarks so they work with any number of iterations.
Also, this adds more usage documentation to `bench_bitcoin -h` and I've cherry-picked two changes from #22999 authored by Jon Atack
ACKs for top commit:
jonatack:
re-ACK e148a5233292d156cda76cb20afb6641fc20f25e
laanwj:
Code review ACK e148a5233292d156cda76cb20afb6641fc20f25e
Tree-SHA512: 2da6de19a5c85ac234b190025e195c727546166dbb75e3f9267e667a73677ba1e29b7765877418a42b1407b65df901e0130763936525e6f1450f18f08837c40c
fa4db8671bb604e11b43a837f91de8866226f166 test: Activate all regtest softforks at height 1, unless overridden (MarcoFalke)
faad1e5ffda255aecf1b0ea2152cd4f6805e678f Introduce -testactivationheight=name@height setting (MarcoFalke)
fadb2ef2fa8561882db463f35df9b8a0e9609658 test: Add extra_args argument to TestChain100Setup constructor (MarcoFalke)
faa46986aaec69e4cf016101ae517ce8778e2ac5 test: Remove version argument from build_next_block in p2p_segwit test (MarcoFalke)
fa086ef5398b5ffded86e4f0d6633c523cb774e9 test: Remove unused ~TestChain100Setup (MarcoFalke)
Pull request description:
All softforks that are active at the tip of mainnet, should also be active from genesis in regtest. Otherwise their rules might not be enforced in user testing, thus making their testing less useful.
To still allow tests to check pre-softfork rules, a runtime argument can change the activation height.
ACKs for top commit:
laanwj:
Code review ACK fa4db8671bb604e11b43a837f91de8866226f166
theStack:
re-ACK fa4db8671bb604e11b43a837f91de8866226f166
Tree-SHA512: 6397d46ff56ebc48c007a4cda633904d6ac085bc76b4ecf83097c546c7eec93ac0c44b88083b2611b9091c8d1fb8ee1e314065de078ef15e922c015de7ade8bf
There are two more cases where waste can be 0, when:
- (Fee - LTF) == -Change Cost
- (Fee - LTF) == -Excess
Adding these two conditions explicitly in the unit test will help
pin the behavior, also demonstrate waste calculation scenarios to new
readers.
673a5bd3377929a0a6a62eda8b560e47bc2cca0c test: validation: add unittest for UpdateTip behavior (James O'Beirne)
2705570109a2a90ecfd3f4180944498626fc2707 test: refactor: separate CreateBlock in TestChain100Setup (James O'Beirne)
298bf5d563cc740c6ae71750d86942e0278b22d6 test: refactor: declare NoMalleation const auto (James O'Beirne)
071200993f3a9412821ce5387851d659baf85327 move-only: unittest: add test/util/chainstate.h (James O'Beirne)
8f5710fd0ac5173b577e5d00708485170b321bcc validation: fix CheckBlockIndex for multiple chainstates (James O'Beirne)
5a807736dacfc3e6fa57231219336acf08be38fb validation: insert assumed-valid block index entries into candidates (James O'Beirne)
01a9b8fe719efab2c268dc738bc93cfbdf92edb7 validation: set BLOCK_ASSUMED_VALID during snapshot load (James O'Beirne)
42b2520db93fd9feb3df4101654391fa7d3e2140 chain: add BLOCK_ASSUMED_VALID for use with assumeutxo (James O'Beirne)
b217020df78bc981d221fe04497c831120ef969f validation: change UpdateTip for multiple chainstates (James O'Beirne)
665072a36df2e4c88705fedd4ac7c955d7f6a488 doc: add comment for g_best_block (James O'Beirne)
ac4051d891e2d5c8ac130da16b85b9d880b44720 refactor: remove unused assumeutxo methods (James O'Beirne)
9f6bb539359b98d5b39482ab8a28a68608f0c645 validation: add chainman ref to CChainState (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Modify UpdateTip and CheckBlockIndex for use with multiple chainstates. Includes a new unittest verifying `g_best_block` behavior (previously untested at the unit level) and various changes necessary for running and testing `ProcessNewBlock()`-like behavior on the background validation chainstate.
This changeset introduces a new block index `nStatus` flag called `BLOCK_ASSUMED_VALID`, and it is applied to block index entries that are beneath the UTXO snapshot base block upon snapshot load. Once each block is validated (during async background validation), the flag is removed. This allows us to avoid (ab)using `BLOCK_VALID_*` flags for snapshot chain block entries, and preserves the original meaning of those flags.
Note: this PR previously incorporated changes to `LoadBlockIndex()` and `RewindBlockIndex()` as noted in Russ' comments below, but once I generated the changes necessary to test the UpdateTip change, I decided to split this changes out into another PR due to the size of this one.
ACKs for top commit:
achow101:
ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c
jonatack:
Code-review re-ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c reviewed diff, rebased to master/debug build/ran unit+functional tests
naumenkogs:
ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c
fjahr:
Code review ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c
ariard:
utACK 673a5bd3
ryanofsky:
Code review ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c. Just linker fix and split commit changes mentioned https://github.com/bitcoin/bitcoin/pull/21526#issuecomment-921064563 since last review
benthecarman:
ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c
Tree-SHA512: 0a6dc23d041b27ed9fd0ee1f3e5971b92fb1d2df2fc9b655d5dc48594235321ab1798d06de2ec55482ac3966a9ed56de8d56e9e29cae75bbe8690bafc2dda383
64e1ddd255771e57a88a20f07dbde04a83bf0c75 log: call LogPrint only once with time data samples (Martin Zumsande)
Pull request description:
When timedata samples are logged, `LogPrint()` is currently invoked multiple times on the same log entry.
This can lead to chaos in the log when other threads log concurrently, as in this example which motivated this PR:
```
2021-09-20T00:28:57Z -48 -26 -11 -8 -6 Addrman checks started: new 37053, tried 83, total 37136
2021-09-20T00:28:57Z -3 -1 -1 -1 -1 +0 | nTimeOffset = -3 (+0 minutes)
```
Fix this by building the log message in a string and logging it one `LogPrint()` call. I also changed the wording slightly so that it becomes understandable what is being logged, example:
```
2021-09-21T21:03:24Z time data samples: -43 -18 -12 -4 -1 -1 +0 +0 +268 | median offset = -1 (+0 minutes)
```
ACKs for top commit:
jnewbery:
Tested ACK 64e1ddd255
laanwj:
Tested ACK 64e1ddd255771e57a88a20f07dbde04a83bf0c75, new message lgtm
Tree-SHA512: ffb7a93166cc8fd6a39200b9e03a9d1e8e975b7ded822ccddd015f553258b991162a5cb867501f426d3ebcfef4f32f0e06e17b18e6b01486b967595d102f8379
ab278007991b912299eaf794d87a636423521d27 log: Remove unnecessary timing logs for Callbacks bench (Douglas Chimento)
Pull request description:
Logging of Callbacks are no longer needed and records times that are not relevant for performance analysis.
resolves#23071
ACKs for top commit:
laanwj:
Thanks. re-ACK ab278007991b912299eaf794d87a636423521d27
jonatack:
Code review ACK ab278007991b912299eaf794d87a636423521d27
Tree-SHA512: be1ea780c4db9407a8799065a8824b9d3610abac72af5907809ed62d493d5a54e65735de45ec5fdd0edb85ef21ec6036105abe8ca00093942980f6f92e7fec50
f2747d1602ec4e1128356b861b2167daf66a845b build: Restrict check for CRC32C intrinsic to aarch64 (W. J. van der Laan)
Pull request description:
`crc32c`'s hardware accelerated code doesn't handle ARM 32-bit at all, but only 64-bit. Make the check in `configure.ac` check for this architecture explicitly. This change does not affect non-ARM architectures.
For the release binaries, the current `configure.ac` check happens to work: it enables it on aarch64 but disables it for armhf. However some combination of compiler version and settings can cause this check to succeed on armhf (as reported on IRC). So make the 64-bit platform requirement explicit.
(details: while the check already explicitly checks the `__crc32d` intrinsic, which strictly doesn't exist on 32-bit ARM, this is not enough! gcc happens to helpfully emulate it:
> These built-in intrinsics for the ARMv8-A CRC32 extension are available when the -march=armv8-a+crc switch is used "uint32_t __crc32d (uint32_t, uint64_t)" Form of expected instruction(s): Two crc32w r0, r0, r0 instructions for AArch32
)
ACKs for top commit:
luke-jr:
re-tACK f2747d1602ec4e1128356b861b2167daf66a845b
jarolrod:
ACK f2747d1602ec4e1128356b861b2167daf66a845b
Tree-SHA512: e5f05f05eeec310ac42685621d86862735586be66dc378db404ec9414ac5aaea7c53d76d76d875b15b11924eee6714076120c07b077183fd7af898704fd81823
b69a106bcd8ddefdb810df6ebb3625c430197e04 test: use test_framework.p2p P2P_SERVICES in functional tests (Jon Atack)
Pull request description:
`P2P_SERVICES` is defined in `test/functional/test_framework/p2p.py`, so we can use it as a single definition for our functional tests. It may also be a tiny bit more efficient to use the constant rather than calculating `NODE_NETWORK | NODE_WITNESS` every time we need it in the tests.
ACKs for top commit:
laanwj:
Code review ACK b69a106bcd8ddefdb810df6ebb3625c430197e04
klementtan:
crACK b69a106bcd8ddefdb810df6ebb3625c430197e04
fanquake:
ACK b69a106bcd8ddefdb810df6ebb3625c430197e04 - didn't look at the formatting changes.
Tree-SHA512: f83e593663a69182986325d9ba2b4b787b87896d6648973f4f802f191a2573201b9e7d7e10e69662ef1965fa63268845726ed1aa5742a2e38dcccf4aebc6a961
0ef08f8bed537435f3f9db1e38b7d6f3551fe830 add missing includes in policy/rbf (glozow)
c6abeb76fbb877f3f16d699c73a1828c7da2e6d1 make MAX_BIP125_RBF_SEQUENCE constexpr (glozow)
3cf46f6055f7cd2e5da81e0d29cafc51ad4aafba [doc] improve RBF documentation (glozow)
c78eb8651b0949fefcafb22940512f4ef98d3358 [policy/refactor] pass in relay fee instead of using global (glozow)
Pull request description:
Followups to #22675 and documentation-only changes intended to clarify the code/logic concerning mempool Replace-by-Fee.
ACKs for top commit:
jnewbery:
utACK 0ef08f8bed537435f3f9db1e38b7d6f3551fe830
fanquake:
ACK 0ef08f8bed537435f3f9db1e38b7d6f3551fe830
Tree-SHA512: 6797ae758beca0c9673cb00ce85da48e9a4ac5cb5100074ca93e004cdb31d24d91a1a7721b57fc2f619addfeb4950d8caf45fee0f5b7528defbbd121eb4d271f
fa5e8c1044cb03a46807d6da6fe8d5f45f7e2e12 Revert "test: Add missing suppression signed-integer-overflow:addrman.cpp" (MarcoFalke)
Pull request description:
Forgot to do this in #22734
ACKs for top commit:
fanquake:
ACK fa5e8c1044cb03a46807d6da6fe8d5f45f7e2e12
Tree-SHA512: cabd741b73c828ae3c40df37d80022039e41e571fa196eebcb41e51c582064a1463a3677eb3b0e997ed141b32003802019474d2b739f0b606b4a16da4f585faa
fa08d4cfb1e3447a76093d46edc1c9de5ceee454 Use C++11 member initializer in CTxMemPoolEntry (MarcoFalke)
Pull request description:
This removes a bunch of boilerplate, makes the code easier to read.
Also, C++11 member initialization avoids accidental uninitialized
members.
Can be reviewed with the git option "--word-diff-regex=." or with "git
difftool --tool=meld".
ACKs for top commit:
jnewbery:
Code review ACK fa08d4cfb1e3447a76093d46edc1c9de5ceee454
shaavan:
Code Review ACK fa08d4cfb1e3447a76093d46edc1c9de5ceee454
Tree-SHA512: 2424861002fbcef2a3f01845662c115b973a7a5103f359305b5d9237c055eb003aa7646fc1cb30e6eaf90810d662f94cedc6f90795e30b56680f9c81f631d64b
fa45a1338adb127d69aee982920e29519bc1fed6 refactor: Remove unused validation includes (MarcoFalke)
Pull request description:
Unused includes will cause needless recompilation when headers are changed. Also, they pretend there are dependencies that don't exist.
Fix both by removing them.
ACKs for top commit:
laanwj:
Code review ACK fa45a1338adb127d69aee982920e29519bc1fed6
theStack:
ACK fa45a1338adb127d69aee982920e29519bc1fed6 ♻️
Tree-SHA512: 69190fd09184b75bce34ce3f315a1817e09ea32779f9ddc2d4790c89b0887b6cebd88aba66fa054c43c9183fc66202a556d982dd7034fc389a75802d8aaac83a
Can be reviewed with -W --ignore-all-space
Fixes:
* Calling ConsumeRandomLengthByteVector 4 times, when 2 is enough.
* Slow execution speed: Finalize is expensive because it invokes
division. Speed up the target by calling Finalize() at most twice per
fuzz input.
f6e4db27ceb67157dc13d13f34351cb87fec2be5 test: add aarch64-apple-darwin platform entry to get_previous_releases (Zero-1729)
Pull request description:
Over the course of reviewing a PR, I had to edit `test/get_previous_releases.py` (after I ran `git clean -xdff`) to run the backwards compatibility tests (e.g. `wallet_upgradewallet`, `feature_backwards_compatibility`, etc.), as currently on master, running the script as indicated in [`test/README.md`](https://github.com/bitcoin/bitcoin/blob/master/test/README.md), for example, on an M1 machine results in the following error, as the `aarch64-apple-darwin*` platform entry is presently not recognised:
> Output from an M1 machine running macOS v11.5.2
```sh
$ test/get_previous_releases.py -b v0.20.1 v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2
Releases directory: releases
Not sure which binary to download for aarch64-apple-darwin20.6.0
```
As a quick fix, this PR adds the missing `aarch64-apple-darwin*` platform entry. Running the script now results in fetching the old binaries, as expected:
```sh
$ test/get_previous_releases.py -b v0.20.1 v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2
Releases directory: releases
Fetching: https://bitcoincore.org/bin/bitcoin-core-0.20.1/bitcoin-0.20.1-osx64.tar.gz
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 20.9M 0 0 0 0 0 0 --:--:-- 0:00:02 --:--:-- 0
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 20.9M 100 20.9M 0 0 136k 0 0:02:37 0:02:37 --:--:-- 95607
Checksum matched
…
Checksum matched
```
After this patch, the backwards compatibility tests also run successfully, as expected.
**Note**: I am open to other possible solutions.
---
Steps to reproduce:
> Ensure you take out the binaries in `releases` if they already exist.
Try running `test/get_previous_releases.py -b v0.15.2` or similar to fetch the old release binaries.
Top commit has no ACKs.
Tree-SHA512: a238d909b70a61be622234bc49b05d2e91a8acfc5ea348d29f2c8a927fb793cb97365e558571e3f46d6a5650c4f3c6e28fa126c6e56b38e1eb98f7c3e3594d0f
fa3669f72f69662049b55ad1a482b4a0f9f7ae40 fuzz: Move all addrman fuzz targets to one file (MarcoFalke)
fa7a883f5a219d5f3c2f992b090db4e6c279db12 addrman: Replace assert with throw on corrupt data (MarcoFalke)
fa298971e6890715e2b0b93f2a7f445d32d6622f Refactor: Turn the internal addrman check helper into a forced check (MarcoFalke)
fae5c633dc05a045aaac370b383e4799cb0e0590 move-only: Move CAddrMan::Check to cpp file (MarcoFalke)
Pull request description:
Assert should only be used for program internal logic errors, not to sanitize external user input.
The assert was introduced via the debug-only runtime option `-checkaddrman` in commit 803ef70fd9f65ef800567ff9456fac525bc3e3c2, thus won't need a backport.
Also, it doesn't really make sense to continue when the deserialized addrman doesn't pass the sanity check.
For example, if `nLastSuccess` is negative, it would later result in integer overflows. Thus, this patch fixes#22931.
Also,
Fixes#22503Fixes#22504Fixes#22519Closes#22498
Steps to test:
```
mkdir -p /tmp/test_235/regtest/
echo 'H4sIAAAAAAAAA/u1f+stZmUGYgELgwPRakfBKBgFo2AUjIJRMApGwSgYBaNgFIyCUTBswdyGpFnLjUKjP9e0bvjYusl6b+L2e7Vs2dd6N//Pua0/xQUALJAn93IQAAA=' | base64 --decode | zcat > /tmp/test_235/regtest/peers.dat
./src/qt/bitcoin-qt -regtest -datadir=/tmp/test_235/ -checkaddrman=1 -printtoconsole | grep -A2 'Loading P2P addresses'
```
Output before:
```
2021-09-10T11:28:37Z init message: Loading P2P addresses…
2021-09-10T11:28:37Z ADDRMAN CONSISTENCY CHECK FAILED!!! err=-16
bitcoin-qt: addrman.cpp:765: void CAddrMan::Check() const: Assertion `false' failed.
(program crashes)
```
Output after:
```
2021-09-10T11:26:00Z init message: Loading P2P addresses…
2021-09-10T11:26:00Z Error: Invalid or corrupt peers.dat (Corrupt data. Consistency check failed with code -16: iostream error). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/tmp/test_235/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.
(program exits)
```
ACKs for top commit:
naumenkogs:
ACK fa3669f72f69662049b55ad1a482b4a0f9f7ae40
jnewbery:
Code review ACK fa3669f72f69662049b55ad1a482b4a0f9f7ae40
vasild:
ACK fa3669f72f69662049b55ad1a482b4a0f9f7ae40
Tree-SHA512: 687e4a4765bbc66495152fa7a49d28ee84b405dc5370ba87b4016b5593e45f54c4ce5cae579e4d433e0e082d20fc263969fa602679c911accef0adb2d6213bd6
This removes a bunch of boilerplate, makes the code easier to read.
Also, C++11 member initialization avoids accidental uninitialized
members.
Can be reviewed with the git option "--word-diff-regex=." or with "git
difftool --tool=meld".
f680d27155374de658d40db0ba40460919aa1ba2 test: use MiniWallet for make_utxo helper in feature_rbf.py (Sebastian Falbesoner)
0f275246027266fa256d0a09251bb2c88d9bd72f test: scale amounts in test_doublespend_tree down by factor 10 (Sebastian Falbesoner)
d1e2481274edf2ac14747be633d86ecd46814ef4 test: scale amounts in test_doublespend_chain down by factor 10 (Sebastian Falbesoner)
Pull request description:
This PR aims to further increase MiniWallet usage in the functional test feature_rbf.py by using it in the `make_utxo(...)` helper, which is the only part that needs a wallet for most sub-tests. In order to do that, the amounts for the utxos have to be scaled down in two sub-tests first (`test_doublespend_chain` and `test_doublespend_tree`, see first two commits), since we need amounts passed to `make_utxo` than can be funded by only one input. For creating UTXOs with a value of 50 BTC, we'd need to implement a method for consolidating multiple utxos into one first, which seems to be overkill.
Note that after this PR's change, there is only one sub-test left (`test_rpc`) that needs the wallet compiled into bitcoind.
ACKs for top commit:
MarcoFalke:
review ACK f680d27155374de658d40db0ba40460919aa1ba2 🦐
Tree-SHA512: 46c8c245086a9e79855c4ede2f8f412333cf2658136805196b203b3567c89398d77fcb80715c0bb72fdc84331cc67544b2fdc259193a3adcb2fc36e147c26fce
The benchmarks can now run much longer due to the minimum of 10ms or
directly with -min_time. With -min_time=20000 I could trigger two ubsan
errors in the benchmarks, which are fixed in this commit by using
unsigned type and adding "& 0xFF".
- use ALLOW_BOOL for -list arg instead of ALLOW_ANY
- touch up `-asymptote=<n1,n2,n3...>` help
- pack Args struct a bit more efficiently
- handle args in alphabetical order
When it is not easily possible to stabilize benchmark machine and code
the argument -min_time can be used to specify a minimum duration
that a benchmark should take. E.g. choose -min_time=1000 if you
are willing to wait about 1 second for each benchmark result.
The default is now set to 10ms instead of 0, which should make runs on
fast machines more stable with negligible slowdown.
