The `ccoins_add` and `ccoins_write` tests check the actual exception error messages now instead of just that they fail for the given parameters.
This enables us testing different exceptions in a more fine-grained way in later changes.
We don't need so much access to the internals of CCoinsCacheEntry, since many tests are just exercising invalid combinations this way.
This implies that `AddFlags` has private access now.
CCoinsCacheEntry provided general access to its internal flags state, even though in reality it could only be clean, fresh, dirty or fresh|dirty.
After it got dirtied we couldn't set the state back to clean by AddFlags(0) - tests were explicitly checking against that.
This commit cleans up the public interface to make this distinction cleaner and invalid behavior impossible instead of just checked by tests.
This includes the removal of redundant `inline` qualifiers (we're inside a struct).
Also renamed `self` to `pair` to simplify the upcoming commits.
Also modernized `EmplaceCoinInternalDANGER` since it was already modified.
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
4feaa28728 refactor: Rely on returned value of GetCoin instead of parameter (Lőrinc)
46dfbf169b refactor: Return optional of Coin in GetCoin (Lőrinc)
e31bfb26c2 refactor: Remove unrealistic simulation state (Lőrinc)
Pull request description:
While reviewing [the removal of the unreachable combinations from the Coin cache logic](https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1721727681), we've noticed that the related tests often [reflect impossible states](https://github.com/bitcoin/bitcoin/pull/30673/files#r1740154464).
Browsing the Coin cache refactoring history revealed that migrating `bool GetCoin` to `optional<Coin> GetCoin` was [already proposed a few times before](https://github.com/bitcoin/bitcoin/pull/18746#issuecomment-842393167).
This refactor makes certain invalid states impossible, reducing the possibility of errors and making the code easier to understand. This will let us remove test code that exercises the impossible states as well.
The PR is done in multiple small steps, first swapping the new `optional` return value, slowly strangling out the usages of the return parameter, followed by the removal of the parameter.
Most of the invalid test states were still kept, except for https://github.com/bitcoin/bitcoin/pull/30673/files#r1748087322, where the new design prohibits invalid usage and https://github.com/bitcoin/bitcoin/pull/30673/files#r1749350258 was just marked with a TODO, will be removed in a follow-up PR.
ACKs for top commit:
andrewtoth:
re-ACK 4feaa28728
achow101:
ACK 4feaa28728
laanwj:
Code review ACK 4feaa28728
theStack:
Code-review ACK 4feaa28728
Tree-SHA512: 818d60b2e97f58c489a61120fe761fb67a08dffbefe7a3fce712d362fc9eb8c2cced23074f1bec55fe71c616a3561b5a8737919ad6ffb2635467ec4711683df7
c98fc36d09 wallet: migration, consolidate external wallets db writes (furszy)
7c9076a2d2 wallet: migration, consolidate main wallet db writes (furszy)
9ef20e86d7 wallet: provide WalletBatch to 'SetupDescriptorScriptPubKeyMans' (furszy)
34bf0795fc wallet: refactor ApplyMigrationData to return util::Result<void> (furszy)
aacaaaa0d3 wallet: provide WalletBatch to 'RemoveTxs' (furszy)
57249ff669 wallet: introduce active db txn listeners (furszy)
91e065ec17 wallet: remove post-migration signals connection (furszy)
055c0532fc wallet: provide WalletBatch to 'DeleteRecords' (furszy)
122d103ca2 wallet: introduce 'SetWalletFlagWithDB' (furszy)
6052c7891d wallet: decouple default descriptors creation from external signer setup (furszy)
f2541d09e1 wallet: batch MigrateToDescriptor() db transactions (furszy)
66c9936455 bench: add coverage for wallet migration process (furszy)
Pull request description:
Last step in a chain of PRs (#26836, #28894, #28987, #29403).
#### Detailed Description:
The current wallet migration process performs only individual db writes. Accessing disk to
delete all legacy records, clone and clean each address book entry for every created wallet,
create each new descriptor (with their corresponding master key, caches and key pool), and
also clone and delete each transaction that requires to be transferred to a different wallet.
This work consolidates all individual disk writes into two batch operations. One for the descriptors
creation from the legacy data and a second one for the execution of the migration process itself.
Efficiently dumping all the information to disk at once atomically at the end of each process.
This represent a speed up and also a consistency improvement. During migration, we either
want to succeed or fail. No other outcomes should be accepted. We should never leave a
partially migrated wallet on disk and request the user to manually restore the previous wallet from
a backup (at least not if we can avoid it).
Since the speedup depends on the storage device, benchmark results can vary significantly.
Locally, I have seen a 15% speedup on a USB 3.2 pendrive.
#### Note for Testers:
The first commit introduces a benchmark for the migration process. This one can be
cherry-picked on top of master to compare results pre and post changes.
Please note that the benchmark setup may take some time (~70 seconds here) due to the absence
of a batching mechanism for the address generation process (`GetNewDestination()` calls).
ACKs for top commit:
achow101:
ACK c98fc36d09
theStack:
re-ACK c98fc36d09
pablomartin4btc:
re-ACK c98fc36d09
Tree-SHA512: a52d5f2eef27811045d613637c0a9d0b7e180256ddc1c893749d98ba2882b570c45f28cc7263cadd4710f2c10db1bea33d88051f29c6b789bc6180c85b5fd8f6
8523d8c0fc ci: display logs of failed tests automatically (furszy)
Pull request description:
Saw it here https://github.com/bitcoin/bitcoin/actions/runs/11488618084/job/31975712362?pr=31000.
The 'test-each-commit' and 'win64' CI jobs currently do not display test logs when an error occurs, making it almost impossible to debug issues that don't arise locally. Fix this by setting the CTest `--output-on-failure` flag (per [README](2f40e453cc/src/test/README.md (L130))).
ACKs for top commit:
hebasto:
ACK 8523d8c0fc, I have reviewed the code and it looks OK.
Tree-SHA512: 59c025099fb623e2ed430cfc1ba808e1d3ff72773d021e2280a44423ae53615c16e96a07014eb8581c95aae241b6d2777e388a8931ff0904feb84ca45cb22763
fb46d57d4e cmake, qt, test: Remove problematic code (Hennadii Stepanov)
Pull request description:
Split from https://github.com/bitcoin/bitcoin/pull/30997.
The removed code aimed to make Qt's minimal integration plugin DLL available for `test_bitcoin-qt.exe` on Windows.
However, there are two issues:
1. The code is broken because the destination directory must end with a trailing slash (`/`).
2. It is unnecessary because Qt's minimal integration plugin is not used on Windows. For more details, please refer to the following code:fb46d57d4e/src/qt/test/CMakeLists.txt (L38-L44)
As a side effect, this PR fixes https://github.com/bitcoin-core/gui/issues/842.
ACKs for top commit:
fanquake:
ACK fb46d57d4e
TheCharlatan:
ACK fb46d57d4e
Tree-SHA512: b44d1c5e352e9bbfbba3c263ee03838cd490435da0490d9c8a152e60515520772c8a87aca08d4510f50c2e46b64ac92c666fa81accf43758af2e896693c44ffa
6c6b2442ed build: Replace MAC_OSX macro with existing __APPLE__ (Lőrinc)
Pull request description:
This PR aims to standardize and simplify macOS-specific checks within our codebase by replacing the custom-defined `MAC_OSX` macro with the existing `__APPLE__`macro, defined in e.g. https://sourceforge.net/p/predef/wiki/OperatingSystems/#macos
We already use `__APPLE__` in our own codebase for e.g. https://github.com/bitcoin/bitcoin/blob/master/src/crypto/sha256.cpp#L22
Local Verification confirms that `MAC_OSX` isn't defined, but `__APPLE__` is:
```bash
% echo | cpp -dM | egrep 'MAC_OSX|__MACOS__|__APPLE__'
#define __APPLE__ 1
```
ACKs for top commit:
fanquake:
ACK 6c6b2442ed - at this point it seems unlikely that we'll need to accomodate non-macOS Apple, so consolidating to `__APPLE__` seems ok for now.
Tree-SHA512: dbf87c96211d9d55426ee85d76ef1e05cda3efd1c9248b0974a82834dafc1c1aece3165bd46e4252f0460dc97079bdbcebe98bbd81e9de0d7399c0bc69d5c050
The removed code aimed to make Qt's minimal integration plugin DLL
available for `test_bitcoin-qt.exe` on Windows.
However, there are two issues:
1. The code is broken because the destination directory must end with a
trailing slash (`/`).
2. It is unnecessary because Qt's minimal integration plugin is not
used on Windows. For more details, please refer to the following
code.
fa1c5cc9df fees: Log non-fatal errors as [warning], instead of info-level (MarcoFalke)
ddddbac9c1 fees: Pin required version to 149900 (MarcoFalke)
fa5126adcb fees: Pin "version that wrote" to 0 (MarcoFalke)
Pull request description:
Coupling the fees serialization with CLIENT_VERSION is problematic, because:
* `CLIENT_VERSION` may change, even though the serialization format does not change. This is harmless, but still confusing.
* If a serialization format change was backported (unlikely), it may lead to incorrect results.
* `CLIENT_VERSION` is changed at a different time during the release process than any serialization format change. This is harmless for releases of Bitcoin Core, but may be confusing when using the development branch.
* It is harder to reason about a global `CLIENT_VERSION` when changing the format, than to reason about a versioning local to the module.
Fix all issues by pinning the current version number in the module locally. In the future it can then be modified locally to the module, if needed.
ACKs for top commit:
hodlinator:
re-ACK fa1c5cc9df
TheCharlatan:
Re-ACK fa1c5cc9df
Tree-SHA512: 93870176ed50cc5a734576d66398a6036b31632228a9e05db1fa5452229e35ba4126f003e7db246aeb9891764ed47bde4470c674ec2bce7fd3ddd97e43944627
82e16e6983 cmake: Refactor install kernel dependencies (Hennadii Stepanov)
42e6277987 build: Add static libraries to Kernel install component (TheCharlatan)
Pull request description:
Fixes the installation of the pkgconfig file and the static library when installing only the `Kernel` component.
This is a followup to fix#30835 and #30814, which were merged shortly after one another, but are interrelated. Can be tested with:
```
cmake -B build -DBUILD_SHARED_LIBS=OFF -DBUILD_KERNEL_LIB=ON
cmake --build build --target bitcoinkernel
cmake --install build --component Kernel
```
ACKs for top commit:
hebasto:
ACK 82e16e6983, tested on Ubuntu 23.10.
fanquake:
ACK 82e16e6983
Tree-SHA512: 07c18a341d4464e489c28fb262600338f1711248309ffb2af0ef3ab1abf06f10873c435895b63010e0be8e44af77046324896dfd872479792aa049831606dc45
a0c9595810 doc: Make list of targets in depends README consistent (laanwj)
Pull request description:
The description of `i686-pc-linux-gnu` and `x86_64-pc-linux-gnu` is incomplete and inconsistent with the others. Fix this. Also use "64 bit" consistently instead of "64-bit".
ACKs for top commit:
maflcko:
lgtm ACK a0c9595810
hebasto:
ACK a0c9595810.
jarolrod:
ACK a0c9595810
rkrux:
ACK a0c9595810
Tree-SHA512: eedefb19639dd08f25627ceaab0d2c3745b256e561e55f8506d14721d0236978f1b1bef79f9fe126b7f42d869887ca988d04b3536d98a27e0eb182f0a7f64183
31cc5006c3 init: Return fatal failure on snapshot validation failure (Martin Zumsande)
8f1246e833 init: Improve chainstate init db error messages (TheCharlatan)
cd093049dd init: Remove incorrect comment about shutdown condition (MarcoFalke)
635e9f85d7 init: Remove misleading log line when user chooses not to retry (TheCharlatan)
720ce880a3 init: Improve comment describing chainstate load retry behaviour (Martin Zumsande)
baea842ff1 init: Remove unneeded argument for mempool_opts checks (stickies-v)
Pull request description:
These are mostly followups from #30968, making the code, log lines, error messages, and comments more consistent.
The last commit is an attempt at improving the error reporting when loading the chainstate. It aims to more cleanly distinguish between errors arising from a specific database, and errors where the culprit may be less clear.
ACKs for top commit:
achow101:
ACK 31cc5006c3
mzumsande:
Code Review / lightly tested ACK 31cc5006c3
BrandonOdiwuor:
Code Review ACK 31cc5006c3.
stickies-v:
ACK 31cc5006c3
Tree-SHA512: 59fba4845ee45a3d91bf55807ae6b1c81458463b96bf664c8b1badfac503f6b01efd52a915fc399294e68a3f69985362a5a10a3844fa23f7707145ebe9ad349b
04e4d52420 test: add test for specifying custom pidfile via `-pid` (Sebastian Falbesoner)
b832ffe044 refactor: introduce default pid file name constant in tests (tdb3)
Pull request description:
This small PR adds test coverage for the `-pid` command line option, which allows to overrule the pid filename (`bitcoind.pid` by default). One can specify either a relative path (within the datadir) or an absolute one; the latter is tested using `self.options.tmpdir`. Note that the functional test file `feature_init.py` so far only contained a stress test; with this new sub-test added, both the description and the test name are adapted to be more generic.
ACKs for top commit:
achow101:
ACK 04e4d52420
tdb3:
ACK 04e4d52420
ryanofsky:
Code review ACK 04e4d52420
naiyoma:
Tested ACK [04e4d52420)
Tree-SHA512: b2bc8a790e5d187e2c84345f344f65a176b62caecd9797c3b9edf10294c741c33a24e535be640b56444b91dcf9c65c7dd152cdffd8b1c1d9ca68e5e3c6ad1e99
The description of `i686-pc-linux-gnu` and `x86_64-pc-linux-gnu` is
incomplete and inconsistent with the rest. Fix this. Also use "64 bit"
consistently instead of "64-bit".
Also, remove not needed and possibly redundant function name and class
names from the log string. Also, minimally reword the log messages.
Also, remove redundant trailing newlines from log messages, while
touching.
fa71bedf86 ci: Approximate MAKEJOBS in image build phase (MarcoFalke)
Pull request description:
The `MAKEJOBS` env var is the default in image builds, which is fine, because it is only relevant when building msan (or iwyu) and only differs when setting MAKEJOBS to something other than `nproc` (currently used as an approximation).
So the normal workflow of `MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh` already works today.
However, `MAKEJOBS="-j1" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh` does not.
This is hard to fix, because making the env var a build arg means that changing it (and only it) requires a new (expensive and redundant) build.
So add an option `HAVE_CGROUP_CPUSET`, which can be set to approximate `MAKEJOBS` a bit. Can be tested via:
`HAVE_CGROUP_CPUSET=yo MAKEJOBS="-j_something" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh`
ACKs for top commit:
fanquake:
ACK fa71bedf86
Tree-SHA512: 43ef194c71d726f4cfa3fe08a5894c7872150f37da7e4fa0c2d89e4572bc63acadb5dae3286a5e5cc14a8ce3e1ebcc14571f1a3541e8db2d18d2f7503764a2f3
a16917fb59 rpc, net: improve `mapped_as` doc for getrawaddrman/getpeerinfo (brunoerg)
bdad0243be rpc, net: getrawaddrman "mapped_as" follow-ups (brunoerg)
Pull request description:
- Change `addrman` to reference to const since it isn't modified (https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612272793).
- Improve documentation of `mapped_as`/`source_mapped_as` in `getrawaddrman` RPC by mentioning that both fields will be only available if asmap flag is set. It is the same message for `mapped_as` field in `getpeerinfo`.
ACKs for top commit:
fjahr:
re-ACK a16917fb59
0xB10C:
re-ACK a16917fb59
laanwj:
re-ACK a16917fb59
Tree-SHA512: c66b2ee9d24da93d443be83f6ef3b2d39fd5bf3f73e2974574cad238ffb82035704cf4fbf1bac22a63734948e285e8e091c2884bb640202efdb473315e770233
4d3da08d1b guix: Enable CET for `glibc` package (Hennadii Stepanov)
Pull request description:
Pulled from #30685. This doesn't need to wait for anything.
ACKs for top commit:
laanwj:
ACK 4d3da08d1b
TheCharlatan:
ACK 4d3da08d1b
Tree-SHA512: 1f4645971381fd342adec52c826fc0023722519a3e28043c9fe8b64bbc1abad822fcc25a64f3f959e3f3a10f5c119029f4cae13c22bac6badcbec9ae8b501dfc
86e2a6b749 [test] A non-standard transaction which is also consensus-invalid should return the consensus error (Antoine Poinsot)
f859ff8a4e [validation] Improve script check error reporting (dergoegge)
Pull request description:
An input script might be invalid for multiple reasons. For example, it might fail both a standardness check and a consensus check, which can lead to a `mandatory-script-verify-flag-failed` error being reported that includes the script error string from the standardness failure (e.g. `mandatory-script-verify-flag-failed (Using OP_CODESEPARATOR in non-witness script)`), which is confusing.
ACKs for top commit:
darosior:
re-ACK 86e2a6b749
ariard:
Re-Code Review ACK 86e2a6b7
instagibbs:
ACK 86e2a6b749
Tree-SHA512: 053939107c0bcd6643e9006b2518ddc3a6de47d2c6c66af71a04e8af5cf9ec207f19e54583b7a056efd77571edf5fd4f36c31ebe80d1f0777219c756c055eb42
Before, we did not explicity say that both fields
`{source_}mapped_as` (that are optional in getrawaddrman)
will be only available if the asmap config flag is set.
Co-authored-by: Jon Atack <jon@atack.com>
The wallet is isolated during migration and reloaded at the end
of the process. There is no benefit on connecting the signals
few lines before unloading the wallet.
fac6cfe5ac lint: commit-script-check.sh: echo to stderr (MarcoFalke)
Pull request description:
This makes it easier to redirect the produced `git diff` on failure. On success, it shouldn't hurt, because the same output is still present, just on stderr.
Can be tested by introducing a fault in any scripted diff and then calling `commit-script-check.sh HEAD~..HEAD > any_file.txt`. Previously the file contained the full output, now it contains just the diff.
ACKs for top commit:
TheCharlatan:
ACK fac6cfe5ac
Tree-SHA512: b4dfad10a4a902729a7ad7533ed0ef86b9e79761083f2ec623d448a551462b268fe04bdba387ca62160dae9ef7b1781e005dec60f18b111d9bfa6b97357108e6
cd0edf26c0 tracing: cast block_connected duration to nanoseconds (0xb10c)
Pull request description:
When the `validation:block_connected` tracepoint was introduced in 8f37f5c2a5, the connect block duration was passed in microseconds `µs`. By starting to use steady clock in fabf1cdb20 this changed to nanoseconds `ns`. As the test only checked if the duration value is `> 0` as a plausibility check, this went unnoticed. This was detected this when setting up monitoring for block validation time as part of the Great Consensus Cleanup Revival discussion.
This change casts the duration explicitly to nanoseconds, updates the documentation, and adds a check for an upper bound to the tracepoint interface tests. The upper bound is quite lax as mining the block takes much longer than connecting the empty test block. It's however able to detect a duration passed in an incorrect unit (1000x off).
A previous version of this PR casted the duration to microseconds `µs` - however, as the last three major releases have had the duration as nanoseconds (and this went unnoticed), we assume that this is the API now and changeing it back to microseconds would break the API again. See also https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2067867597
ACKs for top commit:
maflcko:
re-lgtm ACK cd0edf26c0
laanwj:
re-ACK cd0edf26c0
Tree-SHA512: 54a1eea0297e01c07c2d071ffafbf97dbd080f763e1dc0014ff086a913b739637c1634b1cf87c90b94a3c2f66006acfaada0414a15769cac761e03bc4aab2a77
184f12c154 doc: remove dependency install instructions from win docs (fanquake)
Pull request description:
This duplicates what is in depends, and is outdated.
Closes#31090.
ACKs for top commit:
maflcko:
lgtm ACK 184f12c154
jarolrod:
ACK 184f12c154
BrandonOdiwuor:
ACK 184f12c154
Tree-SHA512: 089c9ff91c501c22ec1b9d5925a2b8c6cd1ea9ac2b75dd6a8c5fe75cf2f0090d808842cb321017894d2da70a30a87dbc1c4c481771d3c4aba13ce44244fcf392
a647d4400d doc: update signet documentation related to build directories (Torkel Rogstad)
Pull request description:
While setting up my own signet I noticed that the binary paths in the documentation for this is out of date, after build artifacts moved to the `build` directory. This PR mimics what happened in #30741
ACKs for top commit:
maflcko:
lgtm ACK a647d4400d
pablomartin4btc:
ACK a647d4400d
tdb3:
Code review and light test ACK a647d4400d
Tree-SHA512: ac7c3806e0ff65860c41d7b7bdad538368d8a6d8d289c10f9714804f963bafd3a9658301b6697f110f5462a92826b62770963508d5eebf88bf9a0a8442d9f72d
fa43c4f93c test: Print CompletedProcess object on error (MarcoFalke)
Pull request description:
It would be good to know the output on `Error parsing command output`. Otherwise test failures are meaningless: https://github.com/bitcoin/bitcoin/issues/30792#issuecomment-2325911157
Fix it by just printing the full `CompletedProcess` object.
Also, use the modern `subprocess.run` to simplify the code.
ACKs for top commit:
BrandonOdiwuor:
Code Review ACK fa43c4f93c
laanwj:
This contains some useful information, so ACK fa43c4f93c
Tree-SHA512: ae7c1cb1f48af2a6feae6d1a5a967c0720f6c6675c1ce20ace7cac18c00f3d4069b8abcc58204855e92ff5303158b9a942bab3b71acae0737768d941a5773c91
