💬 jonatack commented on pull request "test: chain reorg for coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/31885#issuecomment-2676300620)
Didn't compare coverage or look further, but this unit test runs near instantly on my laptop:

```
$ time ./build/src/test/test_bitcoin --run_test=coinstatsindex_tests/coinstatsindex_reorg
./build/src/test/test_bitcoin 0.14s user 0.03s system 62% cpu 0.265 total
```

or the whole file

```
$ time ./build/src/test/test_bitcoin --run_test=coinstatsindex_tests
Running 3 test cases...

*** No errors detected
./build/src/test/test_bitcoin --run_test=coinstatsindex_tests 0.18s user 0.
...

🤔 jonatack reviewed a pull request: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260#pullrequestreview-2635042631)
Concept ACK

💬 jonatack commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1966558267)
The existing `ChainTypeToString()` in the same file above seems to be already invoked for displaying user-facing messages. Perhaps simpler to use it rather than adding this new one.

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1966566569)
Yes, that would be another resolution that makes sense, not 100% sure what -upnp=0 should do in that case, Should that still set the default for -natpmp to 0?
This is something to discuss seperately, i've just removed the TODO.

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2676322495)
Updated for @davidgumberg @maflcko @ryanofsky's comments.

💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966566982)
If I'm understanding the feedback, it comes from the idea of C structs being for POD. My understanding is that in C++, classes and structs are identical except for their default inheritance and member access levels, and for the cases here, structs seem to be the appropriate choice that allows simpler code. See also https://stackoverflow.com/a/46339933. I don't mind dropping this change if there is a compelling reason that I'm not aware of, though.

💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966567194)
Thanks for having a look! Replied below in https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966566982.

💬 jonatack commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#issuecomment-2676325081)
Concept ACK

💬 jonatack commented on issue "Unable to generate coverage report using lcov on MacOs 15.3.1":
(https://github.com/bitcoin/bitcoin/issues/31927#issuecomment-2676341730)
Reproduced the issue on macOS 15.3.1 arm64 m1 max.

🤔 jonatack reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2635167294)
Concept ACK

💬 jonatack commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1966580329)
Following these instructions, this command failed for me with

`error: coverage.profdata: could not read profile data!No such file or directory`.

I then ran `rm -rf ./build/` and re-ran the instructions. Now seeing:

`warning: 2 functions have mismatched data`.

Haven't looked further yet.

I usually build with both `rm -rf ./build/` and `-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++"`, among other flags, so I may need to
...

💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966592114)
> It seems odd to treat -getinfo different than -netinfo (and everything else). It would be good for consistency to treat them the same

Thanks, done.

💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966593211)
Thank you for taking a look! `m_details_level` is capped at a maximum value of 4 since https://github.com/bitcoin/bitcoin/pull/21192/files; that line is currently `m_details_level = std::min(n, NETINFO_MAX_LEVEL);`

💬 Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1966624594)
The first error never occurs if I always remove my `build` folder and then proceeding with the commands specified.

For MacOs , I have llvm 19 which is also a known [issue](https://github.com/bitcoin/bitcoin/issues/31049) so I generally have to specify my commands with this to use llvm 18.

```
cmake -B build \
-DCMAKE_C_COMPILER="$(brew --prefix llvm@18)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm@18)/bin/clang++" \
-DCMAKE_C_FLAGS="-fprofile-instr-generate -fcover
...

💬 yancyribbens commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966625232)
I think you misunderstood my comment (or I am not understanding yours). min provides both an upper an lower bound here, so it's just bounded. In the commit you say "upper bound" but I'm saying it should just read "bound".

💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966627276)
> If I'm understanding the feedback, it comes from the idea of C structs being for POD. My understanding is that in C++, classes and structs are identical except for their default inheritance and member access levels, and for the cases here, structs seem to be the appropriate choice that allows simpler code.

I agree that C++ classes and structs are essentially the same, no confusion there. (C# disallows inheritance for `struct` but not `class`, Rust doesn't allow inheritance, period. I know w
...

💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966623051)
I consider mixing `class`/`struct` in the inheritance hierarchy to be a separate issue.

💬 laanwj commented on pull request "net: update comment for service bit support info for seed.bitcoin.sipa.be":
(https://github.com/bitcoin/bitcoin/pull/29809#issuecomment-2676766227)
imo we should close this, it has been open way too long for a simple comment change, and the place to track the status and capabilities of external services is not the source repository

✅ fanquake closed a pull request: "net: update comment for service bit support info for seed.bitcoin.sipa.be"
(https://github.com/bitcoin/bitcoin/pull/29809)

💬 arejula27 commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#issuecomment-2676854804)
Concept ACK [`a63c5c1a8a012b694`](https://github.com/bitcoin/bitcoin/pull/31917/commits/a63c5c1a8a012b694b8e260fd5d2fda3c8785e98)

I’ve reviewed the changes and have one main observation:

## 1. Fuzz Test Limitations

Limiting fuzz to the most common sizes makes sense for efficiency. However, if long strings are still expected to be supported, there should be explicit test coverage for them.

I checked the unit tests for Base58 in `src/test/base58_tests.cpp`, which reads test cases from
...