501a3dd4ad4a545a05663a78cec61575966045c7 walletdb: hash pubkey/privkey in one shot to avoid leaking secret data (Sebastian Falbesoner)
Pull request description:
In several places in the wallet DB module, byte strings containing serialized public keys and secret keys are created in order to be hashed. To avoid sensitive data lingering in memory (and potentially leaking), don't store the preimage, but hash both public key and secret key in one shot, using the overloaded `Hash` function:
d198635fa2/src/hash.h (L82-L88)
See e.g. #31166 and #31774 for similarly themed PRs (Note that in #31166 we used the explicit `memory_cleanse` approach though, as changing the allocator was not possible.)
ACKs for top commit:
davidgumberg:
crACK 501a3dd4ad
furszy:
ACK 501a3dd4ad4a545a05663a78cec61575966045c7
rkrux:
ACK 501a3dd
theuni:
ACK 501a3dd4ad4a545a05663a78cec61575966045c7
Tree-SHA512: 8a71685b26bf89fca181aed6512a8db843b6d1dc740a468bb33fb2a629a23167a9676c228d1077ad8db2df9db80f47e32ec013737e93df8ee6f4ba505d3d50c9
fec58229fa671cb870ebf795b54b73b7e22a1eb6 contrib: Update fixed feeds (Ava Chow)
27fbdb009f2f007239c4845cad528487b30598bd makeseeds: Choose node info with most recent success when deduplicating (Ava Chow)
982883a1bc4d03efb78d296849a3b8fa04ded26d makeseeds: Update known user agents (Ava Chow)
Pull request description:
ACKs for top commit:
fjahr:
ACK fec58229fa671cb870ebf795b54b73b7e22a1eb6
Tree-SHA512: 2852a9a6a7c299ce04ee4dc438af9547d56a860858201ad2ccdea14640b17876e7e9841ce3a30030e2482cd04e9b386f7ede5c4e51582ebd09b9ce0a2a0bc43b
89386e700ebc232e2beab3a3f3ea0d1ae78ac203 kernel: Use fs:: namespace and unicode path in kernel tests (sedited)
Pull request description:
Add support for unicode characters in paths to the kernel tests by using our fs:: wrappers for std::filesystem calls and adding the windows application manifest to the binary. This exercises their handling through the kernel API.
ACKs for top commit:
hebasto:
ACK 89386e700ebc232e2beab3a3f3ea0d1ae78ac203.
w0xlt:
ACK 89386e700ebc232e2beab3a3f3ea0d1ae78ac203
Tree-SHA512: 7b541f482d84a66c89eec63aea0e7f7626bbbd62082ad7a7fb2c7a517296c291a6ff301c628e5e9e1d7b850ead89005141481a2bfd06d8a9081622e32f7340cc
faa016af542763492a63de8cc972f8f4b52a58cd refactor: Use aliasing shared_ptr in Sock::Wait (MarcoFalke)
Pull request description:
Currently, a no-op lambda is used as the deleter for the temporary shared pointer helper in `Sock::Wait`. This is perfectly fine, but has a few style issues:
* The lambda needs to be allocated on the heap
* It triggers a false-positive upstream GCC-16-trunk bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=123912
Fix all issues by just using an aliasing shared pointer, which points to `this`, but is otherwise empty (sits on the stack without any heap allocations).
ACKs for top commit:
hodlinator:
ACK faa016af542763492a63de8cc972f8f4b52a58cd
sedited:
ACK faa016af542763492a63de8cc972f8f4b52a58cd
vasild:
ACK faa016af542763492a63de8cc972f8f4b52a58cd
Tree-SHA512: b7330862204e79fb61f30694accb16f9a24e5722bd0ceb098ca27c877cff921afa00c0cfd953d4cbb355e6433706961a25b628efefdbe0b48bdec2941eaaee7a
a067ca34106817565e02daca52b3175266714c25 [doc] coin selection filters by max cluster count, not descendant (glozow)
f7be5fb8fc7d2a5831810a0b91666fc774b64b8f [refactor] rename variable to clarify it is unused and cluster count (glozow)
Pull request description:
Followup to #33629.
Fix misleading docs and variable names. Namely, `getTransactionAncestry` returns the cluster count, not max descendant count of ancestor set (not worth reimplementing as it is merely a heuristic). No behavior changes - I don’t think much needs to be changed for the first release containing cluster mempool.
Current `CoinEligibilityFilter`s enforce a maximum ancestor count (summed across all outputs, potentially overestimating) and max descendant count across ancestors of the output.
Since #33629, these filters have started using cluster count instead of max desc count across ancestors. The change isn’t dangerous, as the cluster count bounds descendant count as well. Currently, the wallet is essentially enforcing a mixture of both limits - this is good while we are transitioning. Note that the cluster count enforced is 25, not 64, since it's grabbing the node's descendant count limit. While it is not an apples-to-apples comparison, a cluster count limit of 25 helps us avoid busting legacy descendant limits (which will be common on the network for a while).
Potential things for the future, out of scope for this PR:
- When we get rid of the ancestor/descendant config options, `getPackageLimits` can probably be replaced with hard-coded values.
- Change the `OutputGroup`s to track the actual cluster count that results from spending these outputs and merging their clusters.
- Loosen from 25 after that policy is no longer common.
- Clean up `getPackageLimits`.
ACKs for top commit:
achow101:
ACK a067ca34106817565e02daca52b3175266714c25
ismaelsadeeq:
reACK a067ca34106817565e02daca52b3175266714c25
rkrux:
crACK a067ca34106817565e02daca52b3175266714c25
Tree-SHA512: d7cacd5bf668d42e26e8b83e42a42c280929c3bfd554c3db1de605e5939f8b36c14ecfd2839abeb4eec352363df1891b3420a693c250916391ab10a5ce26396b
Avoid storing the privkey in a vector, which could linger in memory
and potentially leak sensitive data. An alternative approach is to
use `secure_allocator` for the `std::vector` instances, but this
commit has the advantage of also deduplicating code at the same shot.
Thanks to @theuni for suggesting this.
44538f8ada868cb73ffa7a4bfb5267046d3f6b85 kernel: Add recent assumeutxo snapshot info (Ava Chow)
58c2e23fca05d201dfcea092ab032c943c9d5665 kernel: Update headerssync params (Ava Chow)
cf261b071f68b1967077a6d3baac10ea024e653a kernel: update chainTxData (Ava Chow)
8eaf1d26d446ecdc5c69b61334ee51a6f41050d3 kernel: update defaultAssumeValid and minimumChainWork (Ava Chow)
5ca0c55517d4c8e41a1dd9904545d82ee09adedf kernel: update assumed blockchain and chainstate sizes (Ava Chow)
Pull request description:
Update chainparams and headerssync params per the release process.
Also added new assumeutxo snapshots for each network. I've uploaded snapshots to https://achow101.com/files/utxo-snapshots/
ACKs for top commit:
Sjors:
ACK 44538f8ada868cb73ffa7a4bfb5267046d3f6b85
fjahr:
ACK 44538f8ada868cb73ffa7a4bfb5267046d3f6b85
janb84:
ACK 44538f8ada868cb73ffa7a4bfb5267046d3f6b85
sipa:
ACK 44538f8ada868cb73ffa7a4bfb5267046d3f6b85. I re-did all the mainnet parameters, but did not look closely at the other networks.
jaonoctus:
ACK 44538f8ada868cb73ffa7a4bfb5267046d3f6b85
Tree-SHA512: f9b6ccc967c5ef58f734245df459c3136491e9b6a0f6e36f4272bc0787e7b59eabe47a8c8b19a90267eca4a0b5851dfbf45153f96eac599c417f148b3cf264cf
f580cc7e9f26331f7f03a8bbc5722521eb159bb2 doc: Fix `fee` field in `getblock` RPC result (nervana21)
Pull request description:
The `fee` field in the `getblock` RPC result (verbosity 2 and 3) may be omitted when block undo data is not available. Marking it optional in the `RPCResult` aligns the documented schema with the runtime behavior.
ACKs for top commit:
mercie-ux:
ACK f580cc7e9f
satsfy:
ACK f580cc7e9f
instagibbs:
ACK f580cc7e9f26331f7f03a8bbc5722521eb159bb2
w0xlt:
ACK f580cc7e9f26331f7f03a8bbc5722521eb159bb2
luke-jr:
ACK f580cc7e9f26331f7f03a8bbc5722521eb159bb2
Tree-SHA512: e3b44a48e17e21b906967aef124688a34aea2c6af3b6df3c47693fd3002d33e824f764c0060a7ab07751b98567c29eb16f3b3c07bf2999db080ff7adfd087dfd
44feab23a7c6060a3b432c04e3f952c5a7104325 script: Fix undefined behavior in Clone() -- std::transform writes past end of empty vector (Weixie Cui)
Pull request description:
# Motivation
This patch fixes undefined behavior in Clone() in src/script/descriptor.cpp.
When std::transform is used with providers.begin() or subdescs.begin() as the output iterator, the vectors have been reserve()d but have size 0. Writing through begin() in that case writes past the logical end of the vector, which is undefined behavior.
ACKs for top commit:
maflcko:
lgtm ACK 44feab23a7c6060a3b432c04e3f952c5a7104325
rkrux:
ACK 44feab23a7c6060a3b432c04e3f952c5a7104325 because it gets rid of the possible undefined behaviour.
frankomosh:
Code Review ACK 44feab23a7c6060a3b432c04e3f952c5a7104325. Fix seems minimal and correct.
Tree-SHA512: 8af3b6d97c139b32bd47d4c452b6b16befdaa7028a7bc1b6de0ab1f0a8cb35eb068710316a2c07fa60856e17e25307931aa3125b4f41d0fe7726b435483a52db
4ae9a10ada95ab8c1ab01472948d348d9538f3bb doc: add release notes for dbcache bump (Andrew Toth)
c510d126efb6ce9623e0e005829c13a110f65b0e doc: update dbcache default in reduce-memory.md (Andrew Toth)
027cac852796f643e2b6036fd3652dd3ca7785a5 qt: show GetDefaultDBCache() in settings (Andrew Toth)
5b34f251847a3c66b444959800b70e9863b10bbe dbcache: bump default from 450MB -> 1024MB if enough memory (Andrew Toth)
Pull request description:
Alternative to #34641
This increases the default `dbcache` value from `450MiB` to `1024MiB` if:
- `dbcache` is unset
- The system is 64 bit
- At least 4GiB of RAM is detected
Otherwise fallback to previous `450MiB` default.
This should be simple enough to get into v31. The bump to 1GiB shows significant performance increases in #34641. It also alleviates concerns of too high default for steady state, and of lowering the current dbcache for systems with less RAM.
This change only changes bitcoind behavior, while kernel still defaults to 450 MiB.
ACKs for top commit:
ajtowns:
ACK 4ae9a10ada95ab8c1ab01472948d348d9538f3bb
kevkevinpal:
reACK [4ae9a10](4ae9a10ada)
svanstaa:
ACK [4ae9a10](4ae9a10ada)
achow101:
ACK 4ae9a10ada95ab8c1ab01472948d348d9538f3bb
sipa:
ACK 4ae9a10ada95ab8c1ab01472948d348d9538f3bb
Tree-SHA512: ee3acf1fb08523ac80e37ec8f0caca226ffde6667f3a75ae6f4f4f54bc905a883ebcf1bf0e8a8a15c7cfabff96c23225825b3fff4506b9ab9936bf2c0a2c2513
20ae9b98eab20117344cf31f7cde39cadd70ca22 Extend functional test for setBlockIndexCandidates UB (marcofleon)
854a6d5a9a0e40329a2852efb2a8dfec4b54886e validation: fix UB in LoadChainTip (marcofleon)
9249e6089ec4e2eb63f0896961f04d9dbe14651a validation: remove LoadChainTip call from ActivateSnapshot (marcofleon)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/issues/34503. See this issue for more details as well.
Fixes a bug where, under certain conditions, `setBlockIndexCandidates` had blocks in it that were worse than the tip. The block index candidate set uses `nSequenceId` as a sort key, so modifying this field while blocks are in the set results in undefined behavior. This PR populates `setBlockIndexCandidates` after the `nSequenceId` modifications, avoiding the UB.
ACKs for top commit:
achow101:
ACK 20ae9b98eab20117344cf31f7cde39cadd70ca22
sedited:
Re-ACK 20ae9b98eab20117344cf31f7cde39cadd70ca22
sipa:
Code review ACK 20ae9b98eab20117344cf31f7cde39cadd70ca22
Tree-SHA512: 121c170bb70fb6365089d578db63c811e7926e129d7206e569947f7a1f6c5ddc8d5f4937b80f1ba1b7d7daa42789b143ca5b56f154b7ab968a1cd55f925f378d
97e7e79435c69e90cb7f056c704c275421bf0892 test: Enable `system_tests/run_command` "stdin" test on Windows (Hennadii Stepanov)
a4324ce09546d80ab847dbfce715f015139ed593 test: Remove `system_tests/run_command` runtime dependencies (Hennadii Stepanov)
Pull request description:
`system_tests` currently rely on `cat`, `echo`, `false` and `sh` being available in `PATH` at runtime.
This PR:
1. Removes these dependencies.
2. Reduces the number of platform-specific code paths.
The change is primarily motivated by my work on maintaining the [`bitcoin-core`](https://packages.guix.gnu.org/packages/bitcoin-core) package in Guix. It enables the removal of the existing `bash` and `coreutils` native inputs, which in turn makes it possible to drop the implicit dependency on `qtbase@5` (see https://codeberg.org/guix/guix/pulls/4386#issuecomment-8613333).
ACKs for top commit:
maflcko:
re-ACK 97e7e79435c69e90cb7f056c704c275421bf0892 👓
janb84:
ACK 97e7e79435c69e90cb7f056c704c275421bf0892
sedited:
ACK 97e7e79435c69e90cb7f056c704c275421bf0892
Tree-SHA512: 1375c676f85c75d571df1ddfc3a4405767dbf0ed7bfea2927c93ec01b29f9f7ae3383e546d2658f595e8ffafa9ab20bba6fcc628a9f5ebdb288bbef03b645fb6
15c4889497b96037e41019a8f43090af841b36ec index: document TxoSpenderIndex::FindSpender (furszy)
f8b9595aaa966c373b02e6227dc799fed6d038ba test: Add missing txospenderindex coverage in feature_init (Fabian Jahr)
a1074d852a7a46b746fb4ed90d94cb4cc346f9b3 index, rpc, test: Misc formatting fixes (Fabian Jahr)
Pull request description:
This addresses my own comments in the last review of #24539: https://github.com/bitcoin/bitcoin/pull/24539#pullrequestreview-3829110465
The first commit fixes three small formatting errors.
The second commit adds some missing coverage in `feature_init` and refactors the code a bit as well so these misses don't happen so easily in the future.
The third commit is by furzy:
> TxoSpenderIndex::FindSpender returns an Expected<optional<TxoSpender>> but
the two levels of the return type were undocumented, making it unclear what a returned
nullopt means. So added doc clarifying each return case.
ACKs for top commit:
furszy:
ACK 15c4889497b96037e41019a8f43090af841b36ec
sedited:
ACK 15c4889497b96037e41019a8f43090af841b36ec
rkrux:
crACK 15c4889497b96037e41019a8f43090af841b36ec
Tree-SHA512: 2e0f060a54b558d2967ebae0835cf81bd86c2d8d983d670a48d1bee7d347f186623e75db7ae311ca1566807f715c1b3fa67cf734c9467d35e13b84d082f28253
bff8a7a80d2a198e84443a696438925d4c303c7b subprocess: replace __USING_WINDOWS__ with WIN32 (kevkevinpal)
Pull request description:
## Summary
Motivated by https://github.com/bitcoin/bitcoin/pull/34385#pullrequestreview-3826616188
In `subprocess.h` we are now renaming `__USING_WINDOWS__` with `WIN32`
In the rest of the codebase, we are using `WIN32`, so it makes sense to update `subprocess.h` to match that.
---
Use the following `grep` to assert there is no `__USING_WINDOWS__` in the codebase
```
grep -nri --exclude-dir=build "WIN32" ./ -I
rep -nri --exclude-dir=build "__USING_WINDOWS__" ./ -I
```
ACKs for top commit:
sedited:
ACK bff8a7a80d2a198e84443a696438925d4c303c7b
hebasto:
ACK bff8a7a80d2a198e84443a696438925d4c303c7b, I have reviewed the code and it looks OK.
Tree-SHA512: 18c3c8b87cf880053bbf69f837a0a135c5da51cfb15ab1d9fd554d8f931b2ea8202cf0f4d5e6f317d6234480128c2f41a7a1a9d9bd0504697a3c4c6a21797762
The removal of the chain tip from setBlockIndexCandidates was
happening after nSequenceId was modified. Since the set uses
nSequenceId as a sort key, modifying it while the element is in the
set is undefined behavior, which can cause the erase to fail.
With assumeutxo, a second form of UB exists: two chainstates each
have their own candidate set, but share the same CBlockIndex
objects. Calling LoadChainTip on one chainstate mutates nSequenceIds
that are also in the other chainstate's set.
Fix by populating setBlockIndexCandidates after all changes to
nSequenceId.
This call is a no-op. PopulateAndValidateSnapshot already sets both
the chain tip and the coins cache best block to the snapshot block,
so LoadChainTip always hits the early return when it finds that the
two match (tip->GetBlockHash() == coins_cache.GetBestBlock()).
f51665bee72c26d3f3cc6813b6c02adad5f0af6a psbt: validate pubkeys in MuSig2 pubnonce/partial sig deserialization (tboy1337)
Pull request description:
The previous fix for invalid MuSig2 pubkeys (bitcoin/bitcoin#34010) only
addressed the PSBT_IN_MUSIG2_PARTICIPANT_PUBKEYS field. However, the
PSBT_IN_MUSIG2_PUB_NONCE and PSBT_IN_MUSIG2_PARTIAL_SIG fields also
deserialize pubkeys without validation, which could lead to crashes when
invalid pubkeys are processed.
This commit adds validation to the DeserializeMuSig2ParticipantDataIdentifier
function to ensure all pubkeys in MuSig2 pubnonce and partial signature
fields are fully valid elliptic curve points.
The fix:
- Validates both aggregate and participant pubkeys in MuSig2 pubnonce and
partial signature deserialization
- Throws std::ios_base::failure with descriptive error messages for invalid
pubkeys
- Prevents potential crashes from invalid elliptic curve points
- Maintains backward compatibility for valid PSBTs
This completes the fix for issues [#33999](https://github.com/bitcoin/bitcoin/issues/33999) and [#34201](https://github.com/bitcoin/bitcoin/issues/34201).
ACKs for top commit:
rkrux:
lgtm ACK f51665bee72c26d3f3cc6813b6c02adad5f0af6a
w0xlt:
ACK f51665bee7
darosior:
utACK f51665bee72c26d3f3cc6813b6c02adad5f0af6a
Tree-SHA512: 8454d77a05aa003a3121b0a5975e8a000125ee0d62343bfa625a75db113358ba7a210ae0376ca1666957b7de7005e06e5a54c95170430ee5e9e1416719b8af53
Add support for unicode characters in paths to the kernel tests by using
our fs:: wrappers for std::filesystem calls and adding the windows
application manifest to the binary. This exercises their handling
through the kernel API.
98e8af4bb991fd8edeb15c0fb8afa66bff6b5cac wallet: Drain validation interface queue after notifications disconnect (Ava Chow)
52992ebe1c55c8f7219b824f05d22fbc18acb794 interfaces: Add waitForNotifications() to call SyncWithValidationInterfaceQueue() (Ava Chow)
Pull request description:
When the wallet disconnects chain notifications, it is expecting no further notifications to execute, but this is not the case. This results in test failures such as in #34354. Instead of disconnecting the notifications and continuing shutdown, we should wait for the validation interface queue to be drained before the rest of wallet shutdown. This is achieved by adding an `interfaces::Chain::waitForNotifications()` function which calls `SyncWithValidationInterfaceQueue()`.
Fixes#34354
ACKs for top commit:
stickies-v:
utACK 98e8af4bb991fd8edeb15c0fb8afa66bff6b5cac
furszy:
ACK 98e8af4bb991fd8edeb15c0fb8afa66bff6b5cac
rkrux:
crACK 98e8af4bb991fd8edeb15c0fb8afa66bff6b5cac
sedited:
ACK 98e8af4bb991fd8edeb15c0fb8afa66bff6b5cac
Tree-SHA512: 263628556f740cb633d3970c22a0dfdb52a644bd1d0cd5a69c2970524edbb0e25d592cb39fc9bf1d0c281eebce09578526e2958dffee9026fc7473db35bd0dec
da7f70a5322843b70f29456a8bc2227209a0718b test: use port 0 for I2P addresses in p2p_private_broadcast.py (Vasil Dimov)
a8ebcfd34c63f142064b4f5ef7d52299739d4cd6 test: let connections happen in any order in p2p_private_broadcast.py (Vasil Dimov)
67696b207f370e902c8d5fb765e4ff10f6c9e1b4 net: extend log message to include attempted connection type (Vasil Dimov)
Pull request description:
If the following two events happen:
* (likely) the automatic 10 initial connections are not made to all
networks
* (unlikely) the network-specific logic kicks in almost immediately.
It is using exponential distribution with a mean of 5 minutes
(`rng.rand_exp_duration(EXTRA_NETWORK_PEER_INTERVAL)`).
So if both happen, then the 11th connection may not be the expected
private broadcast, but a network-specific connection.
Fix this by retrieving the connection type from
`destinations_factory()`. This is more flexible because it allows
connections to happen in any order and does not break if e.g. the 11th
connection is not the expected first private broadcast.
This also makes the test run faster:
before: 19-44 sec
now: 10-25 sec
because for example there is no need to wait for the initial 10
automatic outbound connections to be made in order to proceed.
Fixes: https://github.com/bitcoin/bitcoin/issues/34387
ACKs for top commit:
achow101:
ACK da7f70a5322843b70f29456a8bc2227209a0718b
andrewtoth:
ACK da7f70a5322843b70f29456a8bc2227209a0718b
mzumsande:
Code Review ACK da7f70a5322843b70f29456a8bc2227209a0718b
Tree-SHA512: 7c293e59c15c148a438e0119343b05eb278205640658c99336d4caf4848c5bae92b48e15f325fa616cbc9d5f394649abfa02406a76e802cffbd3d312a22a6885
The `fee` field in the `getblock` RPC result (verbosity 2 and 3) may be
omitted when block undo data is not available. Marking it optional in
the `RPCResult` aligns the documented schema with the runtime behavior.
bbc8f1e0a7e5739f15b2e646a4ace338083309a3 ipc mining: Prevent ``Assertion `m_node.chainman' failed`` errors on early startup (Ryan Ofsky)
a7cabf92e4de83c87f6b9274ddd2fb70712d29f8 init refactor: Only initialize node.notifications one time (Ryan Ofsky)
c8e332cb33594cc307cdf89b6036a0e95c238cd8 init refactor: Remove node.init accesss in AppInitInterfaces (Ryan Ofsky)
Pull request description:
This fixes ``Assertion `m_node.chainman' failed`` errors first reported https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602551596 when IPC mining methods are called before ChainstateManager is loaded.
The fix works by making the `Init.makeMining` method wait until chainstate data is loaded. It's probably the simplest possible fix but other alternatives like moving the wait to `Mining.createNewBlock` were discussed in the thread https://github.com/bitcoin/bitcoin/pull/34661#discussion_r2848176298 and could be implemented later without changes to clients.
ACKs for top commit:
Sjors:
utACK bbc8f1e0a7e5739f15b2e646a4ace338083309a3
ismaelsadeeq:
ACK bbc8f1e0a7e5739f15b2e646a4ace338083309a3
achow101:
ACK bbc8f1e0a7e5739f15b2e646a4ace338083309a3
Tree-SHA512: 3e2e4e28ccff364b2303efd06ce337a229c28609076638500acb29559f716a15ad99409c6970ce9ad91776d53e3f9d959f1bbbd144ea9a4a2fb578ddbf2da267
When unloading a wallet, there may be unexecuted callbacks in the
validation interface queue that can still execute after we have
completed all of the other wallet shutdown tasks. Instead of letting
these run in the background, once the notifications are disconnected,
wait for the queue to drain before continuing with wallet shutdown.
fa18be2f2ba19d5d35cb8a04fd4e1a7c4fc441ce test: Fix typo (MarcoFalke)
fac932698f6539e9ac4a13df51194bcb60d5d933 ci: Set TEST_RUNNER_PORT_MIN in test-each after cirrus runner switch (MarcoFalke)
Pull request description:
This is needed after the recent switch to cirrus runners in the task in commit c8c9c1e61759f689615a304254fed33cda7f895e.
Otherwise, the CI will fail:
```
node1 stderr Error: Unable to bind to 127.0.0.1:12321 on this computer. Bitcoin Core is probably already running.
```
(https://github.com/bitcoin/bitcoin/actions/runs/22398358349/job/64837827234?pr=31723#step:9:2605)
Also, include a random second commit, so that the CI task is run in this pull.
ACKs for top commit:
l0rinc:
ACK fa18be2f2ba19d5d35cb8a04fd4e1a7c4fc441ce
willcl-ark:
ACK fa18be2f2ba19d5d35cb8a04fd4e1a7c4fc441ce
Tree-SHA512: 6b63f645bf62d3e951ca155cddf3dc562b7ce675ccae4f9179e2202679685b5c147844eb350bd219b173fe2bb976376d0caa073d3e827a48c13aa015f4745b2c
fa6af856341384e4a84c5674e66fe7c1f13dd73c refactor: Use static_cast<decltype(...)> to suppress integer sanitizer warning (MarcoFalke)
fa692974ac2d69e01091f03625cd8a227e310065 util: Fix UB in SetStdinEcho when ENOTTY (MarcoFalke)
Pull request description:
The call to `tcgetattr` may fail with `ENOTTY`, leaving the struct possibly uninitialized (UB).
Fix this UB by returning early when `isatty` fails, or when `tcgetattr` fails. (Same for Windows)
This can be tested by a command that fails valgrind before the change and passes after:
```
echo 'pipe' | valgrind --quiet ./bld-cmake/bin/bitcoin-cli -stdinrpcpass uptime
ACKs for top commit:
achow101:
ACK fa6af856341384e4a84c5674e66fe7c1f13dd73c
l0rinc:
lightly tested code review ACK fa6af856341384e4a84c5674e66fe7c1f13dd73c
sedited:
ACK fa6af856341384e4a84c5674e66fe7c1f13dd73c
Tree-SHA512: 76e2fbcb6c323b17736ee057dbd5e932b2e8cbb7d9fe4488c1dc7ab6ea928a3cde7e72ca0a63f8c8c78871ccb8b669263b712c0e1b530d88f2d45ea41f071201
408d5b12e80151feded31c2a5509e2dc5f15edf3 test: include response body in non-JSON HTTP error msg (Matthew Zipkin)
9dc653b3b4f3049b0e742499b762f7c13bb006cc test: threadpool, add coverage for all Submit() errors (furszy)
ce2a984ee324d37ba1dd7c2c4e27e40e0508bedc test: cleanup, use HasReason in threadpool_tests.cpp (l0rinc)
d9c6769d0324b65121935b7c8a285c6421fe74a6 test: refactor, decouple HasReason from test framework machinery (furszy)
dbbb780af02d850a1f9257f18610cfb9de9cb828 test: move and simplify BOOST_CHECK ostream helpers (Hodlinator)
3b7cbcafcb9b318bf1fa00a3499f514c5ebe9bb6 test: ensure Stop() thread helps drain the queue (seduless)
ca101a2315774f0ed65da633ba99899fd0dad740 test: coverage for queued tasks completion after interrupt (furszy)
bf2c607aaa22d253b9367c11b0a198bd4244ad2f threadpool: active-wait during shutdown (furszy)
e88d2744301a434064714f0a21e1395d41ac3984 test: add threadpool Start-Stop race coverage (furszy)
8cd4a4363fb85f5487a19ace82aa0d12d5fab450 threadpool: guard against Start-Stop race (furszy)
9ff1e82e7dbdf31ddf1c534853da4581a1f41bd5 test: cleanup, block threads via semaphore instead of shared_future (l0rinc)
Pull request description:
A few follow-ups to #33689, includes:
1) `ThreadPool` active-wait during shutdown:
Instead of just waiting for workers to finish processing tasks, `Stop()` now helps them actively.
This speeds up the JSON-RPC and REST server shutdown, resulting in a faster node shutdown when many requests remain unhandled. This wasn't included in the original PR due to the behavior change this introduces.
2) Decouple `HasReason` from the unit test framework machinery
This avoids providing the entire unit test framework dependency to low-level tests that only require access to the `HasReason` utility class. Examples are: `reverselock_tests.cpp`, `sync_tests.cpp`, `util_check_tests.cpp`, `util_string_tests.cpp`, `script_parse_tests.cpp` and `threadpool_tests.cpp`. These tests no longer gain access to unnecessary components like the chainstate, node context, caches, etc. It includes l0rinc's `threadpool_tests.cpp` `HasReason` changes.
3) Include response body in non-JSON HTTP error messages
Straight from pinheadmz [comment](https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2783817192), it makes debugging CI issues easier.
ACKs for top commit:
maflcko:
review ACK 408d5b12e80151feded31c2a5509e2dc5f15edf3 🕗
achow101:
ACK 408d5b12e80151feded31c2a5509e2dc5f15edf3
hodlinator:
re-ACK 408d5b12e80151feded31c2a5509e2dc5f15edf3
Tree-SHA512: 57aa0ef96886f32bf95a0bd7f87c878d31c9df9e34cb96de615eee703ce0824b5cfdf8f5c9cd19a3594559994295b5810c38c94f5efd6291cbbd83a95473357a
Avoid providing the entire unit test framework dependency to tests that only
require access to the HasReason utility class.
E.g. reverselock_tests.cpp, sync_tests.cpp, util_check_tests.cpp, util_string_tests.cpp,
and script_parse_tests.cpp only require access to HasReason and nothing else.
Move the operator<< overloads used by BOOST_CHECK_* out of the
unit test machinery test/setup_common, into test/util/common.h.
And replace the individual per-type ToString() overloads with
a single concept-constrained template that covers any type
exposing a ToString() method. This is important to not add
uint256.h and transaction_identifier.h dependencies to the
shared test/util/common.h file.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
Instead of waiting for the workers to finish processing tasks, help
them actively inside Stop().
This speeds up the JSON-RPC and REST server shutdown procedure,
and results in a faster node shutdown when many requests remain unhandled
