💬 0xB10C commented on issue "ci: failure in interface_usdt_utxocache.py":
(https://github.com/bitcoin/bitcoin/issues/31951#issuecomment-2683691233)
Based on the logs just before the assert:

```log
TestFramework (INFO): handle_utxocache_add(): UTXOCacheChange(outpoint=d09ca39938b434b19a3338ef6b410071d85d4450dd7dc1f7db1bf7d95a53709a:0, height=201, value=4999968800sat, is_coinbase=False)
TestFramework (INFO): handle_utxocache_add(): UTXOCacheChange(outpoint=23b3dfef15c9b64445ffd44920adc48e7746aff1f03b59db3ed71936e90ff201:0, height=201, value=2500031200sat, is_coinbase=True)
TestFramework (INFO): handle_utxocache_spent(): UTXOCacheChange(out
...

💬 andremralves commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#issuecomment-2683764841)
> Moved to draft. @andremralves are you circling back to fix the conflicts?

Sorry for the delay! I've been a little bit busy lately, but I'll take care of the conflicts now.

💬 Prabhat1308 commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#issuecomment-2683847900)
Running the build after these configurations , the fuzz tests are running but I do get these warnings on almost every object being built . Is this expected ?

```
4 warnings generated.
[ 12%] Linking CXX static library libbitcoin_cli.a
4 warnings generated.
[ 12%] Building CXX object src/test/fuzz/util/CMakeFiles/test_fuzz.dir/net.cpp.o
[ 12%] Built target bitcoin_cli
[ 12%] Building CXX object src/CMakeFiles/bitcoin_common.dir/base58.cpp.o
clang++: warning: argument unused during compi
...

💬 andremralves commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#discussion_r1970954872)
> Hey, I'm reviewing this PR; can you please explain to me the thoughts behind removing this line of code?

Without this change the test case `./build/src/bitcoind --bind=0.0.0.0:port=onion` would fail. Since we are now setting bind_on_any as true when `-bind=0.0.0.0`, the server would attempt to bind to `0.0.0.0:port` two times in `CConnman::InitBinds` (lines 3272 and 3286). The addition in lines 3279-3296 handles the default bind for onion and fixes the issue.

💬 andremralves commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#issuecomment-2684119377)
Fixed the conflicts!

👋 andremralves's pull request is ready for review: "Execute Discover() when bind=0.0.0.0 or :: is set"
(https://github.com/bitcoin/bitcoin/pull/31492)

💬 zaidmstrr commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#issuecomment-2684234705)
Tested ACK [1561575](https://github.com/bitcoin/bitcoin/pull/31492/commits/1561575c2d984fd94c9679de3d1de207ef6c7c06)
After creating the dummy interfaces, I ran the test with args ihave1111and2222 and the test passed successfully.

🤔 maflcko reviewed a pull request: "ci: limit max stack size to 512 KiB"
(https://github.com/bitcoin/bitcoin/pull/31367#pullrequestreview-2643558757)
the two tasks are not running in docker, so the change here has no effect

🤔 hodlinator reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2643956281)
Reviewed 5f7b42642ad83faf23c8e75511f2379f9eae25ad

- Re-testing the commands.
- Managed to generate coverage for a functional tests, not sure if we should include it, includes copy-pasting temporary paths for the root test directory.

💬 hodlinator commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1971337297)
nit: Could use more frequently used term:
```suggestion
#### Using LLVM/Clang toolchain
```

💬 hodlinator commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1971368949)
nit: A bit verbose to repeat "directory", more helpful to include *index.html*?
```suggestion
The generated coverage report can be accessed at `build/coverage_report/index.html`.
```

💬 hodlinator commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1971353279)
No sure about the random 2-space indentation, also prefer comments preceding the command they're about.
````suggestion
```shell
# MacOS may require -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" and -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang"
cmake -B build -DCMAKE_C_COMPILER="clang" \
-DCMAKE_CXX_COMPILER="clang++" \
-DCMAKE_C_FLAGS="-fprofile-instr-generate -fcoverage-mapping" \
-DCMAKE_CXX_FLAGS="-fprofile-instr-generate -fcoverage-mapping"
cmake --build bu
...

💬 hodlinator commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1971335163)
I see. According to *doc/dependencies.md*, one only needs to have one of the compilers installed. I think it's clearer to remove the "Default" qualifier and needless to repeat the name of the toolchain.
```suggestion
The following generates a coverage report for unit tests.
```

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1971501892)
Yup, it shouldn't ever override either explicit user preferences or other parameter interactions. Thanks for checking.

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1971508254)
This comment was moved from the old site in `AppInitParameterInteraction` and i think it's a little bit useful in describing why the code is there. Will update it.

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2684851092)
> Probably yes. Though the following would add it to the list of overridden options:
> That seems slightly better than having this functionality enabled without indication.

Thanks. Slightly better. From a user perspective, showing an option which isn't documented as doing anything anymore might also be confusing. Would be better if it was marked as deprecated in some way, like in a different color or with `[deprecated]` after it. But would agree showing something is better than nothing.

👍 ryanofsky approved a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897#pullrequestreview-2644367686)
Code review ACK 6d7648795aa053f535510d11379fdd9d0399004c. Since last review, this was changed to drop dummy values instead of setting them to 0.

re: https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2683554828

> Concept NACK, just makes the code less straightforward and forces re-calculation later if needed. Harmless to leave it

I'm confused about why total fees were stored in a dummy field, and why the fields were represented as a negative number, and why the field didn't seem
...

👍 ryanofsky approved a pull request: "ci: build multiprocess on most jobs"
(https://github.com/bitcoin/bitcoin/pull/30975#pullrequestreview-2644387942)
Code review ACK 6f0b192275ffc2e93a3e48ba9a596ac1090ad130. No changes since last review other than rebase and dropping a documentation change as suggested

👍 ryanofsky approved a pull request: "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help"
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2644417825)
Code review ACK 798bc8d400b354d626678dc7b54c3cb0f464aa7a. Changes since last review were rebasing on top of #31785 to simplify this PR, updating comments and release notes, and also unhiding waitforblock and waitforblockheight methods

🤔 brunoerg reviewed a pull request: "contrib: Add deterministic-unittest-coverage"
(https://github.com/bitcoin/bitcoin/pull/31901#pullrequestreview-2644436422)
light ACK fa99c3b544b631cfe34d52fb5e71636aedb1b423

I have not reviewed the code yet, only checked that `deterministic-unittest-coverage` is working as expected. I tested on MacOS 14.3.

```sh
cmake -B build_31901 -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'
```
then I tested it for 3 tests:
```sh
RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build_31901 coinselector_tests
RUST_BACKTRACE=1 cargo run
...