fa7da227da refactor: Fix implicit-signed-integer-truncation in cuckoocache.h (MarcoFalke)
Pull request description:
Using a file-wide suppression for this implicit truncation has several issues:
* It is file-wide, thus suppressing any other (newly introduced) issues
* The file doesn't compile with `-Wimplicit-int-conversion`
Fix both issues by making the truncation explicit.
ACKs for top commit:
fanquake:
ACK fa7da227da
Tree-SHA512: bf2076ed94c4e80d0d29ff883080edc7a73144c73d6d3e872ec87966177ee3160f4760fc4c774aaa6fb591f4acee450a24b0f7c82291e0bef96582a6d134046e
ff945e553a MOVEONLY: Move utility functions from rpcwallet to wallet/rpc/util (Samuel Dobson)
7b04a064f6 Introduce wallet/rpc/util (Samuel Dobson)
Pull request description:
This is part one of multiple to split up rpcwallet.cpp into smaller, more logical units.
See #23622 for context and overall plan. I'll open PRs in stages to hopefully minimise conflicts.
Can be reviewed with `--color-moved=dimmed-zebra`
The end goal can be seen here: https://github.com/meshcollider/bitcoin/tree/202111_split_walletrpc
ACKs for top commit:
MarcoFalke:
nice, ACK ff945e553a🐰
shaavan:
ACK ff945e553a
Tree-SHA512: 6e3d1de6db770fe2fca540c8c4f30183ab8258c26e3a1fb46937714d28818a52933eafbfcafe2995f6a6e2551a3f3dd3ec93363b81de7912c0d81f5749d1c86d
fa46ac4d9d miner: Remove uncompiled MTP code (MarcoFalke)
fa6b7adf96 style: Add {} to if-bodies in node/miner (MarcoFalke)
Pull request description:
This removes uncompiled code.
Can be checked by inserting `static_assert(STANDARD_LOCKTIME_VERIFY_FLAGS & LOCKTIME_MEDIAN_TIME_PAST)` and compiling or by reading the source code.
Even if the code was compiled, it would be unsafe to execute, since it is not allowed to include transactions that are locked until some time after the current MTP.
Also, rename the member to cause explicit merge conflicts in case there is a patch out there referencing the variable.
ACKs for top commit:
shaavan:
ACK fa46ac4d9d
theStack:
Code-review ACK fa46ac4d9d
Tree-SHA512: 0288f45918996b58d0c0060773aa3cb15c828a649439f3d589c5d6b4854d6da1d8c2ea11d5ca06c654532453ab5ce1892de7ca820e284e96e78b959ef87cac5c
a64078e385 Break validation <-> txmempool circular dependency (glozow)
64e4963c63 [mempool] always assert coin spent (glozow)
bb9078ed51 [refactor] put finality and maturity checking into a lambda (glozow)
bedf246f1e [mempool] only update lockpoints for non-removed entries (glozow)
1b3a11e126 MOVEONLY: TestLockPointValidity to txmempool (glozow)
Pull request description:
Remove 2 circular dependencies: validation - txmempool and validation - policy/rbf - txmempool
Validation should depend on txmempool (e.g. `CChainstateManager` has a mempool and we often need to know what's in our mempool to validate transactions), but txmempool is a data structure that shouldn't really need to know about chain state.
- Changes `removeForReorg()` to be parameterized by a callable that returns true/false (i.e. whether the transaction should be removed due to being now immature or nonfinal) instead of a `CChainState`. The mempool really shouldn't need to know about coinbase maturity or lockpoints, it just needs to know which entries to remove.
ACKs for top commit:
laanwj:
Code review ACK a64078e385
mjdietzx:
reACK a64078e385
theStack:
re-ACK a64078e385
Tree-SHA512: f75995200569c09dfb8ddc09729da66ddb32167ff1e8a7e72f105ec062d2d6a9a390e6b4a2a115e7ad8ad3525f891ee1503f3cd2bed11773abcaf7c3230b1136
123f5de826 Remove calls to global Params() in tx_pool test (lsilva01)
9360778d6e Remove AcceptToMemoryPoolWithTime (lsilva01)
Pull request description:
This PR refactors AcceptToMemoryPool.
. Remove `AcceptToMemoryPoolWithTime` (after #23173, this function is no longer needed).
. Remove the `CChainParams chainparams` parameter from ATMP as they can be inferred from the current chain state.
. Update the `tx_pool` test with new function signature.
ACKs for top commit:
jnewbery:
reACK 123f5de826
glozow:
light code review ACK 123f5de826
theStack:
re-ACK 123f5de826
Tree-SHA512: b672d9f091f5fcb4d163c0a443aa469b2ce6f37201136803201c620c3fa329ffb41abd50c5b528787fd1c47e8e09af5ec20ddaa0a4235352c9c619e5691dddb6
c771ee8571 doc: use BIP125-replaceable (fanquake)
36dc5bb8cb doc: Extract CreateTxDoc in rawtransaction (fanquake)
Pull request description:
For the fields: inputs, outputs, locktime, replaceable. Similar to #23172.
Can be reviewed with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`.
ACKs for top commit:
MarcoFalke:
ACK c771ee8571😸
Tree-SHA512: e6e4211b89bedec472f8381b3cbea5670f82b728955888c794f694164b8d8bdd51a99c64762b625357ac2171005712b82f81ee7c1b8f5c5620bdedeeefa2b9da
fa00447442 scripted-diff: Use clang-tidy syntax for C++ named arguments (MarcoFalke)
fae13c3989 doc: Use clang-tidy comments in crypto_tests (MarcoFalke)
Pull request description:
Incorrect named args are source of bugs, like #22979.
To allow them being checked by `clang-tidy`, use a format it can understand.
ACKs for top commit:
shaavan:
ACK fa00447442
rajarshimaitra:
ACK fa00447442
jonatack:
ACK fa00447442
fanquake:
ACK fa00447442
Tree-SHA512: 4d23a8363da81dfea21a4cd8516ab5e0dc70119e4d503f3f240f38573218b2c2e84083b97e956c62942d78b2f17490f8b3b2e8077d257644fda1d901e2b80507
6c9ee92ffe net: don't check if the listening socket is valid (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Listening sockets in `CConnman::vhListenSocket` are always valid
(underlying file descriptor is not `INVALID_SOCKET`).
ACKs for top commit:
theStack:
Code-review ACK 6c9ee92ffe🔌
Tree-SHA512: b2e29711c6a0c7c85467ca61cfd7fb734eb06bd83a41f88735901caf90aec095ca80707ce5bb897d39c80fdec16819dbf5a84979c9b1ab3dc3fb8b08cebe7c61
faad05c6d2 Crash debug builds when mempool ConsensusScriptChecks fails (MarcoFalke)
Pull request description:
Currently a bug in the function might sneak around our testing infrastructure.
Fix that by turning bugs into crashes during tests.
ACKs for top commit:
glozow:
utACK faad05c6d2, there's something seriously wrong with the code if this returns false, good to throw in debug mode
Tree-SHA512: dfea1cd9ce3f1c303f49cca1417cd5c77c6ed12849aaff7b6ab1b6060f2f0c9cf5d4689017355d11f66639bab35823f65f848e6979042fa875181509dfd5d3d7
d9803f7a0a test: add stress tests for initialization (James O'Beirne)
23f85616a8 test: add node.chain_path and node.debug_log_path (James O'Beirne)
Pull request description:
In the course of coming up with a test plan for #23280, I thought it would be neat to include a Python snippet showing how I tested the initialization process. I quickly realized I was reinventing the functional test framework... so here's a new test.
This change bangs init around like the Fonz hitting a jukebox. It adds some interesting (read: lazy and random) coverage for the initialization process by
- interrupting init with SIGTERM after certain log statements,
- interrupting init at random points, and
- starting init with some essential data missing (block files, block indices, etc.) to test init error paths.
As far as I can tell, some of these code paths are uncovered otherwise (namely the startup errors).
---
Incidentally, I think I may have uncovered some kind of a bug or race condition with indexing initialization based on an intermittent failure in this testcase. This test sometimes fails after shutting down immediately after `loadblk` thread start:
```
2021-10-15T21:14:51.295000Z TestFramework (INFO): Starting node and will exit after line 'loadblk thread start'
36 │ 2021-10-15T21:14:51.296000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up
37 │ 2021-10-15T21:14:51.493000Z TestFramework (INFO): terminating node after 110 log lines seen
38 │ 2021-10-15T21:14:51.625000Z TestFramework (INFO): Starting node and will exit after line 'txindex thread start'
39 │ 2021-10-15T21:14:51.625000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up
------> [[ FAILURE HERE ]] 2021-10-15T21:15:21.626000Z TestFramework (WARNING): missed line {bail_line}; bailing now after {num_lines} lines
```
and then fails to start up afterwards. Combined logs showing `Error: txindex best block of the index goes beyond pruned data`, when the node under test is not pruned:
```
node0 2021-10-15T21:16:51.848439Z [shutoff] [validationinterface.cpp:244] [ChainStateFlushed] Enqueuing ChainStateFlushed: block hash=1014bc4ff4917602ae53d10e9dfe230af4b7d52a6cdaa8a47798b9c288180907
node0 2021-10-15T21:16:51.848954Z [shutoff] [init.cpp:302] [Shutdown] Shutdown: done
test 2021-10-15T21:16:51.882000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/home/james/src/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/home/james/src/bitcoin/./test/functional/stress_init.py", line 87, in run_test
check_clean_start()
File "/home/james/src/bitcoin/./test/functional/stress_init.py", line 60, in check_clean_start
node.wait_for_rpc_connection()
File "/home/james/src/bitcoin/test/functional/test_framework/test_node.py", line 224, in wait_for_rpc_connection
raise FailedToStartError(self._node_msg(
test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status 1 during initialization
test 2021-10-15T21:16:51.882000Z TestFramework (DEBUG): Closing down network thread
test 2021-10-15T21:16:51.933000Z TestFramework (INFO): Stopping nodes
test 2021-10-15T21:16:51.933000Z TestFramework.node0 (DEBUG): Stopping node
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
node0 stderr Error: txindex best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
```
ACKs for top commit:
laanwj:
Code review ACK d9803f7a0a
Tree-SHA512: 4d80dc399daf199a6222e81e47d12d830dc7af07355eddbb7f52479a676a645b8d3d45093ff54a9295f01a163b2f4fe0e038e83fc269969e03d4cfda69eaf111
a3f61676e8 test: Make more shell scripts verifiable by the `shellcheck` tool (Hennadii Stepanov)
Pull request description:
Some shell scripts from `contrib/guix` and `contrib/shell` are not verifiable by the `shellcheck` tool for the following reasons:
- they have no extension (see 4eccf063b2 from bitcoin/bitcoin#21375)
- they have the `.bash` extension while `.sh` is expected
This PR adds these scripts to the input for the `shellcheck` tool, and it fixes discovered `shellcheck` warnings.
ACKs for top commit:
dongcarl:
Code Review ACK a3f61676e8, this is a good robustness improvement for our shell scripts.
jamesob:
crACK a3f61676e8
Tree-SHA512: 6703f5369d9c04c1a174491f381afa5ec2cc4d37321c1b93615abcdde4dfd3caae82868b699c25b72132d8c8c6f2e9cf24d38eb180ed4d0f0584d8c282e58935
6aed8b7e9b [test] tx processing before and after ibd (glozow)
b9e105b664 [net_processing] ignore all transactions during ibd (glozow)
Pull request description:
This is basically a mini, IBD-only version of #21224
Incoming transactions aren't really relevant until we're caught up. That's why we send a giant feefilter and don't send tx getdatas, but we also shouldn't process them if peers send them anyway. Simply ignore them.
ACKs for top commit:
jnewbery:
reACK 6aed8b7e9b
laanwj:
Code review ACK 6aed8b7e9b
Tree-SHA512: 8e1616bf355f9d0b180bdbc5461f24c757dc5d7bc7bf651470f3b0bffcca5d5e68287106255b5cede2d96b42bce448a0f8c0649de35a530c5e079f7c89c70a35
No behavior change.
Parameterize removeForReorg using a CChain and callable that
encapsulates validation logic. The mempool shouldn't need to know a
bunch of details about coinbase maturity and lock finality. Instead,
just pass in a callable function that says true/false. Breaks circular
dependency by removing txmempool's dependency on validation.
Each entry's lockpoints are independent of one another, so there isn't
any reason to update lockpoints for entries that will be removed.
Separating the loops also allows us to move validation logic out and
leave lockpoints in txmempool.
fae239208d wallet: Split signmessage from rpcwallet (MarcoFalke)
Pull request description:
rpcwallet is the file that takes longest to compile, especially with sanitizers enabled it can take several 10s of seconds.
Allow faster incremental and parallel builds by starting to split it up. First, split off `signmessage`, which is unrelated to other stuff such as wallet file handling, wallet encryption, tx creation, or wallet status/info.
ACKs for top commit:
ryanofsky:
Code review ACK fae239208d. Confirmed move only
meshcollider:
Code review ACK fae239208d
Tree-SHA512: 250445cd544e39376f225871270cdcae462f16cfd9d25ede4b148e915642bfac9ee7ef3e8eccdd2443dc74dbf794d3bcd5fe5c58b1d05a2dcec70b8e03b37dff
855bd75503 build, qt: Ditch `no_sdk_version_check.patch` (Hennadii Stepanov)
Pull request description:
Since Qt 5.12.2 (see 5d181961ea) it is possible to control whether to check macOS SDK with the `QT_MAC_SDK_NO_VERSION_CHECK` variable.
ACKs for top commit:
fanquake:
ACK 855bd75503
Tree-SHA512: e490f22da9c37d25b58418ca62e65d2303cfbd83139f4abea4b67c37fa97e9a882ccc9d484b49cd2bb3a8851ea35e63123d3c6db7f804d6b50f41b10bd10c10b
6c7b8837d3 ci: Always run merge_base_script first for consistency (Hennadii Stepanov)
3ad5ace351 ci: Revamp Android SDK cache, and update it timely (Hennadii Stepanov)
e988bc7f2e ci: Make macOS SDK cache independent, and update it timely (Hennadii Stepanov)
Pull request description:
On master (681b25e3cd) SDK (macOS and Android) are not invalidated timely.
For instance, during updating macOS SDK in #21851 it is not possible to run a related CI task successfully.
Another flaw is related to Android SDK -- the following commands 681b25e3cd/ci/test/05_before_script.sh (L25-L32) run regardless of cache state. See CI task [log](https://api.cirrus-ci.com/v1/task/5962074015662080/logs/ci.log) for details.
This PR:
- drops caching of `depends/sdk-sources` in favor of separated caching of `depends/SDKs/$MACOS_SDK` and `depends/SDKs/android`
- establishes conditions for cache invalidation
- bypass unneeded commands when a cache is populated
ACKs for top commit:
fanquake:
ACK 6c7b8837d3
Tree-SHA512: e5a5ebefd47fad495f08b8f821573977c1b8a906b3424396973bdf41da255446edbcbd43ea4f4f837f77ac95380f32e436eb75944f3457ec995057445b3b215e
a99ed89865 psbt: sign without finalizing (Andrew Chow)
Pull request description:
It can be useful to sign an input with `walletprocesspsbt` but not finalize that input if it is complete. This PR adds another option to `walletprocesspsbt` to be able to do that. We will still finalize by default.
This does not materially change the PSBT workflow since `finalizepsbt` needs to be called in order to extract the tx for broadcast.
ACKs for top commit:
meshcollider:
utACK a99ed89865
Sjors:
utACK a99ed89
Tree-SHA512: c88e5d3222109c5f4e763b1b9d97ce4655f68f2985a4509caab2d4e7f5bac5047328fd69696e82a330f5c5a333e0312568ae293515689b77a4747ca2f17caca6
2bc51c5c32 [tracing] tracepoints to utxocache add, spent and uncache (Arnab Sen)
a26e8eef43 [tracing] tracepoint for utxocache flushes (Arnab Sen)
Pull request description:
This PR adds some of the UTXO set cache tracepoints proposed in https://github.com/bitcoin/bitcoin/issues/20981#issuecomment-802688809. The first tracepoints were added in bitcoin#22006.
tracepoint | description
-- | --
`utxocache:flush` | Is called after the caches and indexes are flushed
`utxocache:add` | when a new coin is added to the UTXO cache
`utxocache:spent` | when a coin is spent
`utxocache:uncache` | when coin is removed from the UTXO cache
The tracepoints are further documented in `docs/tracing.md` and the usage is shown via the two newly added example scripts in `contrib/tracing/`.
ACKs for top commit:
laanwj:
Code and documentation review ACK 2bc51c5c32
Tree-SHA512: d6b4f435d3260de4c48b36956f9311f65ab3b52cd03b1e0a4ba9cf47a774d8c4b31878e222b11e0ba5d233a68f7567f8a367b12a6392f688c10c11529341e837
988024fe37 test: add check_node_connections in util (brunoerg)
Pull request description:
This function (`check_node_connections`) exists in `feature_anchors.py` and `p2p_add_connections.py` and does the same thing in both, this PR moves this function to util to avoid repetition and facilitate maintenance.
ACKs for top commit:
laanwj:
Code review ACK 988024fe37
Tree-SHA512: bf86c5659933539c72cb91ad587552b45c918be74d36fb429e78f3b954f01ed0855a85dd49aea35b432fbd18227c05eb3fec8b99c139c3509c39b19bccf6b7fd
fa2c991ec9 refactor: Use underlying type of isminetype for isminefilter (MarcoFalke)
Pull request description:
This does not change behavior, but it would be good for code clarity and to avoid `-Wimplicit-int-conversion` compiler warnings to use the an int of the same width for both `isminetype` and `isminefilter`.
ACKs for top commit:
laanwj:
Code review ACK fa2c991ec9
shaavan:
crACK fa2c991ec9
promag:
Code review ACK fa2c991ec9.
Tree-SHA512: b3e255de7c9b1dea272bc8cb9386b339fe701f18580e03e997c270cac6453088ca2032e26e39f536d66cd1b6fda3e96bdbdc6e960879030e635338d0916277e6
459e208276 Exit early for an empty vChecks in CCheckQueue::Add (Hennadii Stepanov)
c43aa62343 Avoid excessive lock contention in CCheckQueue::Add (Hennadii Stepanov)
Pull request description:
This PR significantly reduces lock contention in the `CCheckQueue` class by releasing a mutex before calling `std::condition_variable::notify_one` and `std::condition_variable::notify_all`.
From C++ [docs](https://en.cppreference.com/w/cpp/thread/condition_variable/notify_one):
> The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization, since the notified thread would immediately block again, waiting for the notifying thread to release the lock.
Related to:
- #23167
- #23223
ACKs for top commit:
martinus:
ACK 459e208, codereview and tested. I first thought this introduced a segfault in `psbt_wallet_tests/psbt_updater_test` because that test failed for me, but thats a different issue fixed in #23403.
vasild:
ACK 459e208276
theStack:
Code-review ACK 459e208276
Tree-SHA512: c197858656392ba3ebcd638d713cf93c9fb48b7b3bad193209490d2828f9c7e3ae4dee6f84674f2f34dceed894139562e29579ee7299e06756c8c990caddc5ed
b79dbe86a9 test: add feature_rbf.py --descriptors to test_runner.py (Sebastian Falbesoner)
166f8ec28e test: always rescan after importing private keys in `init_wallet` helper (Sebastian Falbesoner)
Pull request description:
The functional test feature_rbf.py currently fails on master branch, if descriptor wallets are used (argument `--descriptors`). This is due to the fact that in this case, a call to the helper `init_wallet`
111c3e06b3/test/functional/test_framework/test_framework.py (L428-L434)
creates a wallet without rescanning the blockchain; the test framework maps the importprivkey RPC calls to the importdescriptors RPC without rescanning by default (timestamp='now'). Fix this by always calling with `rescan=True`, which calls importdescriptors with timestamp=0. Also add `feature_rbf.py --descriptors` to the list of the test runner's calls.
Fixes#23563.
ACKs for top commit:
mjdietzx:
ACK b79dbe86a9
Tree-SHA512: a3f3f7a4077066e3c910919d3b5e04bc6b580c1e0a06e9a2fc258950eaea5e59c0f805c8f00432aea722609f2f7e41eebfab653471b76729c5a316825a3d8c86
f13a22a631 wallet: Call load handlers without cs_wallet locked (João Barbosa)
Pull request description:
Don't have `cs_wallet` locked while calling each `context.wallet_load_fns`. A load handler can always lock `cs_wallet` if needed.
The lock was added in 1c7e25db0c to satisfy TSAN. With 44c430ffac most of the code requiring the lock is in `CWallet::AttachChain`. A comment is added to warn about wallets_mutex and cs_wallet lock ordering.
ACKs for top commit:
meshcollider:
re-utACK f13a22a631
ryanofsky:
Code review ACK f13a22a631. Only change since last review is adding a lock order comment
jonatack:
ACK f13a22a631
Tree-SHA512: d51976c3aae4bebc2d1997c88edff712d21fc5523801f5614062a10f826e164579973aeb1981bb1cbc243ecff6af3250362f544c02a79e5d135cbbca1704be62
