💬 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.

📝 maflcko opened a pull request: "test: Use extra_port() helper in feature_bind_extra.py"
(https://github.com/bitcoin/bitcoin/pull/33260)
This is a refactor for self-validating and self-documenting code.

Currently, the test assumes that extra ports are available and just increments them without checking. However, this may not be the case when the test is modified to use more ports. In this case, the tests may fail intermittently and the failure is hard to debug.

Fix this confusion, by calling `p2p_port` each time. This ensures the required `assert n <= MAX_NODES` is checked each time.

Closes https://github.com/bitcoin/bit
...