💬 hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3595490396)
> > The UCRT CI job is up [now](https://github.com/bitcoin/bitcoin/pull/33764).
>
> You'll have to rebase this one and adjust the ci comments in the ci config files, no?

Thanks! Done.

💬 laanwj commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3595491237)
> Seeing the following difference:

That's such a strange difference. Just register selection.

We should probably aim to create a minimum reproduction, to be able to report and troubleshoot this.

🤔 l0rinc reviewed a pull request: "validation: fetch block inputs on parallel threads >40% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-3523358497)
Redid he measurements on a Mac with AppleClang for different sizes to check why there's such a massive speedup for lowe memory:
```bash
for DBCACHE in 450 4500 45000; do \
COMMITS="d5ed4ba9d8627f1897322ce7eb5b34e08e4f73ac b1a791db1c75a47569b690baf7b074b78e08ca5a"; \
STOP=921129; \
DATA_DIR="$HOME/Library/Application Support/Bitcoin"; LOG_DIR="$HOME/bitcoin-reindex-logs"; \
mkdir -p "$LOG_DIR"; \
COMMA_COMMITS=${COMMITS// /,}; \
(echo ""; echo "$COMMITS" | tr ' ' '\n' | wh
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2575931153)
`ConnectBlock` calls `AccessCoin` for every input, shouldn't the test do the same? Aren't we cheating by only calling it when we already know it's not an internal spend?
```C++
for (const auto& tx : block.vtx) {
if (tx->IsCoinBase()) {
BOOST_CHECK(!cache.GetPossiblySpentCoinFromCache(tx->vin[0].prevout));
} else {
for (const auto& outpoint : tx->vin | std::views::transform(&CTxIn::prevout)) {
const auto external{!txids.contains(o
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2576139456)
we're only using `100` here, we might as well:

```suggestion
static constexpr auto num_txs{100};
CBlock CreateBlock() const noexcept
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2575987031)
why are coinbases and internal spends added to the cache, that's not what happens in reality, right?
It should represent the state before the block is connected, and it should populate backed and db caches as well, so maybe something like:

```suggestion
void PopulateCache(const CBlock& block, CCoinsView& view, bool spent = false)
{
CCoinsViewCache cache{&view};
cache.SetBestBlock(uint256::ONE);

std::unordered_set<Txid, SaltedTxidHasher> txids{};
txids.reserve(block.vt
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2576160496)
we should do *something* with the result:

```suggestion
for (const auto& in : tx->vin) {
const auto& c{view.AccessCoin(in.prevout)};
BOOST_CHECK(c.IsSpent());
}
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2576140637)
seems unused

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2576143396)
we could add comments for these cases as well:
```C++
if (i % 3 == 0) {
// External input
txid = Txid::FromUint256(uint256(i));
} else if (i % 3 == 1) {
// Internal spend (prev tx)
txid = prevhash;
} else {
// Test shortid collisions (looks internal, but is external)
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2576158123)
we could also add an in-memory leveldb here to simplify the code and make it more realistic:
```suggestion
CCoinsViewDB db{DBParams{.path = "", .cache_bytes = 1_MiB, .memory_only = true, .wipe_data = true}, CoinsViewOptions{}};
CCoinsViewCache main_cache{&db};
CoinsViewCacheAsync view{main_cache, db};
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2575797791)
I prefer simple but fully functioning chunks that converge towards a feature (as someone illustrated: "skateboard -> bicycle -> scooter -> motorcycle -> car" instead of "left wheels -> right wheels -> doors -> wipers -> radio antenna -> windows -> etc").
So if we can carve out chunks (such as the internal spend guard, or a single-threaded fetcher first), we could guide the reviewer instead of having a big-bang change that's really hard to fully comprehend as such.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2575840858)
wouldn't that percolate to the database layer?

✅ l0rinc closed an issue: "Startup crash on macOS with GCC 15.2: `std::source_location::file_name()` is `nullptr`"
(https://github.com/bitcoin/bitcoin/issues/33964)

💬 l0rinc commented on issue "Startup crash on macOS with GCC 15.2: `std::source_location::file_name()` is `nullptr`":
(https://github.com/bitcoin/bitcoin/issues/33964#issuecomment-3595607653)
Not sure how to create a reliable reproducer for this, closing for now

🚀 fanquake merged a pull request: "cmake: Set `WITH_ZMQ` to `ON` in Windows presets"
(https://github.com/bitcoin/bitcoin/pull/33971)

⚠️ maflcko opened an issue: "integer overflow in FUZZ=package_rbf"
(https://github.com/bitcoin/bitcoin/issues/33981)
Oss-Fuzz: https://issues.oss-fuzz.com/issues/464820824

Input: https://github.com/user-attachments/files/23850907/clusterfuzz-testcase-minimized-package_rbf-5766839826448384.bin.not.txt

Reproduce:

`UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=package_rbf ./bld-cmake/bin/fuzz -runs=1 /tmp/tmp.hLXaXiiFIP/clusterfuzz-testcase-minimized-package_rbf-5766839826448384.bin.not.txt`

```
src/util/feefrac.h:128:14: runt
...

💬 l0rinc commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3595752609)
Thanks for the review @laanwj and @vasild.

> Results look like there is no difference between baseline and PR. Some small differences, within noise.

Your benchmarking machine had quite the fluctuation, I usually try to stabilize the system before measuring these, see:
https://github.com/bitcoin/bitcoin/blob/fe434a469534766f18d7560d968deed37193835f/src/bench/nanobench.h#L2078

Plotting the result you posted for Clang:
<img width="1473" height="791" alt="image" src="https://github.com/us
...

💬 hodlinator commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2576533841)
> `ComputeMerkleRoot` is usually called without mutation checks

That is true for witness-version of the roots but we still perform the checks for the non-witness root. Every time net_processing receives a regular block message (unless we are loading blocks), we check whether the block is mutated:

https://github.com/bitcoin/bitcoin/blob/fa283d28e261416f0785450ab687af199103776a/src/net_processing.cpp#L4647

`IsBlockMutated()` always calls `CheckMerkleRoot()` which always calls `BlockMerkle
...

💬 l0rinc commented on pull request "script: Remove dead code from OP_CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33977#issuecomment-3595815575)
Code review ACK 53ca0dc3cfdad8c9fd994c3758a477e319379db1

The lines are safe to remove since they're side-effect free and the mentioned extra element is included in the initial stack size validation while the cleanup loop stops before popping it.
The comment above it likely doesn't need updating since it's seems to refer to the line below.
The `SCRIPT_ERR_INVALID_STACK_OPERATION` script error is used elsewhere, so we don't need to clean that up either.

🚀 fanquake merged a pull request: "cmake: Make `BUILD_KERNEL_TEST` depend on `BUILD_KERNEL_LIB`"
(https://github.com/bitcoin/bitcoin/pull/33972)