e76e886581 guix: doc: zdiff3 doesn't work (David Gumberg)
ea1be38677 guix: doc: Suggest guix-install.sh --uninstall (David Gumberg)
Pull request description:
Add a note about guix issues with git `merge.conflictstyle` being set to `zdiff3`, and add information about how to uninstall for users of `guix-install.sh`
ACKs for top commit:
achow101:
ACK e76e886581
hebasto:
ACK e76e886581.
sedited:
utACK e76e886581
Tree-SHA512: e9edc49851579c52e7ac762221d0bb51c928da4dff5566ece766af7f7eb75466831a76f885ccc5a26207c84b846e3f42bae26e80ddece3df1611de2c2320c28a
d8f4e7caf0 doc: add release notes (ismaelsadeeq)
248c175e3d test: ensure `ValidateInputsStandardness` optionally returns debug string (ismaelsadeeq)
d2716e9e5b policy: update `AreInputsStandard` to return error string (ismaelsadeeq)
Pull request description:
This PR is another attempt at #13525.
Transactions that fail `PreChecks` Validation due to non-standard inputs now returns invalid validation state`TxValidationResult::TX_INPUTS_NOT_STANDARD` along with a debug error message.
Previously, the debug error message for non-standard inputs do not specify why the inputs were considered non-standard.
Instead, the same error string, `bad-txns-nonstandard-inputs`, used for all types of non-standard input scriptSigs.
This PR updates the `AreInputsStandard` to include the reason why inputs are non-standard in the debug message.
This improves the `Precheck` debug message to be more descriptive.
Furthermore, I have addressed all remaining comments from #13525 in this PR.
ACKs for top commit:
instagibbs:
ACK d8f4e7caf0
achow101:
ACK d8f4e7caf0
sedited:
Re-ACK d8f4e7caf0
Tree-SHA512: 19b1a73c68584522f863b9ee2c8d3a735348667f3628dc51e36be3ba59158509509fcc1ffc5683555112c09c8b14da3ad140bb879eac629b6f60b8313cfd8b91
fa4ec13b44 build: Enable -Wcovered-switch-default (MarcoFalke)
fa2670bd4b refactor: Enable -Wswitch in exhaustive switch (MarcoFalke)
Pull request description:
The compiler flag `-Wswitch` is enabled. However, it can not fire when a `default:` case exists. Fix that by removing the default case where a switch is already handling all cases exhaustively.
Also, enable `-Wcovered-switch-default` to catch those cases at compile time in the future.
Also, apply the comment according to the dev notes.
Can be reviewed via `--ignore-all-space`
ACKs for top commit:
stickies-v:
re-ACK fa4ec13b44, no changes except for addressing silent merge conflict from d339884f1d
l0rinc:
ACK fa4ec13b44
achow101:
ACK fa4ec13b44
sedited:
ACK fa4ec13b44
Tree-SHA512: 8dd9e71a8cd338255f43448a59a1a4d40a9fc16e19a707cc10fb71442d4df9f82a0e5fae77868ef49cd0ea27fdd972687572c1a50b6aba7e08c6ce87576afc6a
9f28120a5b kernel: Add API function for getting a tx input's nSequence (Sebastian Falbesoner)
6b64b181d5 kernel: Add API function for getting a tx's nLockTime (Sebastian Falbesoner)
Pull request description:
This PR introduces two new C API functions to libbitcoinkernel:
* `btck_transaction_get_locktime` to access a transaction's `nLockTime` value
* `btck_transaction_input_get_sequence` to access a transaction input's `nSequence` value
Inspired by https://bnoc.xyz/t/forward-compatible-coinbase-locktimes-for-bip-54. After reading this I thought it would be a nice/useful showcase to check BIP54 compliance of (historical) blocks using bitcoinkernel, without having to manually deserialize the transaction (this is just about one of the four BIP54 rules though, especially the sigops limit is much more involved).
ACKs for top commit:
sedited:
ACK 9f28120a5b
yuvicc:
ACK 9f28120a5b
stickies-v:
ACK 9f28120a5b
Tree-SHA512: 9eae795d6e4b9b367bbfe2665b916121ef64031e8d10667c71741344b5eea4c2562862a937bdf1363cc66b67bb5d48392c9f44e52f0d92d2a5a65e10d061b703
d6f680b427 validation: Move block into BlockDisconnected signal (sedited)
4d02d2b316 validation: Move block into BlockConnected signal (sedited)
8b0fb64c02 validation: Move validation signal events to task runner (sedited)
Pull request description:
This enforces behaviour that is currently already implicit: The destructor for blocks runs mostly in the [scheduler thread](https://bitcoin-dev-tools.github.io/benchcoin/results/pr-176/20472174834/mainnet-default-instrumented-head-flamegraph.svg?x=2762391536960&y=684). The change should make it a bit clearer what the ownership semantics for these validation signals are.
`BlockConnected` already takes a reference to a block that is emplaced in `connected_blocks`. Once `connected_blocks` is iterated through, it is not reused. Similarly `BlockDisconnected` currently takes a reference to a block that is discarded after the call to it. Note that this does not give the guarantee that blocks' lifetimes are extended by other means once they are connected. For example after IBD, the block's lifetime is extended in net_processing's `m_most_recent_block` and `ActivateBestChain` itself takes a copy of the block's shared pointer, meaning its caller may delay de-allocation.
ACKs for top commit:
maflcko:
re-review ACK d6f680b427🔌
stickies-v:
re-ACK d6f680b427
frankomosh:
Re-ACK d6f680b427
Tree-SHA512: 9209a7d23e7af0d76fa70dff958b1329f38ef29ccc49b5a32bcf9f349d59cc2bf70464ebdb130d26077c0ff9362ce9211472231d375ff1c9c89c0ec3020eac80
779e7825db fuzz: wallet: add target for `MigrateToDescriptor` (brunoerg)
Pull request description:
This PR adds fuzz coverage for the scriptpubkeyman migration (`MigrateToDescriptor`). Note that it's a test for the migration of the scriptpubkey manager, not for the whole migration process as tried in #29694, because:
1) The wallet migration deals with DBs which is expensive for fuzzing (was getting around 3 exec/s);
2) Mocking would require lots of refactors.
This target loads keys, HDChain (even inactive ones), watch only and might add tons of different scripts, then calls `MigrateToDescriptor`. It does not play with encrypted stuff because it would make the target super slow. Also, after the migration there are some assertions that would work as a regression test for https://github.com/bitcoin/bitcoin/pull/31452, for example.
ACKs for top commit:
frankomosh:
Code Review ACK 779e7825db
marcofleon:
reACK 779e7825db
Tree-SHA512: 08ef5166602c21658765bc063c5421e81055d094d346c4e2a28215209c6b7768b99a424f3ba47cf718dc8d827a588da22394ba23402a40a71a976d80d65e6c2e
79467e3ec7 threading: never require logging from sync.h (Cory Fields)
Pull request description:
This is an updated version of #34793 after stickies-v [pointed out that the approach there was broken](https://github.com/bitcoin/bitcoin/pull/34793#discussion_r2916369196).
sync.h is low-level and should not require any other subsystems.
Move the lone remaining logging call to the .cpp. Any cost incurred by an additional function call should be trivial compared to the logging itself.
ACKs for top commit:
stickies-v:
re-ACK 79467e3ec7 , thanks for doing the modernization right away 👍
sedited:
ACK 79467e3ec7
Tree-SHA512: 53e4b19cbf6ec81ce6aee06092a07e56877e2247a2614a148ba25c276dc5cf00d68445f9cd43dbd71e4481da74b788e3e20d3634484760d037f94d44bf13ea9c
f3bf63ec4f kernel: acquire coinstats cursor and block info atomically (w0xlt)
5e77072fa6 rpc: fix race condition in gettxoutsetinfo (w0xlt)
Pull request description:
Fixes#34263
A `CHECK_NONFATAL` assertion failure could occur in `gettxoutsetinfo` when a new block was connected between capturing `pindex` and validating it in `GetUTXOStats()`.
The fix removes the early `pindex` capture since `ComputeUTXOStats()` independently fetches the current best block under lock. The response now uses `stats.hashBlock` and `stats.nHeight` (the actual computed values) instead of the potentially stale `pindex`.
ACKs for top commit:
sedited:
ACK f3bf63ec4f
fjahr:
utACK f3bf63ec4f
rkrux:
Concept ACK f3bf63ec4f for removal of the race condition.
Tree-SHA512: c2d5cd5a1b4b4f1c22023c03970fea400a0b78005fa3d09d6567255615ab461c01b584d8a158651ee08769ec86fc4a1200f640ad58fdaa4879c335d90c891f6a
498b6eb6b5 cmake: Migrate away from deprecated SQLite3 target (Daniel Pfeifer)
Pull request description:
CMake version 4.3 deprecated the imported target `Sqlite::Sqlite3`.
Use the preferred name `Sqlite3::Sqlite3` instead and provide an alias for older versions of CMake.
Also define the same alias when using vcpkg.
ACKs for top commit:
hebasto:
ACK 498b6eb6b5, tested on Fedora 43 using CMake 3.22.6 and 4.3.0.
Tree-SHA512: 8ec0e9673ea39c4383d2995d804498675f9bcd1196ec586f338cb3fa63aa632f160dd179bda3c2d70d932d8b1ac821c15d8247e7302465420937c9227f1bf20a
faf71d6cb4 test: [refactor] Use verbosity=0 named arg (MarcoFalke)
99996f6c06 test: Fix intermittent issue in feature_assumeutxo.py (MarcoFalke)
Pull request description:
The test has many issues:
* It fails intermittently, due to the use of `-stopatheight` (https://github.com/bitcoin/bitcoin/issues/33635)
* Using `-stopatheight` is expensive, because it requires two restarts, making the test slow
Fix all issues by using `dumb_sync_blocks` and avoid the two restarts.
Fixes https://github.com/bitcoin/bitcoin/issues/33635
Diff to test:
```diff
diff --git a/src/util/tokenpipe.cpp b/src/util/tokenpipe.cpp
index c982fa6fc4..1dc12ea1d5 100644
--- a/src/util/tokenpipe.cpp
+++ b/src/util/tokenpipe.cpp
@@ -4,2 +4,3 @@
#include <util/tokenpipe.h>
+#include <util/time.h>
@@ -60,2 +61,3 @@ int TokenPipeEnd::TokenRead()
ssize_t result = read(m_fd, &token, 1);
+ UninterruptibleSleep(999ms);
if (result < 0) {
```
On master: Test fails
On this pull: Test passes
ACKs for top commit:
fjahr:
Code review ACK faf71d6cb4
sedited:
ACK faf71d6cb4
Tree-SHA512: 8f7705d2b3f17881134d6e696207fe6710d7c4766f1b74edf5a40b4a6eb5e0f4b12be1adfabe56934d27abc46e789437526bcdd26e4f8172f41a11bc6bed8605
fadf901fd4 rpc: Run type check on decodepsbt result (MarcoFalke)
fa4d5891b9 refactor: Introduce TxDocOptions (MarcoFalke)
fa8250e961 refactor: Add and use RPCResultOptions (MarcoFalke)
Pull request description:
For RPCResults, the type may be ELISION, which is confusing and brittle:
* The elision should only affect the help output, not the type.
* The type should be the real type, so that type checks can be run on it.
Fix this issue by introducing a new print_elision option and using it in `decodepsbt`.
This change will ensure that `RPCResult::MatchesType` is properly run.
Can be tested by introducing a bug:
```diff
diff --git a/src/core_io.cpp b/src/core_io.cpp
index 7492e9ca50..4927b70c8e 100644
--- a/src/core_io.cpp
+++ b/src/core_io.cpp
@@ -436,2 +436,3 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
entry.pushKV("version", tx.version);
+ entry.pushKV("bug", "error!");
entry.pushKV("size", tx.ComputeTotalSize());
```
And then running (in a debug build) `decodepsbt cHNidP8BAAoCAAAAAAAAAAAAAA==`
Before, on master: passes
Now, on this pull: Properly detects the bug
ACKs for top commit:
nervana21:
tACK fadf901fd4
achow101:
ACK fadf901fd4
willcl-ark:
ACK fadf901fd4
satsfy:
re-ACK fadf901fd4
seduless:
re-ACK fadf901fd4
Tree-SHA512: 4fb000dba9fe39bcd2bac72e2d88553f54134a250c985b4ca7150b483d7185009047d8fe4ba75c522bfc26706de20c913b8905e7552ab0c41802ae744cb92038
fa050da980 test: Move event loop creation to network thread (MarcoFalke)
fa9168ffcd test: Use asyncio.SelectorEventLoop() over deprecated asyncio.WindowsSelectorEventLoopPolicy() (MarcoFalke)
Pull request description:
It is deprecated according to https://docs.python.org/3.14/library/asyncio-policy.html#asyncio.WindowsSelectorEventLoopPolicy The replacement exists since python 3.7
Also, move the event loop creation to happen in the thread that runs the loop.
ACKs for top commit:
l0rinc:
ACK fa050da980
sedited:
ACK fa050da980
Tree-SHA512: dce25596a04e8f133630d84c03a770185a81b1bcd0aae975f0dbdd579d22b7b79a9b1172abf46c61d0845d3f5ab4a6414fa0f17c59f0ea0f6fa9bdcac085a2a7
faea12ecd9 test: Fixup docs for NodeClockContext and SteadyClockContext (MarcoFalke)
eeeeb2a0b9 fuzz: Use NodeClockContext (MarcoFalke)
fa4fae6227 test: Add NodeClockContext (MarcoFalke)
Pull request description:
Iterating over fuzz inputs will usually be done in the same process. As the mocktime is global, it can theoretically leak from one fuzz input run into the next run, making it less deterministic.
Fix this issue, by adding and using a context manager to handle the mocktime and reset it before the end.
This refactor should not change any behavior.
ACKs for top commit:
seduless:
re-ACK faea12ecd9
dergoegge:
utACK faea12ecd9
brunoerg:
code review ACK faea12ecd9
Tree-SHA512: e222c4e4217a504d058b30f1e975dfdfff019363c82385bd62f368b16fb029c46a5d1b43cd773dbdd9efcd7f968d46dbe2c75812971696b1b879b8f081fc6b1b
32debfa1ed fuzz: set whitelist permissions on connman target (Bruno Garcia)
Pull request description:
I noticed that `AddWhitelistPermissionFlags` was always being called with an empty `ranges` (whitelist permissions). This function is called when connecting to a node (`ConnectNode`), and if the connection is a manual one, it uses the `vWhitelistedRangeOutgoing` to populate the permissions. However, since we were not setting any whitelist permission on the target, this vector is always empty. This PR improves this target by populating both `vWhitelistedRangeIncoming` and `vWhitelistedRangeOutgoing`.
ACKs for top commit:
Crypt-iQ:
utACK 32debfa1ed
maflcko:
lgtm ACK 32debfa1ed
frankomosh:
crACK 32debfa1ed
Tree-SHA512: 400ce780b9ae41d849ccad04258da4ad92d9bf780bfdeb9bb9c1684722bcc02ae45f13081b809f9e16b28078bd4efb928c6c7e1c3da638307b6c11b193d6dc04
This makes existing behaviour of the block's destructor triggering on
the scheduler thread more explicit by moving it to the thread. The
scheduler thread doing so is useful, since it does not block the thread
doing validation while releasing a block's memory.
DisconnectTip already creates and destroys the block itself, so moving
it into the validation signals is well scoped.
This makes existing behaviour of the block's destructor triggering on
the scheduler thread more explicit by moving it to the thread. The
scheduler thread doing so is useful, since it does not block the thread
doing validation while releasing a block's memory.
Previously, both the caller and the queued event lambda held copies of
the shared_ptr. The block would typically be freed on the scheduler
thread - but only because it went out of scope before the queued event
on the scheduler thread ran. If the scheduler ran first, the block would
instead be freed on the validation thread.
Now, ownership is transferred at each step when invoking the
BlockConnected signal: connected_blocks yields via std::move,
BlockConnected takes by value, and the event lambda move-captures the
shared_ptr. Though it is possible that this only decrements the block's
reference count, blocks are also read from disk in `ConnectTip`, which
now explicitly results in their memory being released on the scheduler
thread.
Currently arguments passed through the validation interface are copied
three times. Once on capture in the event, again when the event itself
is copied into the task runner lambda, and when the various arguments
used by the logging statement are copied into the lambda.
This change avoids the variables captured by the event being copied
again. Next to avoiding needless copies, this is done in preparation of
the following two commits, which seek to clarify the ownership semantics
of the blocks passed through the validation interface.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
CMake version 4.3 deprecated the imported target `Sqlite::Sqlite3`.
Use the preferred name `Sqlite3::Sqlite3` instead and provide an
alias for older versions of CMake. Also define the same alias when
using vcpkg.
685a44c601 fuzz: set fSuccessfullyConnected in connman harness (frankomosh)
Pull request description:
The connman fuzz harness never sets `fSuccessfullyConnected=true` on nodes added through `AddTestNode()`. `NodeFullyConnected()` gates `ForEachNode()` on that flag, making its callback unreachable in the current harness.
Set `fSuccessfullyConnected=true` before `AddTestNode()` to simulate a node that has completed the version handshake.
ACKs for top commit:
maflcko:
lgtm ACK 685a44c601
sedited:
ACK 685a44c601
marcofleon:
ACK 685a44c601
Tree-SHA512: 2c696b8674cb465f468642b5fd37a467bc34dcbf61dc901d784fd2fe0dd13ced5cd6bd9873bcce0f8e60e25d450052e9a562ece08abeb2ab6472e584dba65c40
551875360c ci: Use arch-appropriate binaries in lint install (will)
Pull request description:
In testing #34547 it has been observed that the lint container does not run on aarch64-linux without `qemu binfmt` (or similar).
This is because some tools are hardcoded to download x64 linux binaries. This has meant the linter works fine on:
- x64 linux
- aarch64 MacOS (via Rosetta)
- platforms using qemu
But does not work on e.g. aarch64-linux _without qemu_.
`shellcheck`` offer many platforms: https://github.com/koalaman/shellcheck/releases/tag/v0.11.0 and `mlc` offers are least x64 and aarch64 linux https://github.com/becheran/mlc/releases/tag/v1.2.0.
Try to download the correct binary for the platform using `uname` detection. This should see the linter work on native aarch64 + amd64, whilst maintaining current (emulated) compatibility.
ACKs for top commit:
maflcko:
lgtm ACK 551875360c
Tree-SHA512: 636cccbed3ffff995549c666b0cad1aa9790291a73a0f2212f0374c8878bd916c04e4ecb17fac1611fc2d72d363cececeeaa997af918ad4225355231376ff7b0
Replace the hardcoded x86_64 binary name with $(uname --machine) so the
correct binary is downloaded when building the lint container, where at
all possible.
bde35d61f9 depends: capnp 1.4.0 (fanquake)
Pull request description:
Update capnp in depends to [`1.4.0`](https://github.com/capnproto/capnproto/releases/tag/v1.4.0).
It contains a number of bugfixes, and fixes for 2 CVEs, of which I think only `Fix benign(?) buffer overrun in async readMessage()` is relevant to us, and it seems to be considered benign:
> This is technically undefined behavior (a buffer overrun), but we suspect that it is benign with all known memory allocators. In C++, a zero-sized allocation (made with `operator new(0)`, as is the case here) is required to return a unique pointer, different from any other such allocation. Because of this, all common memory allocators round up a zero-byte allocation to a word-sized allocation (32-bit or 64-bit, depending on the architecture). The overrun written to this allocation was exactly one pointer in size, so always fits into the actual allocation space.
> Nevertheless, the code is in fact relying on undefined behavior, and it is theoretically possible that some memory allocator implements zero-sized allocations in a way that would make this overrun dangerous.
See https://github.com/capnproto/capnproto/compare/release-1.3.0...release-1.4.0 for all changes since 1.3.0.
ACKs for top commit:
sedited:
ACK bde35d61f9
janb84:
ACK bde35d61f9
hebasto:
ACK bde35d61f9.
Tree-SHA512: 33a6c12684b9a6046a38c3b9dd1a5730db352eae07b5dbfe7244228fde3d1627d039c0e0ba7d35fe0968f91a0f476c239fa8f2e356a37b8ac975ac268d271bc2
6202acd284 test: addrman: successive failures in the last week for IsTerrible (brunoerg)
f611d3bdaf refactor: addrman: move consts to .h (brunoerg)
Pull request description:
This PR adds test coverage for the case that an address is considered terrible if we had N successive failures in the last week.
It kills the following mutant (https://corecheck.dev/mutation/src/addrman.cpp#L88):
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index e3981e6a40..f8045491c1 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -65,7 +65,7 @@ bool AddrInfo::IsTerrible(NodeSeconds now) const
}
if (now - m_last_success > ADDRMAN_MIN_FAIL && nAttempts >= ADDRMAN_MAX_FAILURES) { // N successive failures in the last week
- return true;
+ return false;
}
return false;
```
ACKs for top commit:
frankomosh:
re-ACK 6202acd284
naiyoma:
tACK 6202acd284
danielabrozzoni:
tACK 6202acd284
sedited:
ACK 6202acd284
Tree-SHA512: b4736ef91b75ba4c060dc18a2b796aee94d0d8be5ca58b9b873156248cd0dc6910595595e0f56d75bffff4f94c035319ac8428f7d79f07fe685bdba27a188829
sync.h is low-level and should not require any other subsystems.
Move the lone remaining logging call to the .cpp. Any cost incurred by an
additional function call should be trivial compared to the logging itself.
Without this, NodeFullyConnected() filters out every fuzz-constructed node, making ForEachNode's callback unreachable (0/1.13M branch hits from my end).
For RPCResults, the type may be ELISION, which is confusing and brittle:
* The elision should only affect the help output, not the type.
* The type should be the real type, so that type checks can be run on
it.
Fix this issue by introducing a new print_elision option and using it
in decodepsbt.
This change will ensure that RPCResult::MatchesType is properly run.
Also, this clarifies the RPC output minimally:
```diff
--- a/decodepsbt
+++ b/decodepsbt
@@ -35,7 +35,7 @@ Result:
"inputs" : [ (json array)
{ (json object)
"non_witness_utxo" : { (json object, optional) Decoded network transaction for non-witness UTXOs
- ...
+ ... The layout is the same as the output of decoderawtransaction.
},
"witness_utxo" : { (json object, optional) Transaction output for witness UTXOs
"amount" : n, (numeric) The value in BTC
```
fa90b21430 test: Remove unused feature_segwit.py functions (MarcoFalke)
fa6b05c96f test: Remove unused CUSTOM_._COUNT (MarcoFalke)
fa7bac94d8 test: Remove unused wait_for_addr, firstAddrnServices, on_addr (MarcoFalke)
fa388a3585 test: Remove unused self.p2p_conn_index = 1 (MarcoFalke)
fa803710e2 test: Remove unused AddressType (MarcoFalke)
fab5072ce1 ci: Remove vulture (MarcoFalke)
Pull request description:
Currently, `vulture` is run with `--min-confidence=100`, which reduces its checks to dead code after control statements, which is nice, but not really a common nor severe issue. See the discussion in https://github.com/bitcoin/bitcoin/issues/34810#issuecomment-4045927137 and commit 5c005363a8, which had to remove dead code manually.
Reducing the confidence has shown to be too brittle/tedious in the past, so remove the tool for now from CI.
Of course, removing this from CI does not prevent anyone from running it locally and removing dead code.
Fixes https://github.com/bitcoin/bitcoin/issues/34810
ACKs for top commit:
fanquake:
ACK fa90b21430
willcl-ark:
ACK fa90b21430
Tree-SHA512: 6a5998470dae3a17baec29b70b02333f4cd9b81bc4c6a05b56085ff1ba527ed7bdeccd17b09d9ad785ae03c97982ee1f3147e4df3bd537c66b02e9a44d0e5f15
