facb2aab26 test: Turn ElapseSteady into SteadyClockContext (MarcoFalke)
Pull request description:
`ElapseSteady` was introduced a while back, but is only used in one place. It makes more sense if this were a context manager, so that mocktime does not leak from one test into the next.
So turn it into a context manager, rename it and allow easy time advancement via e.g. `steady_ctx += 1h`.
ACKs for top commit:
l0rinc:
ACK facb2aab26
ismaelsadeeq:
utACK facb2aab26
sedited:
ACK facb2aab26
Tree-SHA512: 1df9cc9685d9be4d3ab8deafd99ac1a5ff752064ae54b83bacd6f44ba2c198b091558a306d49d8b1e2200ac669e95915cc792d589fb3a63b2bef7891d325a1e0
6f7b4323cb test: remove UNKNOWN_ERROR from script_tests (Bruno Garcia)
bd31a92d67 script: use SCRIPT_ERR_SCRIPTNUM for CScriptNum errors (Bruno Garcia)
0ca4dcd786 script: add SCRIPT_ERR_SCRIPTNUM error (Bruno Garcia)
Pull request description:
When evaluating a script, the current code is bad for analyzing some errors because it returns `SCRIPT_ERR_UNKNOWN_ERROR` for errors that are clearly known.
`CScriptNum` has two well defined errors: number overflow and non-minimally encoded number. However, for both errors we return as unknown. This PR changes it by adding a new ScriptError that is used for any `CScriptNum` error.
ACKs for top commit:
achow101:
ACK 6f7b4323cb
w0xlt:
ACK 6f7b4323cb
darosior:
ACK 6f7b4323cb
Tree-SHA512: e656d9992251fbc95d33966fa18ce64bf714179d51ba6a7f429e5a55bc58e7fc08827e4ab71ace0dd385dac7e1feaea621b49503387793a30eae7a7e44aa6b0f
964c44cdcd test(miniscript): Prove avoidance of stack overflow (Hodlinator)
198bbaee49 refactor(miniscript): Destroy nodes one full subs-vector at a time (Hodlinator)
50cab8570e refactor(miniscript): Remove NodeRef & MakeNodeRef() (Hodlinator)
15fb34de41 refactor(miniscript): Remove superfluous unique_ptr-indirection (Hodlinator)
e55b23c170 refactor(miniscript): Remove Node::subs mutability (Hodlinator)
c6f798b222 refactor(miniscript): Make fields non-const & private (Hodlinator)
22e4115312 doc(miniscript): Remove mention of shared pointers (Hodlinator)
Pull request description:
Removes one level of unnecessary indirection, which was a change that originally [aided in finding one issue](https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2434704657) in #30866. Simplifies the code one step further than 09a1875ad8 belonging to aforementioned PR.
Also adds test which verifies resistance to stack overflow when it comes to `~Node()` and `Node::Clone()`.
No observed difference when running benchmarks: ExpandDescriptor/WalletIsMineDescriptors/WalletIsMineMigratedDescriptors/WalletLoadingDescriptors.
Followup to #30866.
ACKs for top commit:
achow101:
ACK 964c44cdcd
darosior:
Code review ACK 964c44cdcd
l0rinc:
ACK 964c44cdcd
Tree-SHA512: 32927e8f0f916fb70372ffd110f7ec7207d9e7a099c21c0a7482a12e96593b673c339719f4ab166ad7c086dc43767315fc1742c5b236a3facc45c4cfeb5872e9
d3e681bc06 fuzz: Use `__AFL_SHM_ID` for naming test directories (marcofleon)
Pull request description:
During long multicore fuzzing campaigns with AFL++, stale datadirs can eventually accumulate from time outs, resulting in disk running out of space (see https://github.com/bitcoin/bitcoin/issues/28811). The easiest way to reproduce this is by running our `utxo_total_supply` target using multiple cores with AFL++ and observing the crashes that occur because of all the directories in `/tmp/test_common\ bitcoin/utxo_total_supply/`.
Fix this by using the AFL++ shared memory ID to name the test dirs and cleaning it up before each setup. This ID is unique per AFL++ instance, so multiple cores can run in parallel without conflicts.
Fixes https://github.com/bitcoin/bitcoin/issues/28811
ACKs for top commit:
maflcko:
lgtm ACK d3e681bc06
dergoegge:
utACK d3e681bc06
Tree-SHA512: 420373e5f8a63c84797303ba2ef6657dfe9dacf9c2f3d818524421c24681a0e984c212ecb706217d93f67c2ec16b146a2d37fddcbd6918b2e5e9f634f5e13c10
3e0fd0e4dd refactor: rename will_reuse_cache to reallocate_cache (Andrew Toth)
44b4ee194d validation: reuse same CCoinsViewCache for every ConnectBlock call (Andrew Toth)
8fb6043231 coins: introduce CCoinsViewCache::ResetGuard (Andrew Toth)
041758f5ed coins: use hashBlock setter internally for CCoinsViewCache methods (Andrew Toth)
8dd9200fc9 coins: add Reset on CCoinsViewCache (Andrew Toth)
Pull request description:
This is the first commit of #31132, which can be merged as an independent change. It has a small benefit on its own, but will help in moving the parent PR forward.
Add a `Reset()` method to `CCoinsViewCache` that clears `cacheCoins`, `cachedCoinsUsage`, and `hashBlock` without flushing to the `base` view. This allows efficiently reusing a cache instance across multiple blocks.
Add `CCoinsViewCache::CreateResetGuard` method to return a `CCoinsViewCache::ResetGuard`. The `ResetGuard` automatically calls `Reset()` on destruction. This RAII pattern ensures the cache is always properly reset between blocks.
Add `m_connect_block_view` as a persistent `CCoinsViewCache` for `ConnectBlock`, avoiding repeated memory allocations.
ACKs for top commit:
l0rinc:
ACK 3e0fd0e4dd
achow101:
ACK 3e0fd0e4dd
sedited:
ACK 3e0fd0e4dd
Tree-SHA512: a95feaa062a9eb7cf7514425a7e7adffd347cd1f7b32b4c1fefcde30002141757c184174702b3104a029dcd33194f8bd734159deebb2e668716089305b42cb00
c6ca2b85a3 validation: do not wipe utxo cache for stats/scans/snapshots (Pieter Wuille)
7099e93d0a refactor: rename `FlushStateMode::ALWAYS` to `FORCE_FLUSH` (Lőrinc)
Pull request description:
Revival of https://github.com/bitcoin/bitcoin/pull/30610#issuecomment-3432564955 with the remaining comments applied on top
> Since #28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful.
>
> Split the `FlushStateMode::ALWAYS` mode into a FORCE_SYNC (non-wiping) and a FORCE_FLUSH (wiping), and then use the former in `scantxoutset`, `gettxoutsetinfo`, snapshot creation.
(slightly updated after #30214)
ACKs for top commit:
optout21:
reACK c6ca2b85a3
cedwies:
reACK c6ca2b8 (trivial)
achow101:
ACK c6ca2b85a3
sedited:
ACK c6ca2b85a3
Tree-SHA512: f3525a85dc512db4a0a9c749ad47c0d3fa44085a121aa54cd77646260a719c71f754ec6570ae77779c0ed68a24799116f79c686e7a17ce57a26f6a598f7bf926
CCoinsViewCache::CreateResetGuard returns a guard that calls
Reset on the cache when the guard goes out of scope.
This RAII pattern ensures the cache is always properly reset
when it leaves current scope.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: sedited <seb.kung@gmail.com>
Use the AFL++ shared memory ID environment variable to create
a deterministic datadir path. This prevents accumulation of stale
directories after a fuzz iteration crashes or times out. During
long fuzz campaigns, this accumulation has occasionally resulted
in running out of disk space.
e770392084 test: addrman: test self-announcement time penalty handling (Bruno Garcia)
Pull request description:
This PR adds a test case for addrman that verifies that addresses announcing themselves (addr == source) are exempt from time penalties, while addresses announced by others receive the expected penalty.
It fixes the following mutant (https://corecheck.dev/mutation/src/addrman.cpp#L561):
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index 206b54118e..c6a045fd8d 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -558,7 +558,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, std::c
AddrInfo* pinfo = Find(addr, &nId);
// Do not set a penalty for a source's self-announcement
- if (addr == source) {
+ if (addr != source) {
time_penalty = 0s;
}
```
ACKs for top commit:
maflcko:
review ACK e770392084🐤
achow101:
ACK e770392084
fjahr:
Code review ACK e770392084
naiyoma:
tACK e770392084
Tree-SHA512: ec029d1e1e979f91840af944984cad530a1ce9a0eceb123230817f0ef3b9ad47253eebc4c953d350de2d904b59496fcd4757123c8bd63cf0e09c3581da48fff8
2ee7f9b259 coins: assume `GetCoin` only returns unspent coins (Andrew Toth)
eec551aaf1 fuzz: keep `coinscache_sim` backend free of spent coins (Andrew Toth)
3e4155fcef test: do not return spent coins from `CCoinsViewTest::GetCoin` (Andrew Toth)
ee1e40f580 txdb: assert `CCoinsViewDB::GetCoin` only returns unspent coins (Lőrinc)
Pull request description:
This PR is split out from #33018 to keep that PR focused on removing the `FRESH-but-not-DIRTY` cache state.
### Problem
`::GetCoin()` is an interface for querying the UTXO set, so production implementations should only ever return unspent coins. Tests should mimic this to provide useful feedback.
### Fix:
* Add a fail-fast assertion that `CCoinsViewDB::GetCoin()` never returns a spent coin.
* Align unit tests and fuzz simulations with the production `GetCoin()` contract by never returning spent coins.
* Replace the unreachable “spent coin returned by parent” handling in `CCoinsViewCache::FetchCoin()` with `Assert(!coin.IsSpent())`, drop outdated `spent+FRESH` docs, and tighten `SanityCheck()` invariants.
Behavior is unchanged, it just aligns our tests to exercise valid states.
ACKs for top commit:
andrewtoth:
re-ACK 2ee7f9b259
optout21:
crACK 2ee7f9b259
achow101:
ACK 2ee7f9b259
w0xlt:
reACK 2ee7f9b259
Tree-SHA512: be21cc09690410fc04ca25e1ba47aae6186bc037e413b3bb1e6e9a04e6364cbfac5a2fcdc49b638fec848cd29243fab0cc0581b9923f34fafe8366828f690ed4
Verify that addresses announcing themselves (addr == source) are exempt
from time penalties, while addresses announced by others receive the
expected penalty.
14f99cfe53 rpc: make `uptime` monotonic across NTP jumps (Lőrinc)
a9440b1595 util: add `TicksSeconds` (Lőrinc)
Pull request description:
### Problem
`bitcoin-cli uptime` was derived from wall-clock time, so it could jump by large amounts when the system clock is corrected after `bitcoind` starts (e.g. on RTC-less systems syncing NTP).
This breaks the expectation that uptime reflects process runtime.
### Fix
Compute uptime from a [monotonic clock](https://en.cppreference.com/w/cpp/chrono/steady_clock.html) so it is immune to wall-clock jumps, and use that monotonic uptime for the RPC.
GUI startup time is derived from wall clock time minus monotonic uptime so it remains sensible after clock corrections.
### Reproducer
Revert the fix commit and run the `rpc_uptime` functional test (it should fail with `AssertionError: uptime should not jump with wall clock`):
Or alternatively:
```bash
cmake -B build && cmake --build build --target bitcoind bitcoin-cli -j$(nproc)
DATA_DIR=$(mktemp -d)
./build/bin/bitcoind -regtest -datadir="$DATA_DIR" -connect=0 -daemon
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" -rpcwait uptime
sleep 1
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" setmocktime $(( $(date +%s) + 20000000 ))
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" uptime
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" stop
```
<details>
<summary>Before (uptime jumps with wall clock)</summary>
```bash
Bitcoin Core starting
0
20000001
Bitcoin Core stopping
```
</details>
<details>
<summary>After (uptime stays monotonic)</summary>
```bash
Bitcoin Core starting
0
1
Bitcoin Core stopping
```
</details>
----------
Issue: https://github.com/bitcoin/bitcoin/issues/34326
ACKs for top commit:
maflcko:
review ACK 14f99cfe53🎦
willcl-ark:
tACK 14f99cfe53
w0xlt:
ACK 14f99cfe53
sedited:
ACK 14f99cfe53
Tree-SHA512: 3909973f58666ffa0b784a6df087031b9e34d2022d354900a4dbb6cbe1d36285cd92770ee71350ebf64d6e8ab212d8ff0cd851f7dca1ec46ee2f19b417f53984
a73a3ec553 doc: fix invalid arg name hints for bugprone validation (Lőrinc)
Pull request description:
The extra leading `=` or missing trailing `=` prevented clang-tidy's `bugprone-argument-comment` check from validating the parameter name, as it only matches comments formatted strictly as `/*parameter_name=*/` (see https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html).
I have considered doing a scripted diff, but the values I found aren't so numerous and can easily be reviewed manually.
ACKs for top commit:
b-l-u-e:
ACK a73a3ec tested and saw that argument comments now use the strict "/*param=*/" format required by bugprone-argument-comment
Sjors:
utACK a73a3ec553
maflcko:
review ACK a73a3ec553🍦
Tree-SHA512: 31177934d645116f381668a0f945028d7e04fab1fc6185dd0e3b7451aab71f89f1e4dd07246db667d1c4734eea3e5d73433c8b0e09181b3ece47dacc8677401e
eeee3755f8 fuzz: Return chrono point from ConsumeTime(), Add ConsumeDuration() (MarcoFalke)
faa5a9ebad fuzz: Use min option in ConsumeTime (MarcoFalke)
Pull request description:
Returning a raw i64 is a bit confusing when it comes to chrono types. For example, in the addrman fuzz tests, the `time_penalty` is not a time point, but a duration.
Also, all call-sites assume second resolution right now, so document that better by returning `NodeSeconds` from `ConsumeTime(...)` and `std::chrono::seconds` from `ConsumeDuration(...)`.
ACKs for top commit:
l0rinc:
ACK eeee3755f8
Crypt-iQ:
crACK eeee3755f8
Tree-SHA512: 25dd779a1bf79fa42c6e69db0f0593ad4daa4c0d746e8e82a26bdd65391a27c38e484431056d4e2207b542c511a71cb536c259809728a7166b8d304c0490e321
9a9d797ef6 kernel: Add support for block headers (yuvicc)
b851ff6cae kernel: Add Handle/View pattern for BlockValidationState (yuvicc)
Pull request description:
Adds a new `btck_BlockHeader` type and associated functions to create, access, and validate block headers. Block headers will have their own type (`btck_BlockHeader`) that can be created from raw data, copied, and queried for all the standard header fields (hash, prev hash, timestamp, bits, version, nonce). We can also extract headers from full blocks or block tree entries.
The first commit here refactors `BlockValidationState` to use Handle/View pattern so external code can own them, which is required for the header processing in the API.
#### New Block Header API
- **`btck_BlockHeader` type**: Opaque handle for block headers
- **Header methods**:
- `btck_block_header_create()`: Create header from 80-byte serialized data
- `btck_block_header_copy()`: Copy block headers
- `btck_block_header_destroy()`: Destroy header object
- `btck_block_header_get_hash()`: Calculate block hash
- `btck_block_header_get_prev_hash()`: Get previous block hash
- `btck_block_header_get_timestamp()`: Get block timestamp
- `btck_block_header_get_bits()`: Get difficulty target (compact format)
- `btck_block_header_get_version()`: Get block version
- `btck_block_header_get_nonce()`: Get proof-of-work nonce
- `btck_block_get_header()`: Extract header from a full block
- `btck_block_tree_entry_get_block_header()`: Get header associated with a block tree entry
- **Header Processing Methods:**
- **`btck_chainstate_manager_process_block_header()`**: Validates and processes a block header without requiring the full block. This performs proof-of-work verification, timestamp validation, and updates the internal chain state.
- **`btck_chainstate_manager_get_best_entry()`**: Returns the block tree entry with the most cumulative proof-of-work.
Why `btck_chainstate_manager_get_best_entry()` is included alongside header validation? Just as we have logic to get the tip for block validation (so you can request more blocks extending your best from your peers), we need the equivalent for header validation. To make header validation worthwhile, knowing what the best current header is seems useful—it tells you what headers to request next from peers.
### Testing
Added tests in `test_kernel.cpp` that cover creating headers from raw data, extracting all header fields, and processing headers through the chainstate manager.
CC sedited
ACKs for top commit:
stringintech:
re-ACK 9a9d797e
sedited:
Re-ACK 9a9d797ef6
janb84:
ACK 9a9d797ef6
Tree-SHA512: 1dde9ef860543c906d1bb5e604f0d2956e7382fcbb55090686261b2277270a1fd3826f02ecf1749b2774da66e88f686c7845172b4c68b62259e7a7aee0825fa2
1911db8c6d string: add LineReader (Matthew Zipkin)
ee62405cce time: implement and test RFC1123 timestamp string (Matthew Zipkin)
eea38787b9 string: add AsciiCaseInsensitive{KeyEqual, Hash} for unordered map (Matthew Zipkin)
4e300df712 string: add `base` argument for ToIntegral to operate on hexadecimal (Matthew Zipkin)
0b0d9125c1 Modernize GetBindAddress() (Matthew Zipkin)
a0ca851d26 Make GetBindAddress() callable from outside net.cpp (Matthew Zipkin)
Pull request description:
This is a component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194). It is the first six commits of #32061 and provides a string-parsing utility (`LineReader`) that is also consumed by #34158.
These are the functions that are added / updated for HTTP and Torcontrol:
- `GetBindAddress()`: Given a socket, provides the bound address as a CService. Currently used by p2p but moved from `net` to `netbase` so other modules can call it.
- `ToIntegral()`: Already used to parse numbers from strings, added new argument `base = 10` so it can also be used to parse hexadecimal integers. HTTP chunked transfer-encoding uses hex-encoded integers to specify payload size: https://datatracker.ietf.org/doc/html/rfc7230.html#section-4.1
- `AsciiCaseInsensitive` comparators: Needed to store HTTP headers in an `unordered_map`. Headers are key-value pairs that are parsed with case-insensitive keys: https://httpwg.org/specs/rfc9110.html#rfc.section.5.1
- `FormatRFC1123DateTime()`: The required datetime format for HTTP headers (e.g. `Fri, 31 May 2024 19:18:04 GMT`)
- `LineReader`: Fields in HTTP requests are newline-terminated. This struct is given an input buffer and provides methods to read lines as strings.
ACKs for top commit:
maflcko:
review ACK 1911db8c6d👲
furszy:
utACK 1911db8c6d
sedited:
ACK 1911db8c6d
Tree-SHA512: bb8d3b7b18f158386fd391df6d377c9f5b181051dc258efbf2a896c42e20417a1b0b0d4637671ebd2829f6bc371daa15775625af989c19ef8aee76118660deff
fab2f3df4b fuzz: Exclude too expensive inputs in descriptor_parse targets (MarcoFalke)
Pull request description:
Accepting "expensive" fuzz inputs which have no real use-case is problematic, because it prevents the fuzz engine from spending time on the next useful fuzz input.
For example, those will take several seconds (!) and the flamegraph shows that base58 encoding is the cause:
```
curl -fLO 'f5abf41608'
curl -fLO '78cb317546'
FUZZ=mocked_descriptor_parse ./bld-cmake/bin/fuzz ./f5abf41608addcef3538da61d8096c2050235032
FUZZ=descriptor_parse ./bld-cmake/bin/fuzz ./78cb3175467f53b467b949883ee6072e92dbb267
```
This will also break 32-bit fuzzing, see https://github.com/bitcoin/bitcoin/issues/34110#issuecomment-3759461248.
Fix all issues by checking for `HasTooLargeLeafSize`.
Sorry for creating several pull requests to fix this class of issue, but I think this one should be the last one. 😅
ACKs for top commit:
brunoerg:
reACK fab2f3df4b
frankomosh:
re-ACK fab2f3df4b
Tree-SHA512: 4ecf98ec4adc39f6e014370945fb1598cdd3ceba60f7209b00789ac1164b6d20e82a69d71f8419d9a40d57ee3fea36ef593c47fe48b584b6e8344c44f20a15c1
c9ce1c7c4a test: Fix P2PK script test (billymcbip)
Pull request description:
I found another script_tests case that isn't behaving the way it was meant to. It's a P2PK spend where we add an `OP_NOP8` to the scriptSig to make it non-push-only. The test should check that [`scriptSig.IsPushOnly()`](691dc830c6/src/script/interpreter.cpp (L2055)) is only enforced in P2SH mode when the scriptPubKey actually matches the P2SH pattern. To test this, we need to **turn on the P2SH flag**.
ACKs for top commit:
sipa:
ACK c9ce1c7c4a
darosior:
utACK c9ce1c7c4a
Tree-SHA512: 0af1d7b4651478349abc97cf0c009488cf5af5f97135382f7dd37cef0ef9b563192244330899a54ee7e0296bf03ba702e37a7aa15248c5c0ab4745095efc2402
8b9d30e3fa bench/test: clarify merkle bench and witness test intent (Lőrinc)
Pull request description:
Follow-up to #32497.
Clarify why the witness merkle test uses an odd leaf count (it exercises leaf duplication in `ComputeMerkleRoot()`), and make the coinbase witness hash initialization explicit.
Also simplify the leaf-copy loop in the `MerkleRoot` benchmark for readability.
No production code is changed in this follow-up, for simplicity and safety.
ACKs for top commit:
optout21:
ACK 8b9d30e3fa
maflcko:
lgtm ACK 8b9d30e3fa
achow101:
ACK 8b9d30e3fa
w0xlt:
ACK 8b9d30e3fa
danielabrozzoni:
tACK 8b9d30e3fa
Tree-SHA512: 6efca7c19ebf96bb8d0def4217ed30d3b74b58a7be15566967e98aba9b03aaddd0e0ebb3b8f43130b5f397a7d9eed0470a48a55438f440e0bceefb87edd16b27
This is a helper struct to parse HTTP messages from data in buffers
from sockets. HTTP messages begin with headers which are
CRLF-terminated lines (\n or \r\n) followed by an arbitrary amount of
body data. Whitespace is trimmed from the field lines but not the body.
https://httpwg.org/specs/rfc9110.html#rfc.section.5
https://httpwg.org/specs/rfc9110.html#rfc.section.5.1
Field names in HTTP headers are case-insensitive. These
structs will be used in the headers map to search by key.
In libevent field names are also converted to lowercase for comparison:
evhttp_find_header()
evutil_ascii_strcasecmp()
EVUTIL_TOLOWER_()
Introduces btck_BlockHeader type with accessor methods and btck_chainstate_manager_process_block_header() for validating headers without full blocks. Also, adds btck_chainstate_manager_get_best_entry() to query the header with most cumulative proof-of-work.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
Follow-up to bitcoin/bitcoin#32497.
Clarify why the witness merkle test uses an odd leaf count (it exercises leaf duplication in `ComputeMerkleRoot()`), and make the coinbase witness hash initialization explicit.
Also simplify the leaf-copy loop in the MerkleRoot benchmark for readability.
No production code is changed in this follow-up, for simplicity and safety.
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
faa18dceba refactor: Use std::bind_front over std::bind (MarcoFalke)
Pull request description:
`std::bind` has many issues:
* It is verbosely listing all placeholders, but in a meaningless way, because it doesn't name the args or their types.
* It silently ignores args passed to it, when one arg is overridden. For example [1] compiles fine on current master.
* Accidentally duplicated placeholders compile fine as well.
* Usually the placeholders aren't even needed.
* This makes it hard to review, understand, and maintain.
Fix all issues by using `std::bind_front` from C++20, which allows to drop the brittle `_1, _2, ...` placeholders. The replacement should be correct, if the trailing placeholders are ordered.
Introducing the same silent bug on top of this pull request [2] will now lead to a compile failure.
----
[1]
```diff
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 694fb535b5..7661dd361e 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -412,3 +412,3 @@ void WalletModel::subscribeToCoreSignals()
m_handler_status_changed = m_wallet->handleStatusChanged(std::bind(&NotifyKeyStoreStatusChanged, this));
- m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5));
+ m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, CTxDestination{}, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5));
m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind(NotifyTransactionChanged, this, std::placeholders::_1, std::placeholders::_2));
```
[2]
```diff
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 578713c0ab..84cced741c 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -412,3 +412,3 @@ void WalletModel::subscribeToCoreSignals()
m_handler_status_changed = m_wallet->handleStatusChanged(std::bind_front(&NotifyKeyStoreStatusChanged, this));
- m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind_front(NotifyAddressBookChanged, this));
+ m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind_front(NotifyAddressBookChanged, this, CTxDestination{}));
m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind_front(NotifyTransactionChanged, this));
ACKs for top commit:
janb84:
cr ACK faa18dceba
fjahr:
Code review ACK faa18dceba
hebasto:
ACK faa18dceba, I have reviewed the code and it looks OK.
Tree-SHA512: 9dd13f49527e143a2beafbaae80b1358981f07a2ce20d25cffb1853089a32ff71639e6d718d1d193754522f9ac04e3e168ba017d5fc67a11a5918e79a92b3461
Add C API functions for managing BlockValidationState lifecycle:
- btck_block_validation_state_create()
- btck_block_validation_state_copy()
- btck_block_validation_state_destroy()
Introduce BlockValidationStateApi<> template to share common getter methods between BlockValidationState (Handle) and BlockValidationStateView (View) classes in the C++ wrapper. This enables external code to create and own BlockValidationState objects needed for the new process_block_header() API.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
faa59b3679 util: Add Expected::swap() (MarcoFalke)
fabb47e4e3 util: Implement Expected::operator*()&& (MarcoFalke)
fab9721430 util: Implement Expected::value()&& and Expected::error()&& (MarcoFalke)
fac4800959 util: Add Expected<void, E> specialization (MarcoFalke)
fa6575d6c2 util: Make Expected::value() throw (MarcoFalke)
fa1de1103f util: Add Unexpected::error() (MarcoFalke)
faa109f8be test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK == (MarcoFalke)
fad4a9fe2b Set bugprone-unused-return-value.AllowCastToVoid (MarcoFalke)
Pull request description:
Reviewers requested more member functions In https://github.com/bitcoin/bitcoin/pull/34006.
They are currently unused, but bring the port closer to the original `std::expected` implementation:
* Make `Expected::value()` throw when no value exists
* Add `Unexpected::error()` methods
* Add `Expected<void, E>` specialization
* Add `Expected::value()&&` and `Expected::error()&&` methods
* Add `Expected::swap()`
Also, include a tiny tidy fixup:
* tidy: Set `AllowCastToVoid` in the `bugprone-unused-return-value` check
ACKs for top commit:
stickies-v:
re-ACK faa59b3679
ryanofsky:
Code review ACK faa59b3679. Thanks for the update. The commit I objected to is fixed now and the rest of the implementation seems good enough for code that's probably temporary.
hodlinator:
re-ACK faa59b3679
Tree-SHA512: b6ac28c1e7241837d9db83fe7534d713ca1283c20a77d2273743157d329f041ec0b503658d14b2f4425211808b61a88fed115d77149e0546825acd3bd9198edf
3dd815f048 validation: pre-reserve leaves to prevent reallocs with odd vtx count (Lőrinc)
7fd47e0e56 bench: make `MerkleRoot` benchmark more representative (Lőrinc)
f0a2183108 test: adjust `ComputeMerkleRoot` tests (Lőrinc)
Pull request description:
#### Summary
`ComputeMerkleRoot` [duplicates the last hash](39b6c139bd/src/consensus/merkle.cpp (L54-L56)) when the input size is odd. If the caller provides a `std::vector` whose capacity equals its size, that extra `push_back` forces a reallocation, doubling its capacity (causing peak memory usage of 3x the necessary size).
This affects roughly half of the created blocks (those with odd transaction counts), causing unnecessary memory fragmentation during every block validation.
#### Fix
* Pre-reserves vector capacity to account for the odd-count duplication using `(size + 1) & ~1ULL`.
* This syntax produces [optimal assembly](https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2553107836) across x86/ARM and 32/64-bit platforms for GCC & Clang.
* Eliminates default construction of `uint256` objects that are immediately overwritten by switching from `resize` to `reserve` + `push_back`.
#### Memory Impact
[Memory profiling](https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3563724551) shows **50% reduction in peak allocation** (576KB → 288KB) and elimination of reallocation overhead.
#### Validation
The benchmark was updated to use an odd leaf count to demonstrate the real-world scenario where the reallocation occurs.
A full `-reindex-chainstate` up to block **896 408** ran without triggering the asserts.
<details>
<summary>Validation asserts</summary>
Temporary asserts (not included in this PR) confirm that `push_back` never reallocates and that the coinbase witness hash remains null:
```cpp
if (hashes.size() & 1) {
assert(hashes.size() < hashes.capacity()); // TODO remove
hashes.push_back(hashes.back());
}
leaves.reserve((block.vtx.size() + 1) & ~1ULL); // capacity rounded up to even
leaves.emplace_back();
assert(leaves.back().IsNull()); // TODO remove
```
</details>
#### Benchmark Performance
While the main purpose is to improve predictability, the reduced memory operations also improve hashing throughput slightly.
ACKs for top commit:
achow101:
ACK 3dd815f048
optout21:
reACK 3dd815f048
hodlinator:
re-ACK 3dd815f048
vasild:
ACK 3dd815f048
w0xlt:
ACK 3dd815f048 with minor nits.
danielabrozzoni:
Code review ACK 3dd815f048
Tree-SHA512: e7b578f9deadc0de7d61c062c7f65c5e1d347548ead4a4bb74b056396ad7df3f1c564327edc219670e6e2b2cb51f4e1ccfd4f58dd414aeadf2008d427065c11f
969c840db5 log,blocks: avoid `ComputeTotalSize` and `GetHash` work when logging is disabled (Lőrinc)
babfda332b log,net: avoid `ComputeTotalSize` when logging is disabled (Lőrinc)
1658b8f82b refactor: rename `CTransaction::GetTotalSize` to signal that it's not cached (Lőrinc)
Pull request description:
### Context
The new accounting options introduced in https://github.com/bitcoin/bitcoin/pull/32582 can be quite heavy, and are not needed when debug logging is disabled.
### Problem
`PartiallyDownloadedBlock::FillBlock()` and `PeerManagerImpl::SendBlockTransactions()` accumulate transaction sizes for debug logging by calling `ComputeTotalSize()` in loops, which invokes expensive `GetSerializeSize()` serializations.
The block header hash is also only computed for the debug log.
### Fixes
Guard the size and hash calculations with `LogAcceptCategory()` checks so the serialization and hashing work only occurs when compact block debug logging is enabled.
Also modernized the surrounding code a bit since the change is quite trivial.
### Reproducer
You can test the change by starting an up-to-date `bitcoind` node with `-debug=cmpctblock` and observing compact block log lines such as:
> [cmpctblock] Successfully reconstructed block 00000000000000000001061eaa6c0fe79258e7f79606e67ac495765cb121a520 with 1 txn prefilled, 3122 txn from mempool (incl at least 3 from extra pool) and 641 txn (352126 bytes) requested
<details>
<summary>Test patch</summary>
```patch
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index 58620c93cc..f16eb38fa5 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -186,6 +186,7 @@ bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const
ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing, bool segwit_active)
{
+ LogInfo("PartiallyDownloadedBlock::FillBlock called");
if (header.IsNull()) return READ_STATUS_INVALID;
block = header;
@@ -218,6 +219,7 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
}
if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) {
+ LogInfo("debug log enabled");
const uint256 hash{block.GetHash()}; // avoid cleared header
uint32_t tx_missing_size{0};
for (const auto& tx : vtx_missing) tx_missing_size += tx->ComputeTotalSize(); // avoid cleared txn_available
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 5600c8d389..c081825f77 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2470,6 +2470,7 @@ uint32_t PeerManagerImpl::GetFetchFlags(const Peer& peer) const
void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlock& block, const BlockTransactionsRequest& req)
{
+ LogInfo("PeerManagerImpl::SendBlockTransactions called");
BlockTransactions resp(req);
for (size_t i = 0; i < req.indexes.size(); i++) {
if (req.indexes[i] >= block.vtx.size()) {
@@ -2480,6 +2481,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo
}
if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) {
+ LogInfo("debug log enabled");
uint32_t tx_requested_size{0};
for (const auto i : req.indexes) tx_requested_size += block.vtx[i]->ComputeTotalSize();
LogDebug(BCLog::CMPCTBLOCK, "Peer %d sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)\n", pfrom.GetId(), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
```
</details>
ACKs for top commit:
davidgumberg:
reACK 969c840db5
achow101:
ACK 969c840db5
hodlinator:
re-ACK 969c840db5
sedited:
Re-ACK 969c840db5
danielabrozzoni:
reACK 969c840db5
Tree-SHA512: 9780102d29778165144e3602d934ed4cb96660fd7b9ff2581b223c619e419139b8348e60f226af448702ae527736a1806d169b44342c5a82795590f664e16efe
a3c71c7201 [test] Add BIP 328 test vectors for Musig2 (w0xlt)
Pull request description:
Built on https://github.com/bitcoin/bitcoin/pull/31244
This PR adds explicit tests for Bitcoin Core's MuSig2 interface.
Any issues in musig2.{cpp,h} will likely also be caught by the descriptor tests, but having more detailed tests for the MuSig2 class itself improves test reporting/coverage.
It uses BIP 328 test vectors.
ACKs for top commit:
achow101:
ACK a3c71c7201
rkrux:
lgtm ACK a3c71c7
Tree-SHA512: fc13beb5445c292cd7c75a47810fb1c4032ee2e3c1800dc44089b95959ccce8330291084bf788457e1d55c02d706ef04be7044badfee134149e004c44b19ec32
Can be tested through emptying the function body of ~Node() or replacing Clone() implementation with naive version:
```C++
Node<Key> Clone() const
{
std::vector<Node> new_subs;
new_subs.reserve(subs.size());
for (const Node& child : subs) {
new_subs.push_back(child.Clone());
}
return Node{internal::NoDupCheck{}, m_script_ctx, fragment, std::move(new_subs), keys, data, k};
}
```
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Functional parity is achieved through making Node move-able.
Unfortunately ~Node() now needs to have the recursion linter disabled, as it is unable to figure out that recursion stops 1 level down. The former smart pointers must have been circumventing the linter somehow.
NodeRef & MakeNodeRef() are deleted in the following commit (broken out to facilitate review).
Makes a lot of fields in miniscript.h non-const in order to allow move-operations 2 commits later.
Also fixes adjacent comment typos.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
9c7e4771b1 test: Test listdescs with priv works even with missing priv keys (Novo)
ed945a6854 walletrpc: reject listdes with priv key on w-only wallets (Novo)
9e5e9824f1 descriptor: ToPrivateString() pass if at least 1 priv key exists (Novo)
5c4db25b61 descriptor: refactor ToPrivateString for providers (Novo)
2dc74e3f4e wallet/migration: use HavePrivateKeys in place of ToPrivateString (Novo)
e842eb90bb descriptors: add HavePrivateKeys() (Novo)
Pull request description:
_TLDR:
Currently, `listdescriptors [private=true]` will fail for a non-watch-only wallet if any descriptor has a missing private key(e.g `tr()`, `multi()`, etc.). This PR changes that while making sure `listdescriptors [private=true]` still fails if there no private keys. Closes #32078_
In non-watch-only wallets, it's possible to import descriptors as long as at least one private key is included. It's important that users can still view these descriptors when they need to create a backup—even if some private keys are missing ([#32078 (comment)](https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2781428475)). This change makes it possible to do so.
This change also helps prevent `listdescriptors true` from failing completely, because one descriptor is missing some private keys.
### Notes
- The new behaviour is applied to all descriptors including miniscript descriptors
- `listdescriptors true` still fails for watch-only wallets to preserve existing behaviour https://github.com/bitcoin/bitcoin/pull/24361#discussion_r920801352
- Wallet migration logic previously used `Descriptor::ToPrivateString()` to determine which descriptor was watchonly. This means that modifying the `ToPrivateString()` behaviour caused descriptors that were previously recognized as "watchonly" to be "non-watchonly". **In order to keep the scope of this PR limited to the RPC behaviour, this PR uses a different method to determine `watchonly` descriptors for the purpose of wallet migration.** A follow-up PR can be opened to update migration logic to exclude descriptors with some private keys from the `watchonly` migration wallet.
### Relevant PRs
https://github.com/bitcoin/bitcoin/pull/24361https://github.com/bitcoin/bitcoin/pull/32186
### Testing
Functional tests were added to test the new behaviour
EDIT
**`listdescriptors [private=true]` will still fail when there are no private keys because non-watchonly wallets must have private keys and calling `listdescriptors [private=true]` for watchonly wallet returns an error**
ACKs for top commit:
Sjors:
ACK 9c7e4771b1
achow101:
ACK 9c7e4771b1
w0xlt:
reACK 9c7e4771b1 with minor nits
rkrux:
re-ACK 9c7e4771b1
Tree-SHA512: f9b3b2c3e5425a26e158882e39e82e15b7cb13ffbfb6a5fa2868c79526e9b178fcc3cd88d3e2e286f64819d041f687353780bbcf5a355c63a136fb8179698b60
fabf8d1c5b fuzz: Restore SendMessages coverage in process_message(s) fuzz targets (MarcoFalke)
fac7fed397 refactor: Use std::reference_wrapper<AddrMan> in Connman (MarcoFalke)
Pull request description:
*Found and reported by Crypt-iQ (thanks!)*
Currently the process_message(s) fuzz targets do not have any meaningful `SendMessages` code coverage. This is not ideal.
Fix the problem by adding back the coverage, and by hardening the code here, so that the problem hopefully does not happen again in the future.
### Historic context for this regression
The regression was introduced in commit fa11eea405, which built a new deterministic peerman object. However, the patch was incomplete, because it was missing one hunk to replace `g_setup->m_node.peerman->SendMessages(&p2p_node);` with `peerman->SendMessages(&p2p_node);`.
This means the stale and empty peerman from the node context and not the freshly created and deterministic peerman was used.
A simple fix would be to just submit the missing patch hunk. However, this still leaves the risk that the issue is re-introduced at any time in the future. So instead, I think the stale and empty peerman should be de-constructed, so that any call to it will lead to a hard sanitizer error and fuzz failure.
Doing that also uncovered another issue: The connman was holding on to a reference to a stale and empty addrman.
So fix all issues by:
* Allowing the addrman reference in connman to be re-seatable
* Clearing all stale objects, before creating new objects, and then using references to the new objects in all code
ACKs for top commit:
Crypt-iQ:
crACK fabf8d1c5b
frankomosh:
ACK fabf8d1c5b
marcofleon:
code review ACK fabf8d1c5b
sedited:
ACK fabf8d1c5b
Tree-SHA512: 2e478102b3e928dc7505f00c08d4b9e4f8368407b100bc88f3eb3b82aa6fea5a45bae736c211f5af1551ca0de1a5ffd4a5d196d9473d4c3b87cfed57c9a0b69d
