6f7b4323cb46687ba9df7073a9cde427985d2dfc test: remove UNKNOWN_ERROR from script_tests (Bruno Garcia)
bd31a92d671610e1173a4e6f0c761d94724441ae script: use SCRIPT_ERR_SCRIPTNUM for CScriptNum errors (Bruno Garcia)
0ca4dcd78665bc5258a2b9cfcef0dcdf971a88c3 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 6f7b4323cb46687ba9df7073a9cde427985d2dfc
w0xlt:
ACK 6f7b4323cb
darosior:
ACK 6f7b4323cb46687ba9df7073a9cde427985d2dfc
Tree-SHA512: e656d9992251fbc95d33966fa18ce64bf714179d51ba6a7f429e5a55bc58e7fc08827e4ab71ace0dd385dac7e1feaea621b49503387793a30eae7a7e44aa6b0f
964c44cdcd6be5f39aed1aeda9c305803eb3b25f test(miniscript): Prove avoidance of stack overflow (Hodlinator)
198bbaee4959119a63b4038cd0dbb519f4daf6f0 refactor(miniscript): Destroy nodes one full subs-vector at a time (Hodlinator)
50cab8570e8f7553a94e750f66ad9228a728e72e refactor(miniscript): Remove NodeRef & MakeNodeRef() (Hodlinator)
15fb34de41cb069e2bad93a64722bdb32ff00690 refactor(miniscript): Remove superfluous unique_ptr-indirection (Hodlinator)
e55b23c170eb1a80a71e2de8b48cf8a0aebda843 refactor(miniscript): Remove Node::subs mutability (Hodlinator)
c6f798b22247bc092e72eed9e9f69a0cbaca5134 refactor(miniscript): Make fields non-const & private (Hodlinator)
22e4115312b929502574ba3681ee2c3b3fd14d96 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 09a1875ad8cddeb17c19af34b8282d37fed0937e 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 964c44cdcd6be5f39aed1aeda9c305803eb3b25f
darosior:
Code review ACK 964c44cdcd6be5f39aed1aeda9c305803eb3b25f
l0rinc:
ACK 964c44cdcd6be5f39aed1aeda9c305803eb3b25f
Tree-SHA512: 32927e8f0f916fb70372ffd110f7ec7207d9e7a099c21c0a7482a12e96593b673c339719f4ab166ad7c086dc43767315fc1742c5b236a3facc45c4cfeb5872e9
d3e681bc06758fe0686cd96fcfd4a1c4c5af62b4 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 d3e681bc06758fe0686cd96fcfd4a1c4c5af62b4
dergoegge:
utACK d3e681bc06758fe0686cd96fcfd4a1c4c5af62b4
Tree-SHA512: 420373e5f8a63c84797303ba2ef6657dfe9dacf9c2f3d818524421c24681a0e984c212ecb706217d93f67c2ec16b146a2d37fddcbd6918b2e5e9f634f5e13c10
3e0fd0e4ddd894f0e7db1772f10ceaa1dddfb951 refactor: rename will_reuse_cache to reallocate_cache (Andrew Toth)
44b4ee194d3bdccd86cf5e151b2fc1479aabbb6c validation: reuse same CCoinsViewCache for every ConnectBlock call (Andrew Toth)
8fb6043231ea396aaa1165b36b082c89e10fcafd coins: introduce CCoinsViewCache::ResetGuard (Andrew Toth)
041758f5eda5725daad4ae20f66c7d19ba02d063 coins: use hashBlock setter internally for CCoinsViewCache methods (Andrew Toth)
8dd9200fc9b0d263f8f75943ce581a925d061378 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 3e0fd0e4ddd894f0e7db1772f10ceaa1dddfb951
achow101:
ACK 3e0fd0e4ddd894f0e7db1772f10ceaa1dddfb951
sedited:
ACK 3e0fd0e4ddd894f0e7db1772f10ceaa1dddfb951
Tree-SHA512: a95feaa062a9eb7cf7514425a7e7adffd347cd1f7b32b4c1fefcde30002141757c184174702b3104a029dcd33194f8bd734159deebb2e668716089305b42cb00
c6ca2b85a3e6e73674e210aee4ed69c4af2848e4 validation: do not wipe utxo cache for stats/scans/snapshots (Pieter Wuille)
7099e93d0a80c65a547131d7bab977b09573310c 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 c6ca2b85a3e6e73674e210aee4ed69c4af2848e4
cedwies:
reACK c6ca2b8 (trivial)
achow101:
ACK c6ca2b85a3e6e73674e210aee4ed69c4af2848e4
sedited:
ACK c6ca2b85a3e6e73674e210aee4ed69c4af2848e4
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.
e770392084aa52e5568cf001da4d537fda1d71b3 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 e770392084aa52e5568cf001da4d537fda1d71b3 🐤
achow101:
ACK e770392084aa52e5568cf001da4d537fda1d71b3
fjahr:
Code review ACK e770392084aa52e5568cf001da4d537fda1d71b3
naiyoma:
tACK e770392084aa52e5568cf001da4d537fda1d71b3
Tree-SHA512: ec029d1e1e979f91840af944984cad530a1ce9a0eceb123230817f0ef3b9ad47253eebc4c953d350de2d904b59496fcd4757123c8bd63cf0e09c3581da48fff8
2ee7f9b259059d59e127852ea898b58183604b46 coins: assume `GetCoin` only returns unspent coins (Andrew Toth)
eec551aaf1dff4cccc15e486d5618a8a44d8314c fuzz: keep `coinscache_sim` backend free of spent coins (Andrew Toth)
3e4155fcefe0aafcc9cb84640e303e05477605a3 test: do not return spent coins from `CCoinsViewTest::GetCoin` (Andrew Toth)
ee1e40f58000921e95f08bcb199a452eb5c4d9b2 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 2ee7f9b259059d59e127852ea898b58183604b46
optout21:
crACK 2ee7f9b259059d59e127852ea898b58183604b46
achow101:
ACK 2ee7f9b259059d59e127852ea898b58183604b46
w0xlt:
reACK 2ee7f9b259059d59e127852ea898b58183604b46
Tree-SHA512: be21cc09690410fc04ca25e1ba47aae6186bc037e413b3bb1e6e9a04e6364cbfac5a2fcdc49b638fec848cd29243fab0cc0581b9923f34fafe8366828f690ed4
Verify that addresses announcing themselves (addr == source) are exempt
from time penalties, while addresses announced by others receive the
expected penalty.
14f99cfe53f07280b6f047844fc4fba0da8cd328 rpc: make `uptime` monotonic across NTP jumps (Lőrinc)
a9440b1595be7053b17895f7ee36652bac24be6e 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 14f99cfe53f07280b6f047844fc4fba0da8cd328 🎦
willcl-ark:
tACK 14f99cfe53f07280b6f047844fc4fba0da8cd328
w0xlt:
ACK 14f99cfe53f07280b6f047844fc4fba0da8cd328
sedited:
ACK 14f99cfe53f07280b6f047844fc4fba0da8cd328
Tree-SHA512: 3909973f58666ffa0b784a6df087031b9e34d2022d354900a4dbb6cbe1d36285cd92770ee71350ebf64d6e8ab212d8ff0cd851f7dca1ec46ee2f19b417f53984
a73a3ec5532ddc05c1b013d868d9994f2889c9cf 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 a73a3ec5532ddc05c1b013d868d9994f2889c9cf
maflcko:
review ACK a73a3ec5532ddc05c1b013d868d9994f2889c9cf 🍦
Tree-SHA512: 31177934d645116f381668a0f945028d7e04fab1fc6185dd0e3b7451aab71f89f1e4dd07246db667d1c4734eea3e5d73433c8b0e09181b3ece47dacc8677401e
eeee3755f8c415b227820479b5492261f3a8aa08 fuzz: Return chrono point from ConsumeTime(), Add ConsumeDuration() (MarcoFalke)
faa5a9ebad15fe41e8ddf45f11ad72bdc5aabf99 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 eeee3755f8c415b227820479b5492261f3a8aa08
Crypt-iQ:
crACK eeee3755f8c415b227820479b5492261f3a8aa08
Tree-SHA512: 25dd779a1bf79fa42c6e69db0f0593ad4daa4c0d746e8e82a26bdd65391a27c38e484431056d4e2207b542c511a71cb536c259809728a7166b8d304c0490e321
9a9d797ef6ed8e1b3e876fc93cf1a6395ab270e9 kernel: Add support for block headers (yuvicc)
b851ff6cae71934bf2389d109908339d60ec6e5b 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 9a9d797ef6ed8e1b3e876fc93cf1a6395ab270e9
janb84:
ACK 9a9d797ef6ed8e1b3e876fc93cf1a6395ab270e9
Tree-SHA512: 1dde9ef860543c906d1bb5e604f0d2956e7382fcbb55090686261b2277270a1fd3826f02ecf1749b2774da66e88f686c7845172b4c68b62259e7a7aee0825fa2
1911db8c6dc6b32c8971b14b2b271ec39d9f3ab9 string: add LineReader (Matthew Zipkin)
ee62405cce2bf3d14117bdb327832f12584968d6 time: implement and test RFC1123 timestamp string (Matthew Zipkin)
eea38787b9be99c3f192cb83fc18358397e4ab52 string: add AsciiCaseInsensitive{KeyEqual, Hash} for unordered map (Matthew Zipkin)
4e300df7123a402aef472aaaac30907b18a10c27 string: add `base` argument for ToIntegral to operate on hexadecimal (Matthew Zipkin)
0b0d9125c19c04c1fc19fb127d7639ed9ea39bec Modernize GetBindAddress() (Matthew Zipkin)
a0ca851d26f8a9d819708db06fec2465e9f6228c 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 1911db8c6dc6b32c8971b14b2b271ec39d9f3ab9 👲
furszy:
utACK 1911db8c6dc6b32c8971b14b2b271ec39d9f3ab9
sedited:
ACK 1911db8c6dc6b32c8971b14b2b271ec39d9f3ab9
Tree-SHA512: bb8d3b7b18f158386fd391df6d377c9f5b181051dc258efbf2a896c42e20417a1b0b0d4637671ebd2829f6bc371daa15775625af989c19ef8aee76118660deff
fab2f3df4beb230eef63bdcf5042b6417c0012dc 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 fab2f3df4beb230eef63bdcf5042b6417c0012dc
frankomosh:
re-ACK fab2f3df4beb230eef63bdcf5042b6417c0012dc
Tree-SHA512: 4ecf98ec4adc39f6e014370945fb1598cdd3ceba60f7209b00789ac1164b6d20e82a69d71f8419d9a40d57ee3fea36ef593c47fe48b584b6e8344c44f20a15c1
c9ce1c7c4a12b54ada7d48f100ec3a141ae99f86 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 c9ce1c7c4a12b54ada7d48f100ec3a141ae99f86
darosior:
utACK c9ce1c7c4a12b54ada7d48f100ec3a141ae99f86
Tree-SHA512: 0af1d7b4651478349abc97cf0c009488cf5af5f97135382f7dd37cef0ef9b563192244330899a54ee7e0296bf03ba702e37a7aa15248c5c0ab4745095efc2402
8b9d30e3facff0cd132dc3faf6282d75b1f9b532 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 8b9d30e3facff0cd132dc3faf6282d75b1f9b532
maflcko:
lgtm ACK 8b9d30e3facff0cd132dc3faf6282d75b1f9b532
achow101:
ACK 8b9d30e3facff0cd132dc3faf6282d75b1f9b532
w0xlt:
ACK 8b9d30e3fa
danielabrozzoni:
tACK 8b9d30e3facff0cd132dc3faf6282d75b1f9b532
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>
faa18dceba1dd69703a252f751c233d227164689 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 faa18dceba1dd69703a252f751c233d227164689
fjahr:
Code review ACK faa18dceba1dd69703a252f751c233d227164689
hebasto:
ACK faa18dceba1dd69703a252f751c233d227164689, 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>
faa59b367985648df901bdd7b5bba69ef898ea08 util: Add Expected::swap() (MarcoFalke)
fabb47e4e3dba7c03f9242440cb55eb37b493a7a util: Implement Expected::operator*()&& (MarcoFalke)
fab9721430aa83ddb266aca029e270aec81c021d util: Implement Expected::value()&& and Expected::error()&& (MarcoFalke)
fac48009598611d28b6583559af513c337166aeb util: Add Expected<void, E> specialization (MarcoFalke)
fa6575d6c2d27d173162888226df669fb8aeea47 util: Make Expected::value() throw (MarcoFalke)
fa1de1103fe5d97ddddc9e45286e32751151f859 util: Add Unexpected::error() (MarcoFalke)
faa109f8be7fca125c55ca84e6c0baf414c59ae6 test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK == (MarcoFalke)
fad4a9fe2b8d3a3aa09eca4f47e1741912328785 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 faa59b367985648df901bdd7b5bba69ef898ea08
ryanofsky:
Code review ACK faa59b367985648df901bdd7b5bba69ef898ea08. 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 faa59b367985648df901bdd7b5bba69ef898ea08
Tree-SHA512: b6ac28c1e7241837d9db83fe7534d713ca1283c20a77d2273743157d329f041ec0b503658d14b2f4425211808b61a88fed115d77149e0546825acd3bd9198edf
3dd815f048c80c9a35f01972e0537eb42531aec7 validation: pre-reserve leaves to prevent reallocs with odd vtx count (Lőrinc)
7fd47e0e56087cd3b5fd76a532bdc3ac331e832e bench: make `MerkleRoot` benchmark more representative (Lőrinc)
f0a21831087410687c4ca31ac00e44f380b859be 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 3dd815f048c80c9a35f01972e0537eb42531aec7
optout21:
reACK 3dd815f048c80c9a35f01972e0537eb42531aec7
hodlinator:
re-ACK 3dd815f048c80c9a35f01972e0537eb42531aec7
vasild:
ACK 3dd815f048c80c9a35f01972e0537eb42531aec7
w0xlt:
ACK 3dd815f048c80c9a35f01972e0537eb42531aec7 with minor nits.
danielabrozzoni:
Code review ACK 3dd815f048
Tree-SHA512: e7b578f9deadc0de7d61c062c7f65c5e1d347548ead4a4bb74b056396ad7df3f1c564327edc219670e6e2b2cb51f4e1ccfd4f58dd414aeadf2008d427065c11f
969c840db52da796c319f84c9a9a20b1de902ccf log,blocks: avoid `ComputeTotalSize` and `GetHash` work when logging is disabled (Lőrinc)
babfda332b6aa41143eb694193358ef2c76ebefe log,net: avoid `ComputeTotalSize` when logging is disabled (Lőrinc)
1658b8f82b99f9ba3b42a50ba1f72b19a38c8e75 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 969c840db52da796c319f84c9a9a20b1de902ccf
hodlinator:
re-ACK 969c840db52da796c319f84c9a9a20b1de902ccf
sedited:
Re-ACK 969c840db52da796c319f84c9a9a20b1de902ccf
danielabrozzoni:
reACK 969c840db52da796c319f84c9a9a20b1de902ccf
Tree-SHA512: 9780102d29778165144e3602d934ed4cb96660fd7b9ff2581b223c619e419139b8348e60f226af448702ae527736a1806d169b44342c5a82795590f664e16efe
a3c71c720158fd024d072ad32e78a3b353de0723 [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 a3c71c720158fd024d072ad32e78a3b353de0723
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>
9c7e4771b13d4729fd20ea08b7e2e3209b134fff test: Test listdescs with priv works even with missing priv keys (Novo)
ed945a685473712c1a822379effa42fd49223515 walletrpc: reject listdes with priv key on w-only wallets (Novo)
9e5e9824f11b1b0f9e2a4e28124edbb1616af519 descriptor: ToPrivateString() pass if at least 1 priv key exists (Novo)
5c4db25b61d417a567f152169f4ab21a491afb95 descriptor: refactor ToPrivateString for providers (Novo)
2dc74e3f4e5e6f01c8810359b91041bc6865f1c7 wallet/migration: use HavePrivateKeys in place of ToPrivateString (Novo)
e842eb90bb6db39076a43b010c0c7898d50b8d92 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 9c7e4771b13d4729fd20ea08b7e2e3209b134fff
achow101:
ACK 9c7e4771b13d4729fd20ea08b7e2e3209b134fff
w0xlt:
reACK 9c7e4771b1 with minor nits
rkrux:
re-ACK 9c7e4771b13d4729fd20ea08b7e2e3209b134fff
Tree-SHA512: f9b3b2c3e5425a26e158882e39e82e15b7cb13ffbfb6a5fa2868c79526e9b178fcc3cd88d3e2e286f64819d041f687353780bbcf5a355c63a136fb8179698b60
fabf8d1c5bdb6a944762792cdc762caa6c1a760b fuzz: Restore SendMessages coverage in process_message(s) fuzz targets (MarcoFalke)
fac7fed397f0fa3924600c1806a46d235a62ee5f 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 fa11eea4059a608f591db4469c07a341fd33a031, 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 fabf8d1c5bdb6a944762792cdc762caa6c1a760b
frankomosh:
ACK fabf8d1c5bdb6a944762792cdc762caa6c1a760b
marcofleon:
code review ACK fabf8d1c5bdb6a944762792cdc762caa6c1a760b
sedited:
ACK fabf8d1c5bdb6a944762792cdc762caa6c1a760b
Tree-SHA512: 2e478102b3e928dc7505f00c08d4b9e4f8368407b100bc88f3eb3b82aa6fea5a45bae736c211f5af1551ca0de1a5ffd4a5d196d9473d4c3b87cfed57c9a0b69d
a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43 Fix 11-year-old mis-categorized error code in OP_IF evaluation (Calin Culianu)
Pull request description:
This was introduced by commit ab9edbd6b6eb3efbca11f16fa467c3c0ef905708.
It appears the original author may have gotten tired and pasted the wrong error code into this 1 place. Every other situation where the value stack lacks the required number of arguments for the op-code, SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here.
This commit fixes the situation.
EDIT: Note this turns out to be a dupe of the abandoned #30359 .
ACKs for top commit:
billymcbip:
tACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
achow101:
ACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
darosior:
utACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
sedited:
ACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
Tree-SHA512: e8c01a3e2448b5d49b76a0cab3f38a2d0249b71beeb7d9d05d5ecc3812bd91d0bd1d0f78b809b6f4ccb73186fa119cb1ed3779a73284b83a67ae219ef378fa6c
Transaction hashes are cached, it may not be intuitive that their sizes are actually recalculated every time.
This is done before the other refactors to clarify why we want to avoid calling this method;
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
