👍 davidgumberg approved a pull request: "Revert compact block cache inefficiencies"
(https://github.com/bitcoin/bitcoin/pull/33253#pullrequestreview-3157177236)
crACK b7b249d3adfbd3c

This PR reverts two performance regressions that broke cache locality of the wtxid's we need to hash during compact block reconstruction and it adds a nice benchmark that demonstrates the regressions and might have caught them before they were merged.

For more context: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2025-08-23#1755938803-1755950270;

I reviewed the code changes here directly, and I also compared them to the commits they claimed to be reverting, a
...

💬 davidgumberg commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2302199451)
+1

💬 davidgumberg commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2302369266)
nit:

```suggestion
bench.unit("block").run([&] {
```

and maybe:

```suggestion
bench.unit("block").timeUnit(1ms, "ms").run([&] {
```

💬 davidgumberg commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2302045141)
What makes this section not performance critical, is that we're likely going to have to do a full round-trip anyways to get the collided transactions, but I still think the suggestion is more correct/consistent, so might be nice to do if retouching. Out of scope, but we can also remove the first clause of the `if` check entirely, since it's checking the inverse of the `if` block above.

💬 ajtowns commented on pull request "refactor: Use typesafe Wtxid in compact block encodings":
(https://github.com/bitcoin/bitcoin/pull/29752#issuecomment-3226578153)
(`vExtraTxnForCompact` are additional txs to what's in the mempool; `CTxMemPool::txns_randomized` is what could be removed if there wasn't a performance regression)

💬 yuvicc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2302809114)
I guess the first err msg is from logging while the second is from `init.cpp`. From the comment https://github.com/bitcoin/bitcoin/pull/33231#issuecomment-3222458339, I can see that we would want to stop the execution.

💬 yuvicc commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#issuecomment-3226778128)
Concept ACK

💬 maflcko commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#issuecomment-3226815985)
> the relevant bottleneck for me (tested on Ubuntu and Debian) was not `ARG_MAX`, but `MAX_ARG_STRLEN`, which accounts for the maximum per arg, not the sum.

Thanks for clarifying that this was intentionally picked and that `MAX_ARG_STRLEN` is a limit per arg, separate from the command size limit `SC_ARG_MAX`. `SC_ARG_MAX` is larger on many modern systems, but not all. Also, it reduces to `MAX_ARG_STRLEN`, or even lower on some systems. I've edited the pull description, to better reflect this.
...

🤔 hodlinator reviewed a pull request: "index: store per-block transaction locations for efficient lookups"
(https://github.com/bitcoin/bitcoin/pull/32541#pullrequestreview-3155477609)
Reviewed 4441827ef4e9b5fe306c5f0a81a52b5d2b5e0b69

Sorry for the bag of nits, take from it what you will. The questions have higher priority for sure. Tried adding coverage of `LocationsIndex` to *test/functional/feature_init.py* but ran into interference with `TxIndex`, could be some more general issue though. Haven't had time to uncover the reason yet.

Suggestions+test branch:
https://github.com/bitcoin/bitcoin/compare/4441827ef4e9b5fe306c5f0a81a52b5d2b5e0b69...hodlinator:bitcoin:pr/3254
...

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2300885992)
nit: Unused: clientversion.h

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2300903099)
nit: snake_case and narrower types:
```suggestion
Assume(block.data->vtx.size() <= std::numeric_limits<uint32_t>::max());
const uint32_t tx_count{static_cast<uint32_t>(block.data->vtx.size())};
uint32_t tx_offset{HEADER_SIZE + GetSizeOfCompactSize(tx_count)};
```

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2300949481)
Don't see a good way around zeroing the memory, but comment makes it sound like a good thing?
I guess it's slightly better than returning a vector containing garbage upon failure, but even better would be to clear it upon failure?

nit: Could change:
```diff
- bool ReadRawTransaction(const uint256& block_hash, size_t i, std::vector<std::byte>& out) const;
+ std::vector<std::byte> ReadRawTransaction(const uint256& block_hash, uint32_t i) const;
```
And only return non-empty vector o
...

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2301676647)
nit:
```suggestion
std::string strJSON = objTx.write() + '\n';
```

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2301676036)
nit: Might as well use the fact that it's only 1 char:
```suggestion
strHex.push_back('\n');
```

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2301710958)
2 nits:
1. Could reword and elaborate
```suggestion
* LocationsIndex is used to store and retrieve the location of transactions within a block.
*
* This is done in a compressed manner, allowing the whole index to fit in RAM on decent hardware.
* For example, the on-disk size was <X>GiB at block height <Y>.
```
2. Could clarify PR description unless I'm mistaken.
```diff
-allowing it to be cached
+allowing it to be cached in RAM
```

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2300933020)
nit: Could make `row` `const` as well and nail down the types:
```suggestion
bool LocationsIndex::ReadRawTransaction(const uint256& block_hash, uint32_t i, std::vector<std::byte>& out) const
{
const uint32_t row{i / TXS_PER_ROW}; // used to find the correct DB row
const uint32_t column{i % TXS_PER_ROW}; // index within a single DB row
```

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2301692085)
nit: Since we already have `index` and `locations_index` in scope here, could call this `block`, or `block_index`?
```suggestion
const CBlockIndex* block{chainman.m_blockman.LookupBlockIndex(*block_hash)};
```

💬 hodlinator commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2301697459)
nit: Should use snake_case as per developer-notes.md:
* `uri_parts`
* `str_hex`
* `obj_tx`
* `str_JSON`

💬 Sjors commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3227126023)
I tried if f474673fed789257b2a9e9e5aabe6e734128b00a fixes #32759. It doesn't, but that's probably not expected since it just removes an implicit dependency.

To update the time machine, I had to manually `guix download` one item to build with `--no-substitutes`:
- `guix download https://downloads.sourceforge.net/project/boost/boost/1.83.0/boost_1_83_0.tar.bz2`

Still in the process of updating the time machine x86_64 which is taking quite long. Will publish hashes when that's done and also
...

💬 hodlinator commented on pull request "clang-format: align brace-after-struct and *-class formatting":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2303151367)
Why change the C++ Coding Style? I got used to the current one, which encourages `struct`s for POD as a separate thing.