💬 Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586114315)
nit: I think the `it->nHeight > 0` branch is unnecessary? If `nHeight` is 0 and we are here, then that means the fork point was before the genesis block which should be impossible?

💬 Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586270472)
Would it be a good idea to also call `InvalidChainFound(to_connect.front())` similar to [here](https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/validation.cpp#L3289)?

💬 Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586804372)
Should this be 2 instead? See [here](https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/validation.h#L1058).

💬 Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2589659925)
I hacked together a patch that does this, it hasn't found anything. Feel free to use if you want:

<details>
<summary> diff </summary>

```diff

diff --git a/src/test/fuzz/block_index_tree.cpp b/src/test/fuzz/block_index_tree.cpp
index 037d168e0e..a8c0dc98cf 100644
--- a/src/test/fuzz/block_index_tree.cpp
+++ b/src/test/fuzz/block_index_tree.cpp
@@ -49,6 +49,8 @@ FUZZ_TARGET(block_index_tree, .init = initialize_block_index_tree)
blocks.push_back(genesis);
bool abort_run{fa
...

💬 Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586279490)
(Just a clarifying question)

Is this true of ABC? From my reading, this is true of ABCStep but not ABC since ABC will stop (besides failure or interrupt) only when our tip is the block with most work (see: [here](https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/validation.cpp#L3519)).

📝 wnqqnw19 opened a pull request: "Wnqqnw19"
(https://github.com/bitcoin/bitcoin/pull/34007)
```
## Changes Made

1. **Renamed `format` parameter to `format_version`** (line 18)
- Avoids shadowing Python's built-in `format()` function
- Improves code clarity and follows Python best practices

2. **Removed redundant local assignment of `ADDRMAN_NEW_BUCKET_COUNT`** (line 35)
- Now uses the imported constant from `test_framework.netutil` directly
- Ensures consistency with the centralized constant definition
- If the constant changes in the future, this code will a
...

✅ fanquake closed a pull request: "Wnqqnw19"
(https://github.com/bitcoin/bitcoin/pull/34007)

💬 purpleKarrot commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3613330088)
> Some low-level code could benefit from being able to use `std::expected` from C++23.

Can you elaborate? How would it benefit? Over what alternative?

👍 ryanofsky approved a pull request: "mining: getCoinbase() returns struct instead of raw tx"
(https://github.com/bitcoin/bitcoin/pull/33819#pullrequestreview-3541130613)
Code review ACK 5761dac2f5481c27550483279a007c2b850bc990, with the two suggestions applied since last review.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2589928992)
> Why not simply call `NumToOpenAdd(num_for_rebroadcast)`

This would be the change:

<details>
<summary>Use only num_for_rebroadcast</summary>

```diff
void PeerManagerImpl::ReattemptPrivateBroadcast(CScheduler& scheduler)
{
- // The following heuristic is subject to races, but that is ok: if it overshoots,
- // we will open some private connections in vain, if it undershoots, the stale
- // transactions will be picked on the next run.
-
- size_t active_connections{0
...

👍 sedited approved a pull request: "depends: update freetype and document remaining `bitcoin-qt` runtime libs"
(https://github.com/bitcoin/bitcoin/pull/33952#pullrequestreview-3541144045)
Guix build:
```
760ca6bb241b02b496785a0123cea4e9e49e03e27e2280824237bd4125ffb1d0 guix-build-41e657aacfa6/output/aarch64-linux-gnu/SHA256SUMS.part
432c63a2879ef00793c9378f64d5130752258c0d845b565c4fa7c708399be01a guix-build-41e657aacfa6/output/aarch64-linux-gnu/bitcoin-41e657aacfa6-aarch64-linux-gnu-debug.tar.gz
15c65f8720598ff04dba8d15b0b49e5ab9064f6798159b331681806a62f28426 guix-build-41e657aacfa6/output/aarch64-linux-gnu/bitcoin-41e657aacfa6-aarch64-linux-gnu.tar.gz
d53c1ef1fd1501f0452d
...

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3613425769)
> > Some low-level code could benefit from being able to use `std::expected` from C++23.
>
> Can you elaborate? How would it benefit? Over what alternative?

Sure. The alternatives were listed in doc diff in the commit, but I've pulled it out and put in the pull description as well.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2589984428)
Marking as resolved, checked with @andrewtoth on IRC, thanks!

🤔 jaonoctus reviewed a pull request: "init: point out -stopatheight may be imprecise"
(https://github.com/bitcoin/bitcoin/pull/33993#pullrequestreview-3541279438)
re-ACK ff06e2468a5d3eeebeffe781904c34c9d1b44385

💬 hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2590030400)
Thanks! Reworked.

📝 0xB10C opened a pull request: "log: don't rate-limit "new peer" with -debug=net"
(https://github.com/bitcoin/bitcoin/pull/34008)
Previously, when `debug=net` is enabled, we log "New [..] peer connected" for new inbound peers with `LogPrintf`. However, `LogPrintf` will get rate-limited since https://github.com/bitcoin/bitcoin/pull/32604. When we specifically turn on `debug=net`, we don't want these log messages to be rate-limited.

To fix this, use `LogDebug` if `debug=net` is enabled. Otherwise use `LogPrintf`. This means we don't rate-limit the message with `debug=net` (due to `LogDebug` not being rate-limited) but sti
...

💬 0xB10C commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2590031496)
I'm not super happy with this approach of duplicating the log statement, but I think it's the clearest to a reader. If someone feels strongly, I'm very happy to change it to something better! There are two alternatives that came to mind:

1. Use a macro to avoid duplicating the log statements: Not sure if we want to define a macro extra for this, but could be done. This was my initial approach I dismissed.
2. Extract the log message string and format it with e.g. `std::format()` first, then pass
...

💬 hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3613552394)
The [feedback](https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2550408178) from @fanquake has been addressed.

💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#issuecomment-3613627392)
> what do you think about wrapping all the code in `db_key.h` under a namespace like "util", "common", "index::util", or "util::index" (just threw some names), so it's clear when reading the code that these functions are defined elsewhere and shared, without needing to search around for the definition.

I did that in 5646e6c0d3581f12419913b88745f51c7a3161b9 - let me know what you think. I used `index_util`, turned out it was required a bit more often than I would have expected.

💬 ajtowns commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2590075362)
Perhaps

```c++
auto mapped_as_info = [&]() -> std::string {
const auto mapped_as{m_connman.GetMappedAS(pfrom.addr);
return (mapped_as ? strprintf(", mapped_as=%d", mapped_as) : "");
};
if (!pfrom.IsInboundconn()) {
LogInfo("New %s %s peer ...", ..., mapped_as_info());
} else {
LogDebug(BCLog::NET, "New %s %s ...", ..., mapped_as_info());
}
```