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

🚀 fanquake merged a pull request: "[29.x] backport #33212"
(https://github.com/bitcoin/bitcoin/pull/33251)

📝 fanquake opened a pull request: "ci: return to using dash in CentOS job"
(https://github.com/bitcoin/bitcoin/pull/33261)
`dash` is available again: https://bugzilla.redhat.com/show_bug.cgi?id=2335416.

💬 maflcko commented on issue "tracing: test `interface_usdt_net.py` fails due to garbage message type in `net:outbound_message` tracepoint":
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3227642428)
I can reproduce via `-DAPPEND_CXXFLAGS='-O1 -g2'`, but it passes via `-DAPPEND_CXXFLAGS='-O1 -fno-inline -g2'`, so I wonder if this is a compiler error, or if the error is something else.

👍 maflcko approved a pull request: "ci: return to using dash in CentOS job"
(https://github.com/bitcoin/bitcoin/pull/33261#pullrequestreview-3159280776)
lgtm. Either shell should be fine here, for the purpose to test a non-bash shell.