💬 l0rinc commented on pull request "bench: Add more realistic Coin Selection Bench":
(https://github.com/bitcoin/bitcoin/pull/33160#discussion_r2302156688)
I'd avoid the `Narrow No-Break Space` chars from docs if possible, they're rendered differently on different mediums

💬 l0rinc commented on pull request "bench: Add more realistic Coin Selection Bench":
(https://github.com/bitcoin/bitcoin/pull/33160#discussion_r2302174002)
Please rebase, `spendable` was removed since: https://github.com/bitcoin/bitcoin/commit/6a7aa015747e2634fe5a4b2f7fa0d104eb75c796#diff-38f1a8db124a979cb6dd76ce263f7aae0053d6967ee909e6356115fa0402dc8cL78

💬 l0rinc commented on pull request "bench: Add more realistic Coin Selection Bench":
(https://github.com/bitcoin/bitcoin/pull/33160#discussion_r2302160133)
Same, seems simpler to just leave it to deterministic instead of hard-coding a confusing seed
```suggestion
FastRandomContext det_rand{/*fDeterministic=*/true};
```

note: can we reuse this for the random source below as well?

💬 l0rinc commented on pull request "bench: Add more realistic Coin Selection Bench":
(https://github.com/bitcoin/bitcoin/pull/33160#discussion_r2302166150)
we could dedup considerable here for better signal-to-noise ratio - it's a lot of work to find the differences between the values:
```C++
const auto txout{wtx->tx->vout.at(0)};
const int input_bytes{CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr)};
const COutput output{(COutPoint{wtx->GetHash(), 0}), txout, /*depth=*/6 * 24, input_bytes, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/0};
if (int y{rng.randrange(100)}; y < 35) {

...

💬 l0rinc commented on pull request "bench: Add more realistic Coin Selection Bench":
(https://github.com/bitcoin/bitcoin/pull/33160#discussion_r2302187694)
nit: since you're also mentioning the minimum in the comment first, we might as well put the fixed size first here
```suggestion
targets.push_back(10'000'000 + det_rand.randrange(90'000'000));
```

Alternatively, to avoid all the 0s, consider:
```suggestion
targets.push_back(COIN / 10 + rng.randrange(COIN / 9 * 10));
```
This would unify it with the rest of the usages.

---

this has come up multiple times, we should really add a `randrange` helper with min/max value
...

💬 l0rinc commented on pull request "bench: Add more realistic Coin Selection Bench":
(https://github.com/bitcoin/bitcoin/pull/33160#discussion_r2302195389)
`nInput` should be `uint32_t` otherwise we'd have a narrowing conversion in `OutPoint{tx.GetHash(), nInput}`

👍 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');
```