💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3647326460)
I have rewritten `SpanningForestState::UpdateChunk` to no longer traverse the chunk by walking dependencies, but by looping over all dependencies of the chunk directly, and using the `top_setinfo` to figure out which ones to update. It's a few percent faster, simpler, and additionally allowed dropping the tracking of parent dependencies of a transaction entirely.

👍 instagibbs approved a pull request: "fuzz: Fix bugs in `clusterlin_postlinearize_tree` target"
(https://github.com/bitcoin/bitcoin/pull/34061#pullrequestreview-3572724599)
ACK a70a14a3f4f4d66025302c00eb4ff8108835cc5d

💬 davidgumberg commented on pull request "doc: guix: Troubleshooting zdiff3 issue and uninstalling.":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2614933849)
The mistake I made was that `or running...` should be inside of the parentheses. The items in the parentheses don't "need" to be parallel with "Uninstall" even if you respect the parallel structure device as a rule (which it isn't.)

I moved the guix install script instructions inside the parentheses.

💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#issuecomment-3647464529)
@sedited @l0rinc thank you both for your reviews! I rebased and updated with your suggestions.

🤔 l0rinc requested changes to a pull request: "test: Add test on skip heights in CBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/33661#pullrequestreview-3572005492)
I might have gone a bit overboard, but I have tried explaining every change that I suggested in context.

For convenience, here's the full local diff I did during review (this is the latest state, some of the suggestions may reflect a previous one):
```C++
BOOST_AUTO_TEST_CASE(get_skip_height_test)
{
// Even heights: clear the lowest set bit.
BOOST_CHECK_EQUAL(GetSkipHeight(0b00010010),
0b00010000);
BOOST_CHECK_EQUAL(GetSkipHeight(0b00100010),

...

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614369333)
Could unify these to `BOOST_REQUIRE` instead of `assert`

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614388593)
can you use brace init consistently to avoid narrowing? And corecheck suggests const reference - though we can likely inline it instead...

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614370366)
return value isn't needed, could be simplified to:

```suggestion
--heightWalk;
```

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614371127)
```suggestion
BOOST_REQUIRE_GT(delta, 0);
```

And if we keep this, it could go closer to the `delta` declaration

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614455685)
why 100 and why 5%? The numbers are already visible in the code, but not sure why - can we have that in the comment instead?

The 5% doesn't seem to hold for non-deterministic random - seems arbitrary.
Neither does the `100` - so my suspicion seems to have been accurate that these aren't real properties of the system, just accidental ones of the sample.

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614448211)
nit: we could reduce the scope of the iteration-only variables:
```C++
int iteration_count{0};
for (int height_walk{start_height}; height_walk > target_height; ++iteration_count) {
```

nit: `heightWalk` -> `height_walk`

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614428365)
This may be useful during development, but it's basically testing the testing logic itself, isn't it? If you keep them, can you add them after the initializations to have separate init and check sections, something like:
```C++
const int start_height{(chain_size - 1) - ctx.randrange(chain_size / 2)}; // pick a height in the second half of the chain
const int delta{min_distance + ctx.randrange(start_height - min_distance + 1)}; // pick a target height, earlier by at least min_distance
const
...

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614408109)
I think you mean that the second and third **set** bits are zerod, right?
```C++
// Even heights: clear the lowest set bit.
...
// Odd heights: clear the 2nd and 3rd set bits.
```

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614473206)
Given that we've already called `BuildSkip`, do we need to test this directly or can we do it indirectly as well?

```suggestion
const int height_skip{pindexWalk->pskip->nHeight};
const int height_skip_prev{pindexWalk->pprev->pskip->nHeight};
```

💬 Sjors commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#issuecomment-3647551260)
@cobratbq there's certainly no need to sign commits, but when people do sign, it's nice that we don't break the signature.

💬 Sjors commented on pull request "doc: improvements to doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/pull/33986#discussion_r2615124857)
Looks like this one only has a markdown version, link gives a 404.

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615178895)
> The problem is that in the rare case where the connection does not conclude with `PING`

I don't think this is a rare case at all. It happens frequently where a connection is aborted early e.g. during handshake. In these cases we won't have a tx either, so the proposed solution would not open a new connection either and make private broadcast be less reliable.

I think we should leave this as is. The race condition that is described here is actually the rare occurrence. I did not experienc
...

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3647697081)
> `PR` performs better with `100 mb` dbcache than `master` with `7 GB`;

wat

💬 pablomartin4btc commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r2615258470)
tiny nit (not a blocker):
```suggestion
getaddrmaninfo RPC internally. Users querying older, unmaintained node versions would need to use an older bitcoin-cli version. (#26988)
```

🤔 pablomartin4btc reviewed a pull request: "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-3573191027)
ACK 325f98bca07c3a0c48a46dc18f67376d44c5341d

_(Left a small [suggestion](https://github.com/bitcoin/bitcoin/pull/26988/files#r2615258470) since the older node may be operated by someone other than the CLI user, and upgrading it might not be possible or appropriate when this change is the only reason. I think this would also align with @jonatack’s earlier [note](https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-3559469161).)_

💬 Raimo33 commented on pull request "streams: replace `std::find` with `memchr` (5x improvement)":
(https://github.com/bitcoin/bitcoin/pull/34044#issuecomment-3647753086)
> I would be curious whether a `-reindex -stopafterblockimport` speedup is measurable - since my understanding is that the magic is usually at the beginning anyway, so it's also possible this ends up slowing down the average case.

I've ran a reindex benchmark, see updated PR description. I'm not sure if ~1.2% is an irrelevant gain, but I argue a 5-6x improvement on the `FindByte` method is significant and should be considered for merging regardless, even for possible future use of this method
...