3f44f9aef7 test: Add coverage for m_blocks_unlinked invariant in LoadBlockIndex (marcofleon)
0e4b0bacec validation: Don't add pruned blocks to m_blocks_unlinked on startup (marcofleon)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/35050
The `m_blocks_unlinked` map keeps track of blocks that have transactions but whose parent (or any ancestor) does not. This happens when a block is received before its parent, or during a reorg, when `FindMostWorkChain()` encounters a block whose ancestors were pruned.
The bug this PR addresses is a rare interaction of these two cases, which happens on startup when `BlockManager::LoadBlockIndex()` rebuilds `m_blocks_unlinked`. The check there only considers whether a block has transactions, and pruned blocks keep `nTx > 0` but clear `BLOCK_HAVE_DATA`. So if there's a pruned block on a stale fork whose parent has no transactions, that block is added to `m_blocks_unlinked` without having data on disk. This violates an [assertion](ad3f73862b/src/validation.cpp (L5352)) in `CheckBlockIndex()`.
Get rid of this unintended case by gating on `BLOCK_HAVE_DATA` before adding to `m_blocks_unlinked`.
ACKs for top commit:
achow101:
ACK 3f44f9aef7
sedited:
Re-ACK 3f44f9aef7
stratospher:
ACK 3f44f9a. nice!
Tree-SHA512: 275d0f8588524c01c4e701c8635973cd4a086d31c10d252a498c1ef668bdb3895ba1cae265dbe88f8983ca7ddbe32247824753c7c1f49e59c8bce0df377b784c
2189a6f5f2 p2p: Saturate LocalServiceInfo::nScore updates at INT_MAX (codeabysss)
Pull request description:
The overflow for signed arithmetic yields undefined behavior.
This changes prevents undefined behavior in local address scoring by saturating `nScore` updates at `INT_MAX` in both `SeenLocal()` and `AddLocal()` update paths.
Fixes: #24049.
ACKs for top commit:
Crypt-iQ:
ACK 2189a6f5f2 pending CI
achow101:
ACK 2189a6f5f2
sedited:
ACK 2189a6f5f2
Tree-SHA512: b861e58ec9d6e18b17768f5cbee31ee825717e1a7216c332eb6fcbe63a7ac24e213ba638aea6f03cb710d9c2d8fe736cc626f11011ed66c3938acf6c38b0ef2a
21a1380c13 key: cleanse ChainCode on destruction (Thomas)
b3a3f88346 crypto: cleanse HMAC stack buffers after use (Thomas)
Pull request description:
`CHMAC_SHA256` and `CHMAC_SHA512` leave two stack buffers populated on return: `rkey[]` holds `K' ⊕ ipad` after the constructor, and `temp[]` holds the inner-hash output after `Finalize()`.
When the HMAC is keyed with sensitive material (chain code in `BIP32Hash()` in `hash.cpp` for BIP32 child key derivation; PRK in HKDF-Expand in `hkdf_sha256_32.cpp`, used for BIP324 transport keying), `rkey` is one constant XOR from that key, and `temp` is a one-way digest covering it.
This PR cleanses both buffers with `memory_cleanse()`, matching the convention already used in `chacha20.cpp` and `chacha20poly1305.cpp`. No observable change for callers.
Update: Cleansing the HMAC primitive's internal buffers still leaves a caller's `ChainCode` value populated in memory after use. The second commit promotes `ChainCode` from `typedef uint256` to a `base_blob<256>` subclass with a `memory_cleanse()` destructor, so chain codes in `CExtKey`, `CExtPubKey`, and local variables are cleansed on scope exit. `MUSIG_CHAINCODE` is retyped from `constexpr uint256` to `const ChainCode` to match its BIP328 semantic role; this also removes the GCC-14 consteval lambda workaround.
ACKs for top commit:
davidgumberg:
crACK 21a1380c13
optout21:
ACK 21a1380c13
achow101:
ACK 21a1380c13
winterrdog:
ACK 21a1380c13
Tree-SHA512: 022c8372da3e2c9c269ef55b695d8415241acf64be04692f30da0e682dd1d05178f95601a3bd208573fd0630656b3dedcf6de34a2a3cf794515c0268e710af75
19b32a2e18 fuzz: reset the mockable steady clock between iterations (Hao Xu)
Pull request description:
Fix the issue mentioned by https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-4422112607
And this is my investigation on it: https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-4639472489
`CheckGlobalsImpl`'s constructor runs at the start of every fuzz iteration and already resets the global RNG flags and the mockable `NodeClock` (`SetMockTime(0s)`), but it never reset the mockable steady clock. A value written to `g_mock_steady_time` by one input therefore leaks into the next iteration.
The most common source is `FuzzedSock`'s constructor, which calls `SetMockTime(INITIAL_MOCK_TIME)` (through `ElapseTime(0s)`) and never clears it: once any input constructs a `FuzzedSock`, the steady clock stays mocked for every subsequent iteration in the same process. This is one of the global-state leaks tracked
in #29018.
### Fix
Reset `MockableSteadyClock` symmetrically with `NodeClock`:
```diff
g_used_system_time = false;
SetMockTime(0s);
+MockableSteadyClock::ClearMockTime();
```
Besides removing the leak, this puts the steady clock under the same discipline as the system clock: a target that reads `MockableSteadyClock::now()` without first mocking it (via `FuzzedSock`, `SteadyClockContext`, …) is now caught by the existing `g_used_system_time` check at the end of the iteration, instead of
silently reusing a value left over from a previous input.
Clearing in `~FuzzedSock()` would be wrong: several `FuzzedSock`s can be alive simultaneously (e.g. `process_messages` adds 1–3 peers), so clearing in one destructor would corrupt the mock observed by the others. Resetting at the iteration boundary keeps it decoupled from socket lifetimes.
### Testing
Verified with the global-state-detector approach from #29018 (snapshotting/diffing the writable globals around each iteration):
- **Before:** a single empty input to `process_message` reports `g_mock_steady_time` changing `00 → 01` (`0` → `INITIAL_MOCK_TIME`).
- **After:** that report is gone; the only remaining diffs are the benign one-time initialization of `ConsumeTime`'s function-local statics.
`p2p_headers_presync` (uses `SteadyClockContext`) and `pcp_request_port_map` (uses `FuzzedSock`) still run to `succeeded` without aborting, confirming existing steady-clock readers are unaffected.
This leak is invisible to coverage-based checks such as `deterministic-fuzz-coverage`, because `g_mock_steady_time` is only consumed through coarse time comparisons (e.g. the 250 ms presync rate-limiter): a changed value doesn't change the executed branches, so only a memory-diffing detector can see it.
ACKs for top commit:
maflcko:
lgtm ACK 19b32a2e18
marcofleon:
Nice catch, ACK 19b32a2e18
Tree-SHA512: b875795addb2914eae489adc703438483f8e464b9a210bd5d76189f13266dae5843c8749590d59e78bf171f19aa7cee21ca678cd311843d8a88cbe9831f20b6a
54de023a7c guix: add setup.sh (fanquake)
Pull request description:
This is the first change in #25573, which splits out the setup & tarball generation code from `build.sh`, so that it can be re-used, from multiple (future) build scripts.
ACKs for top commit:
willcl-ark:
ACK 54de023a7c
hebasto:
ACK 54de023a7c.
Tree-SHA512: 9a7f2fe322d281b9867414511af5243f4dd659ea42637f4eb8cc0c8629c94dab842669bb7c503f9fa67cab3fac65561364f07b5c0fda8e6d8c24e7bf161025ef
35a814a045 test: Limit clocks to one active instance (MarcoFalke)
55e402ffef scripted-diff: Rename NodeClockContext to FakeNodeClock (seduless)
1e9546fcf4 test: Use NodeClockContext in more call sites (seduless)
758fea59a8 test: Drop ++ from NodeClockContext default constructor (seduless)
7c2ec3949a test: Enter mocktime before peer creation in block_relay_only_eviction (seduless)
Pull request description:
Follow-up to #34858
Updates remaining `SetMockTime` call sites that are clean, mechanical swaps fitting the spirit of the original PR (see: https://github.com/bitcoin/bitcoin/pull/34858#pullrequestreview-4031647119 and https://github.com/bitcoin/bitcoin/pull/34858#issuecomment-4221757881). Further updates to `SetMockTime` are more complex and deserve separate, isolated PRs.
The default constructor for `NodeClockContext` increments to the next tick, which is a defensive measure to prevent time going backwards on construction. This has caused some confusion (see thread: https://github.com/bitcoin/bitcoin/pull/34858#discussion_r3057648646) and can be safely removed after updating the only test where this is load-bearing (b3c9bd7f2df230525c8e339394a315a2c500055d) (see: https://github.com/bitcoin/bitcoin/pull/34858#discussion_r3091085328). The removal also tightens the `addrman_tests/addrman_evictionworks` test to sit exactly on the `ADDRMAN_REPLACEMENT` boundary (4h), catching mutations such as:
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index d3dae59ae7..d0929c62cb 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -920,3 +920,3 @@ void AddrManImpl::ResolveCollisions_()
// Has successfully connected in last X hours
- if (current_time - info_old.m_last_success < ADDRMAN_REPLACEMENT) {
+ if (current_time - info_old.m_last_success <= ADDRMAN_REPLACEMENT) {
erase_collision = true;
```
The last follow-up item is updating `NodeClockContext` to `FakeNodeClock` to make it clear it is intended for testing (motivated by https://github.com/bitcoin/bitcoin/pull/34858#pullrequestreview-4082110904 and supported in https://github.com/bitcoin/bitcoin/pull/34858#issuecomment-4214352770).
ACKs for top commit:
maflcko:
re-ACK 35a814a045🛒
sedited:
ACK 35a814a045
Tree-SHA512: ade776e288a4b7bbc4c8855c14d61381b5b20329fe1e72fee87f773e47a9519975d58c277fbacda37dd73c0c1d4ce358c92dcdc4ca049d58cb3453ddf751b45b
fa2afba28b p2p: Release m_peer_mutex early in InitiateTxBroadcastToAll (MarcoFalke)
Pull request description:
The `InitiateTxBroadcastToAll` method holds the `m_peer_mutex` while updating the bloom filters for all peers. This is perfectly fine, because updating the bloom filters is fast. Though, from a style-perspective, the lock does not need to be held for the whole function. Also, holding the lock longer, may confuse Tsan into a lock-order inversion false-positive (ref: https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514926359).
So "fix" both issues in this style-refactor.
ACKs for top commit:
xyzconstant:
Code review ACK fa2afba28b
shuv-amp:
ACK fa2afba28b
danielabrozzoni:
Code Review ACK fa2afba28b
sedited:
ACK fa2afba28b
Tree-SHA512: c47849a4c3cc11c74b61fec3425db8ec7f78db4ca43d7bf3145ce640f7b0872701c09495f0dfe77109d09d5716d920ad3d7308483fe41564c30867b3e80432e7
fba713a28c scripted-diff: Rename UNIQUE_NAME to BITCOIN_UNIQUE_NAME (Hennadii Stepanov)
Pull request description:
https://github.com/bitcoin/bitcoin/pull/34454#issuecomment-3822800049:
> ... it is annoying that we keep running into the same bug over and over again (IIRC it happened in the past at least once for Bitcoin Core). Surely this is going to happen again in the future.
And here we go again.
---
The `nb30.h` Windows header [defines](b536c4fdb0/mingw-w64-headers/include/nb30.h (L78)) `UNIQUE_NAME` as a macro.
This introduces a fragile dependency on header inclusion order: if Windows headers happen to be included before `UNIQUE_NAME` is used, the preprocessor expands it into a numeric literal, causing syntax errors.
Rename the macro to `BITCOIN_UNIQUE_NAME` to remove this fragility and avoid the collision entirely.
---
Noticed while doing a Guix build of the [QML repo](https://github.com/bitcoin-core/gui-qml) for Windows.
Recent similar PRs: https://github.com/bitcoin/bitcoin/pull/34454 and https://github.com/bitcoin/bitcoin/pull/34868.
ACKs for top commit:
maflcko:
lgtm ACK fba713a28c
sedited:
ACK fba713a28c
w0xlt:
ACK fba713a28c
Tree-SHA512: 7a63b99a754e797eb8fa5d6a598606150f47ae1130d1d26067c509830e6575f0378ce63fe0ca35c69dce9a394451a34ddadd8b3d5f6f9a7e4c529108af546fb6
5b65e31270 test: remove two unnecessary nodes from the test (rkrux)
Pull request description:
A discussion in the review of #35443 PR brought this test to my attention.
The test needs multiple wallets that can be created on a single node, multiple nodes are not required.
As there is a cost associated with setting-up and tearing-down nodes, this patch helps in reducing the test time as well.
ACKs for top commit:
ekzyis:
ACK 5b65e31270
polespinasa:
lgtm ACK 5b65e31270
sedited:
ACK 5b65e31270
Tree-SHA512: f6b4a96b9beee968ef5438fd9db582a48834ff36ba27c19dd7012902528fa713424212530e34cc16b58c19c023f1accd2b89fe846ef2cc36677c24e160c5b817
d0b76c7f3e rpc+bitcoin-tx: Specify correct type for ParseFixedPoint() (Hodlinator)
43ca54ca00 refactor(test): Make CAmount arg explicit for BuildCreditingTransaction() (Hodlinator)
b5e91e946c wallet: Remove CoinsResult::Clear() (Hodlinator)
Pull request description:
The *knapsack_solver_test* in *coinselector_tests.cpp* was accumulating satoshi amounts beyond 21M BTC. This was uncovered while experimenting with adding checks to `CAmount`. Fix that by fully resetting the `CoinsResult` object accumulating those amounts, inspired by https://github.com/bitcoin/bitcoin/issues/35449#issuecomment-4613968627.
Also, while we're at it, add 2 commits which correct some `int64_t`/`CAmount` confusion.
Fixes https://github.com/bitcoin/bitcoin/issues/35449
ACKs for top commit:
sedited:
ACK d0b76c7f3e
furszy:
utACK d0b76c7f3e
brunoerg:
code review ACK d0b76c7f3e
Tree-SHA512: 6d989ded6f6327dc657f437dc256d4adf42a34a1252621421ee38d7851c6cdc97a462f033a4728e3aa7d5514deee4db6e83646105633f9cf7ed6e7e90406b67d
0bfc5e4fff add release notes (Pol Espinasa)
fdc9fc1df2 test: check getprivatebroadcast and abortprivatebroadcast throw if the node is running without -privatebroadcast set (Pol Espinasa)
7b821ef9b7 rpc: getprivatebroadcastinfo and abortprivatebroadcast throw if -privatebroadcast is disabled (Pol Espinasa)
Pull request description:
Makes `getprivatebroadcast` and `abortprivatebroadcast` throw if `-privatebroadcast=0`.
This is motivated by: https://github.com/sparrowwallet/sparrow/issues/1989
Knowing if `privatebroadcast` is set can be useful for some external software like Sparrow to avoid call `getprivatebroadcastinfo` each time to see if broadcast was done through that.
ACKs for top commit:
stickies-v:
ACK 0bfc5e4fff
sedited:
ACK 0bfc5e4fff
rkrux:
code review ACK 0bfc5e4fff
andrewtoth:
ACK 0bfc5e4fff
Tree-SHA512: 3bdb3909e93fc3835d801e1efc2bbec673a75a1ff089debd59e8970a0ff2b44d4e00b7ac26f10c972dcb50bf042521921370e1ec57885d67cd8459b3831da898
The `nb30.h` Windows header defines `UNIQUE_NAME` as a macro.
This introduces a fragile dependency on header inclusion order: if
Windows headers happen to be included before `UNIQUE_NAME` is used, the
preprocessor expands it into a numeric literal, causing syntax errors.
Rename the macro to `BITCOIN_UNIQUE_NAME` to remove this fragility and
avoid the collision entirely.
-BEGIN VERIFY SCRIPT-
sed -i 's/\<UNIQUE_NAME\>/BITCOIN_UNIQUE_NAME/g' $(git grep -l 'UNIQUE_NAME' ./src/)
-END VERIFY SCRIPT-
ec6cf49b91 blockstorage: Remove cs_LastBlockFile recursive mutex (sedited)
Pull request description:
The `cs_LastBlockFile` mutex is redundant: all critical sections are already covered by cs_main. This is demonstrated in this patch by replacing all instances of locking `cs_LastBlockFile` with pairs of `AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)` annotations. No additional `::cs_main` LOCK(...)s are introduced (besides for test-only code).
It is also not clear for which sections `cs_LastBlockFile` is responsible for. It is annotated for `m_blockfile_cursors`, but sporadically and inconsistently also covers `m_blockfile_info` (e.g. in `LoadBlockIndexDB`).
Since it has no semantic meaning, and seems confusing to developers, remove it.
An alternative to this patch would be expanding the scope of what `cs_LastBlockFile` covers and turning it into a non-recursive mutex. I prepared such a patch some time ago, but found it unsatisfactory. It was not clear to me if the lock was now covering too much or too little, and its purpose remained unclear. If this patch is accepted, I would expect the project to eventually implement a separate, narrowly-scoped block storage lock to allow for a more parallelizable block processing routine.
ACKs for top commit:
stickies-v:
re-ACK ec6cf49b91
janb84:
re- ACK ec6cf49b91
pablomartin4btc:
ACK ec6cf49b91
Tree-SHA512: e5942bc87300b0db9a0b91d5fe26dab455049e6cef7c96bb12b28141fa04711d46c6af105c0e1a83a9f261edde2c8b8b43ecf577a27d54b4610d784676a85627
The cs_LastBlockFile mutex is redundant: all critical sections are
already covered by cs_main. This is demonstrated in this patch by
replacing all instances of locking cs_LastBlockFile with pairs of
`AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)`
annotations. No additional `::cs_main` LOCK(...)s are introduced.
It is also not clear for which sections `cs_LastBlockFile` is
responsible for. It is annotated for `m_blockfile_cursors`, but
sporadically and inconsistently also covers `m_blockfile_info`.
Since it has no semantic meaning, and seems confusing to developers,
remove it.
SteadyClockContext and FakeNodeClock assume they are the only active
instance. Overlapping them in the same scope would silently clobber
each other.
Add a CRTP base class, LimitOne, that asserts at construction if
another instance already exists.
The previous name did not indicate the type was intended for
testing. Renaming to FakeNodeClock makes this explicit and
allows call sites to drop the ctx suffix on the variable name.
Suggested in #34858 review feedback.
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" -- src | xargs sed -i "s/$1/$2/g"; }
s '\<NodeClockContext\>' 'FakeNodeClock'
s '\<clock_ctx\>' 'clock'
-END VERIFY SCRIPT-
This refactor is a follow-up to commit
faad08e59c and does not
change any behavior.
These call sites are clean mechanical swaps. The remaining ones
require non-trivial test refactoring and are left for future
follow-ups.
The increment was originally added so that mocked time would not appear
to go backward relative to real-clock timestamps captured before
construction, since Now<NodeSeconds>() rounds the current time down to
a whole second. In practice the tests do not mix real and mocked
timestamps in a way that exposes this, so the increment is unnecessary.
This is a follow-up to commit faad08e59c.
Hoisting the NodeClockContext above peer creation ensures m_connected is
captured under mocktime, making the MINIMUM_CONNECT_TIME check
deterministic regardless of which peer is selected for eviction.
This is a prerequisite for the next commit, which removes the
one-second advance from the NodeClockContext default constructor.
5deb053a75 fuzz: fix dead HD keypaths (de)serialization round-trip (Sebastian Falbesoner)
Pull request description:
`DeserializeHDKeypaths()` was writing into the original `hd_keypaths` map instead of `deserialized_hd_keypaths`. As a result the latter was always empty and the round-trip assertion following was trivially true, so the serialize/deserialize round-trip wasn't actually being exercised.
That bug was introduced with the commit introducing the fuzz target (commit f898ef65c9, #18994).
ACKs for top commit:
sedited:
ACK 5deb053a75
Tree-SHA512: 0d8770aa5da2e132caedd522c8c95c4ceb6d1bcc4d5b6605784fd7d2df41fce29fcd25fc2741c4e751b942b548888833eb9d9d6318505a5c59f7b1f105c990ae
`DeserializeHDKeypaths()` was writing into the original `hd_keypaths`
map instead of `deserialized_hd_keypaths`. As a result the latter was
always empty and the round-trip assertion following was trivially true,
so the serialize/deserialize round-trip wasn't actually being exercised.
That bug was introduced with the commit introducing the fuzz target
(commit f898ef65c9, #18994).
CheckGlobalsImpl's constructor runs at the start of every fuzz iteration
and already resets the global RNG flags and the mockable NodeClock via
SetMockTime(0s), but it never reset the mockable steady clock. A value
written to g_mock_steady_time by one input therefore leaked into the
next one. For example, FuzzedSock's constructor calls
SetMockTime(INITIAL_MOCK_TIME) and never clears it, so the mocked steady
time stays set for all subsequent iterations.
Reset MockableSteadyClock symmetrically with NodeClock so each input
starts from an unmocked steady clock. This also brings the steady clock
under the same discipline as the system clock: a target that reads
MockableSteadyClock::now() without first mocking it is now caught by the
existing g_used_system_time check instead of silently reusing a leaked
value.
b2fbd5b5dd ci: run ipc functional tests in arm job (fanquake)
Pull request description:
These are currently skipped, because `pycapnp` isn't installed (https://github.com/bitcoin/bitcoin/actions/runs/26943765833/job/79499532298#step:10:4286):
```bash
interface_ipc.py | ○ Skipped | 0 s
interface_ipc_mining.py | ○ Skipped | 0 s
```
They seem to work fine locally. Not sure if this was missed, or on purpose.
ACKs for top commit:
Sjors:
ACK b2fbd5b5dd
sedited:
ACK b2fbd5b5dd
Tree-SHA512: d9ec06c0d65447102c3354ccddf5c03505e6338a08efd43f6ef495fafba3a6d9bf8c9d8f8e2a29f16931bcc5058911597a08aa938fb40bd9beab8b501c5194ef
The test needs multiple wallets that can be created on a single node, multiple
nodes are not required.
As there is a cost associated with setting-up and tearing-down nodes, this patch
helps in reducing the test time as well.
bf0d257c11 net: un-default the OpenNetworkConnection()'s proxy_override argument (Eugene Siegel)
5a3756d150 test: add a regression test for private broadcast v1 retries (Vasil Dimov)
ab35a028ed test: make reusable filling of a node's addrman (Vasil Dimov)
2333be9cbc test: make reusable starting a standalone P2P listener (Vasil Dimov)
2ffa81fac4 test: make reusable SOCKS5 server starting (Vasil Dimov)
32d072a49f doc: add release notes for #35319 (Vasil Dimov)
d01b461f71 net: ensure no direct private broadcast connections (Vasil Dimov)
fd230f942d net: use the proxy if overriden when doing v2->v1 reconnections (Vasil Dimov)
Pull request description:
This PR includes https://github.com/bitcoin/bitcoin/pull/35319 and on top of that adds a regression functional test.
The functional test exercises the relevant code paths without modifying non-test code. To do that it does:
* Add a bunch of IPv4 addresses to the node's addrman (they will be added without P2P_V2 flag).
* Get them to report P2P_V2 in their service flags and connect to each one, so that the flags
in addrman are updated to contain P2P_V2.
* Get one successful connection to a Tor peer (.onion) so that bitcoind assumes the configured
Tor proxy works and is indeed a proxy to the Tor network. This will make it open private
broadcast connections also to IPv4 addresses via that proxy.
* Start some private broadcast connections.
* Remember the destination IPv4 address of the first connection and get it to fail the v2
transport.
* Wait for a subsequent connection also through the Tor proxy to the same IPv4 and expect
it to be v1, i.e. the v2->v1 downgrade retry.
The test fails without the fix - the v1 retry never arrives to the Tor proxy. And passes with the fix. The fix is in the first commit here and in https://github.com/bitcoin/bitcoin/pull/35319, can remove it by `git show fd230f942d | git apply -R`.
ACKs for top commit:
Crypt-iQ:
reACK bf0d257c11
andrewtoth:
ACK bf0d257c11
instagibbs:
ACK bf0d257c11
sedited:
utACK bf0d257c11
Tree-SHA512: 11e89be36577199e0312e5e63efeac04e295faaba1cf1c13a30e683d35f473c8dbb419d1897b0333c2e993c10637adecafcf90fe08c812065c793cbc903744c9
7735c13488 test: run bitcoin-cli -ipcconnect check under valgrind with -datadir (Michael Dietz)
Pull request description:
This case invokes bitcoin-cli via raw subprocess.run() without -datadir, so it reads the default datadir's bitcoin.conf (e.g. ~/.bitcoin) and fails whenever that real config is unusable. Pass the node's -datadir so the check reads the test's own bitcoin.conf and depends only on the build's IPC support, not the host environment.
ACKs for top commit:
maflcko:
lgtm ACK 7735c13488
sedited:
ACK 7735c13488
Tree-SHA512: 5264433e2cd1747b9b9b4437b80f1849b5fa01620dd958c34c124925270dfca85c3809186285bf1dd70a8358552e1ebb589ec84093e11bb0d66cfe10f04dde81
4a6d1458b4 ci: add pyzmq to msan job (fanquake)
c21b58e263 ci: use pyzmq over zmq (fanquake)
Pull request description:
`zmq` seems to be an alias for `pyzmq`, and the project page, https://pypi.org/project/zmq/, states "You are probably looking for pyzmq.". So switch to `pyzmq`, which is what we document, and use in all other jobs.
Also add `pyzmq` to native MSAN, so that `interface_zmq.py` is run.
ACKs for top commit:
sedited:
ACK 4a6d1458b4
Tree-SHA512: 86b96f6b2dca032bf7460113173472c8384834b1f38d961dff1ee325cce75410cc02062da798484f12fb28f676611df7ea406ee8385f9324b298c036826881f9
82901981bf ci: use Warp cache for Docker layers (will)
Pull request description:
We switched to GH cache recently, but the performance is beyond terrible. We have already switched the main cache actions over to warp, and this completes the transfer with the docker buildkit cache to the Warp endpoint.
ACKs for top commit:
m3dwards:
ACK 82901981bf
Tree-SHA512: caee053598c5082b1f0cc52a6abfe6314ac2d5d840238480f1706f55a892f08ca533ac3d5d4f9f20a84958df1c9100ae00518686072515a1011be994619b4df9
7249b376a0 opt: Skip UTXOs with worse waste, same eff_value (Murch)
5204291860 opt: Skip evaluation of equivalent input sets (Murch)
ba1807b981 coinselection: Track effective_value lookahead (Murch)
fa226ab902 coinselection: BnB skip exploring high waste (Murch)
7ecea1dc5d coinselection: Track whether BnB completed (Murch)
3ca0f36164 coinselection: rewrite BnB in CoinGrinder-style (Murch)
2e73739837 coinselection: Track BnB iteration count in result (Murch)
Pull request description:
This PR rewrites the implementation of the BnB coinselection algorithm
to skip the duplicate evaluation of previously visited input selections.
In the original implementation of BnB, the state of the search is
backtracked by explicitly walking back to the omission branch and then
testing again. This retests an equivalent candidate set as before, e.g.,
after backtracking from {ABC}, it would evaluate {AB_}, before trying
{AB_D}, but {AB_} is equivalent to {AB} which was tested before.
CoinGrinder tracks the state of the search instead by remembering which
UTXO was last added and explicitly shifting from that UTXO directly to
the next, so after {ABC}, it will immediately move on to {AB_D}. We
replicate this approach here.
As fewer nodes are visited, this approach will enumerate more possible
combinations than the original implementation given the same limit for
iterations.
ACKs for top commit:
achow101:
ACK 7249b376a0
w0xlt:
reACK 7249b376a0
Tree-SHA512: fd5851ceea3a3a4699fc062254fa5438daa4275b4d52325983e63670040cf0ba35112be9e63813d8f30b38993c031f3df343b2152eb8c068d272fbff72d1881a
801e3bfe38 chainparams: add overloads for RegTest and SigNet with no options (Antoine Poinsot)
4995c00a9c chainparams: make deployment configuration available on all test networks (Antoine Poinsot)
df7ed5f355 chainparams: encapsulate deployment configuration logic (Antoine Poinsot)
Pull request description:
It's sometimes useful to test a deployment on other networks than regtest. This may be e.g. because regtest lacks a property relevant for the test, or simply because the test aims to be portable while regtest is Bitcoin Core specific.
This PR makes it possible to set the `-vbparams` and `-testactivationheight` options on any network in unit tests, and on any **test** network as a startup option.
This is preparatory work for a BIP 54 implementation, but may be useful separately.
ACKs for top commit:
edilmedeiros:
utACK 801e3bfe38
achow101:
ACK 801e3bfe38
sedited:
ACK 801e3bfe38
instagibbs:
ACK 801e3bfe38
Tree-SHA512: 10649dc9bbc70a830bb0c4b1c965de5bf2e6be1a6c2832bdf11e9248dacb4a1f60421b410d0284213e88de6c54218252ae5de423b4c6e659d10bab1cbe7e7e87
107d4178d9 versionbits: update VersionBitsCache doc comment to match current behaviour (Antoine Poinsot)
94e3ac0b21 doc: release notes and bips doc update for #34779 (Antoine Poinsot)
1d5240574a qa: test we don't warn for ignored unknown version bits deployments (Antoine Poinsot)
f802edf57c versionbits: Limit live activation params and activation warnings per BIP323 (Anthony Towns)
Pull request description:
This implements https://github.com/bitcoin/bips/pull/2116, which repurposes 24 version bits as extra nonce space for miners rather than soft fork deployment coordination. 24 bits allows a miner to perform up to 72 PH before needing a fresh job from its controller. The current 16 bits in use by miners only allow up to 280 TH, which [apparently led some ASIC designers to start rolling the timestamp field](https://github.com/bitaxeorg/ESP-Miner/pull/1553#issuecomment-3937736319) on their beefier machines.
Mailing list discussion available [here](https://gnusha.org/pi/bitcoindev/6fa0cb45-37d6-4b41-9ff8-03730fd96d6e@mattcorallo.com/). A previous shot at this is https://github.com/bitcoin/bitcoin/pull/13972 (with a smaller extranonce space).
This change only affects the warning logic.
ACKs for top commit:
ajtowns:
ACK 107d4178d9
achow101:
ACK 107d4178d9
sedited:
Re-ACK 107d4178d9
optout21:
ACK 107d4178d9
Tree-SHA512: cfaf5d7de1e8c020a4d7f4b1096b6c3e0e3b41ea840a4652ebcdabc345c5c557161c8304f1d7d6de541a2bf1df3c855ad7b64e49dd8c8af3937876d134bb5aba
Signed overflow on nScore updates is undefined behavior. Use
SaturatingAdd in AddLocal() and SeenLocal() so increments saturate at
INT_MAX instead of overflowing.
Add unit test coverage for saturation in both code paths.
Speeds of 1MB/s and 15 minute cached docker image pulls during builds
are not uncommon.
Warp runners provide a local GitHub Actions cache protocol proxy for
Docker layer cache traffic. Point BuildKit's gha cache backend at that
proxy on Warp runners so cached image layers do not have to be fetched
from GitHub's slower cache service.
Add a default for provider so other users (e.g. qa-assets) don't have to
update this unless they use custome runners.
55d37546fa Bugfix: GUI/Intro: Handle errors from SelectParams the same as if during InitConfig (Luke Dashjr)
Pull request description:
Without this, invalid vbparams just silently exit with no message
ACKs for top commit:
sedited:
ACK 55d37546fa
hebasto:
ACK 55d37546fa, tested on Fedora 44.
Tree-SHA512: 0508ca64c86a651b9b21ae2a1e26dfb84c0dbb0b20d309da499545be2733d42c98012b84d81177ba0635d0d8bce87c888b256a014e2065bd4a80db88e73ec3d4
zmq seems to be an alias for pyzmq, and the project page,
https://pypi.org/project/zmq/, states "You are probably looking for
pyzmq.". So switch to pyzmq, which is what we document, and use in all
other jobs.
8cb8653a22 fuzz: target concurrent leveldb reads (Andrew Toth)
6609088fe6 fuzz: extract ConsumeDBParams helper (Andrew Toth)
Pull request description:
Inspired by https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-4054461591.
We currently do concurrent leveldb reads when accessing our indexes.
1. `txindex` - we call `FindTx()` from multiple RPC threads.
2. `blockfilterindex` - we call `LookupFilter/Header()` concurrently from `msghand` thread for p2p requests as well as RPC threads.
3. `coinstatsindex` - we call `LookUpStats()` from multiple RPC threads.
4. `txospenderindex` - we call `FindSpender()` from multiple RPC threads.
We also read from our chainstate and blocks index while background compactions are writing.
While OSS-Fuzz does cover leveldb (https://github.com/google/oss-fuzz/blob/master/projects/leveldb/fuzz_db.cc), it doesn't cover multi threaded access. Without a deterministic hypervisor this fuzz harness won't be deterministic, but we can at least run it with TSan to get a higher confidence that the synchronization code in leveldb is correct. Hopefully other reviewers find this useful.
This harness creates a threadpool with 16 threads, and then creates an in-memory levelDB which it seeds with deterministically random values. It chooses a random set of keys to query. It first performs all queries on the db on a single thread to get a baseline, then synchronizes all threads on a latch so they hit the db at the same time. Each thread performs the same queries, and afterwards are all checked against the baseline.
It uses a `DeterministicEnv` to capture background compaction work when seeding the db, which is also run immediately after the latch is released. This causes a race between compaction and reading, ensuring we exercise many thread synchronization code paths in leveldb.
I ran both TSan and ASan/UBSan overnight with no issues.
ACKs for top commit:
fjahr:
Code review ACK 8cb8653a22
l0rinc:
ACK 8cb8653a22
Tree-SHA512: 2ca31a824715b92e258c84ecf0c762f43ee2a528e3a3192f94d8aaeddf6e99f820a0297ce9efcc95bc32c7ec74489f240a25bab856d724d768117a7d95a33974
2ef6679c2c test: Check that MuSig2 signing does not reuse nonces (Ava Chow)
bb05986c0a musig: Include pubnonce in session id (Ava Chow)
Pull request description:
It is safe to have multiple musig signing sessions over the same message so long as the nonces used are different. Including the pubnonce in the session id allows for multiple simultaneous signing sessions over the same message, rather than asserting when the user tries to do this.
The second commit tests this behavior, both ensuring that there is no crash, and verifying that both sessions produce unique nonces and signatures to verify that no reuse is occurring.
Lastly, the assertion in `SetMuSig2SecNonce` is retained as hitting it now would indicate that a nonce has been reused. We prefer to assert and crash rather than do something that is highly likely to leak a private key.
Fixes#35250
ACKs for top commit:
rkrux:
lgtm ACK 2ef6679c2c
junbyjun1238:
utACK 2ef6679c2c
theStack:
ACK 2ef6679c2c
Tree-SHA512: 9fb60b68ebe0ea9656408afb65b9ec9f280632e1bb84a4821b074c8d8569847845f7c29da800c757b9ddf3aa31aa890dd9e3646cf119917a714e7daf20be2198
