💬 vasild commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-3497705768)
I re-assessed this carefully.

It seems ok to me to have v2only with the following properties:

* Can only be enabled if not listening, like suggested above. Otherwise we have to either:
* accept v1 incoming which defeats the purpose of this because then we will have cleartext connections or
* deny v1 incoming (only allow v2 incoming) which is a problem if this is widely adopted because then old nodes that do not support v2 will have a hard time finding peers to connect to and will b
...

👍 l0rinc approved a pull request: "test: Add test on skip heights in CBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/33661#pullrequestreview-3428711246)
ACK ea998e18f6d64f485d4e2d85a1028474fb35120a

Thanks for taking my suggestion, the code is very clean and easy to understand now.
I left some nits I still saw, but they're not blocking.

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2499361757)
nit: below we're using brace init + dividing a `double` by a `size_t` is also double-division (untested):
```suggestion
const auto avg{double(std::accumulate(skip_diffs.begin(), skip_diffs.end(), 0)) / skip_diffs.size()};
```

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2499366014)
nit: redundant comment

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2499367082)
nit: redudnant comment, the code already expresses this clearly

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2499375298)
nit: should likely be a `BOOST_REQUIRE` since this is still the setup page, not yet the validation stage

💬 maflcko commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#issuecomment-3497856061)
> Profiling the performance regression in [#33618 (comment)](https://github.com/bitcoin/bitcoin/issues/33618#issuecomment-3399937858) revealed that `CBlockIndexWorkComparator` and its underlying `base_uint<256u>::CompareTo` are hot paths during block validation, consuming ~4% of CPU time.

I don't follow here. The issue is about the additional `CWallet::WriteBestBlock` overhead? `CBlockIndexWorkComparator` seems unrelated to the issue, at least I can't see how it changed the result between 29.
...

💬 ismaelsadeeq commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3497856893)
_Apologies for the force pushes, I am attempting to fix a C.I failure which happens only remotely due to `SetMockTime` not able to nudge the node clock in ms, I adjusted the time advances to be in seconds which a large buffer_

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499419184)
when can this be true?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499427113)
nit: trailing newline shouldn't be needed anymore

```suggestion
LogPrintLevel(BCLog::VALIDATION, BCLog::Level::Warning, "InputFetcher failed to fetch input: %s.", e.what());
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499420838)
```suggestion
if (std::ranges::binary_search(m_txids, input.outpoint.hash.ToUint256().GetUint64(0))) {
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499430732)
```suggestion
std::ranges::sort(m_txids);
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499446404)
not yet sure I fully understand why this is needed - it's more complex, but does it result in at least a theoretic speedup?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499464047)
should we document here what happens in case of a cache miss or collision?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499449442)
`continue` skipping a single line is a bit confusing an dangerous - it should only skip the next line:
```suggestion
if (!input.coin.IsSpent()) {
temp_cache.EmplaceCoinInternalDANGER(COutPoint{input.outpoint}, std::move(input.coin));
}
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499442182)
based on https://en.cppreference.com/w/cpp/atomic/atomic_flag/wait.html
```suggestion
input.ready.wait(/*old*/false, std::memory_order_acquire);
```

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2499486476)
this will fix the linter failure as well (whitespace after `*`):
```suggestion
*
* @return whether there are more inputs in the queue to fetch
```

💬 jurraca commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3497961898)
ACK 44717da29b626bfdcb20a32318689d74c4113b7b

I think it's a better pattern to force users to specify the filename, but I can't tell whether users currently depend on the default.

💬 sipa commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3498015913)
I'm neutral on this PR.

I do use the `asmap.dat` loading by default, but (a) don't care about changing that to specify it explicitly and (b) I hope #28794 happens soonish so it becomes a non-issue (as I will switch to built-in asmap files rather than explicit ones in the datadir).

💬 maflcko commented on pull request "script: remove dead code in `CountWitnessSigOps`":
(https://github.com/bitcoin/bitcoin/pull/33786#issuecomment-3498072962)
review ACK 24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7 🐏

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 24bcad3d4df5
...