💬 l0rinc commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#issuecomment-2697572586)
ACK 75486c8ed87a480b9f0c4dc7a10f3cd4eee87b12

Compared to [previous review](https://github.com/bitcoin/bitcoin/compare/725c0fd95b15157bd0ec141cc24d27818f536656..75486c8ed87a480b9f0c4dc7a10f3cd4eee87b12#diff-7ff2e1d1ed59fa96766a663236d71bfdd00b4af3321fde39f7dbf022bd968a0a), `APPEND_LDFLAGS` was removed and the description simplified to avoid repetition.

💬 laanwj commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2697629617)
Concept and code review ACK 4cd95a2921805f447a8bcecc6b448a365171eb93

Seems low-risk. As this is compile-time logic, i don't think this should actually affect the compiled code? (haven't verified, though)

🤔 vasild reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2656773286)
Approach ACK 1245e05325b41101eddc76ba9214d910489e35b6

In the light of https://github.com/bitcoin/bitcoin/issues/31047 and https://github.com/bitcoin/bitcoin/pull/31394 I have been thinking that is would be useful to at least document the steps needed to gather coverage with clang. Thanks for the initiative!

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979133216)
This works, but maybe it is better to use the `APPEND_*` alternatives?

https://github.com/bitcoin/bitcoin/blob/15717f0ef3960969ee550a4a41741987b86684dc/CMakeLists.txt#L33-L37

i.e.

```suggestion
-DAPPEND_CFLAGS="-fprofile-instr-generate -fcoverage-mapping" \
-DAPPEND_CXXFLAGS="-fprofile-instr-generate -fcoverage-mapping"
```

If you decide to do that, then you will also need `-DAPPEND_LDFLAGS="-fprofile-instr-generate -fcoverage-mapping"` because `CMAKE_CXX_FLAGS` causes the f
...

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979323892)
Why `-sparse`?
```
--sparse[=true|false]
Do not emit function records with 0 execution count. Can only be
used in conjunction with -instr. Defaults to false, since it can
inhibit compiler optimization during PGO.
```

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979077210)
This defaults to `CMAKE_BUILD_TYPE=RelWithDebInfo` which uses `-O2`. Using a debugger on an optimized binary usually results in bogus line numbers due to optimizations that can't be mapped back to source code lines. I do not know if it is the same with coverage.

To be on the safe side I have been using `-DCMAKE_BUILD_TYPE=Debug` (which uses `-O0`). Also, I don't see a reason to use `-O2`, other than to have the tests finish faster.

The example in https://clang.llvm.org/docs/UsersManual.htm
...

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979318687)
Using `%m` will help reduce the chance of `.profraw` files being overwritten in case the same PID is reused by the OS:

```suggestion
LLVM_PROFILE_FILE="default_%m_%p.profraw" ctest --test-dir build # Use "-j N" here for N parallel jobs.
```

https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#running-the-instrumented-program mentions:

> For a program such as the [Lit](https://llvm.org/docs/CommandGuide/lit.html) testing tool which invokes other programs, it may be necessary to set
...

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979457702)
If there are profile files from more than one executable, like from `test_bitcoin` and from `bitcoind` then the syntax of this changes a little bit: drop `build/src/test/test_bitcoin` and add `-object=build/src/test/test_bitcoin -object=build/src/bitcoind`:

```
llvm-cov show
--object=build/src/test/test_bitcoin \
--object=build/src/bitcoind \
--instr-profile=build/coverage.profdata \
--format=html \
--show-instantiation-summary \
--show-line-counts-or-regions \
...

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979389115)
I would suggest collecting coverage from functional tests as well because that would change a bit the arguments of `llvm-cov` (see later) and because the path to the profile file has to be specified as an absolute path which is not obvious:

```md
Generating the raw profile data based on `ctest` execution:

```shell
LLVM_PROFILE_FILE=$(pwd)/build/raw_profile_data/%m_%p.profraw build/test/functional/test_runner.py
`` `
```
it has to be a full path because otherwise the `.profraw` file wi
...

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979463614)
Locally, I have been using this as well:

```
-ignore-filename-regex="src/crc32c/|src/leveldb/|src/minisketch/|src/secp256k1/|src/test/"
```

💬 marcofleon commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r1979488543)
It seems like the conditional never ends up being true, which you can see in the test's [coverage](https://marcofleon.github.io/coverage/miniminer/coverage/root/bitcoin/src/test/fuzz/mini_miner.cpp.html).

In general, I do think you're right about wanting the fuzz test to cover a range of possible values, but I'm not quite familar enough with the mining code to know what the best approach here would be.

💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2697689385)
friendly ping: @sipa @darosior

🤔 rkrux reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2657415572)
Few comments.

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1979442112)
What scenario is not tested if this flow is not done and the transaction generated above is mined directly?

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1979452088)
The `descriptorprocesspsbt` RPC uses a different flow than the `walletprocesspsbt` RPC because it calls `ProcessPSBT()` instead of `FillPSBT()`. I think it'd be nice if this tests also ensures the sighash is added via the `descriptorprocesspsbt` RPC as well.

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1979502013)
Let's add a TODO in this file to write a test that checks `PSBT_GLOBAL_TX_MODIFIABLE` field is correctly set if `SIGHASH_SINGLE` is used in any of the inputs once the PSBT V2 PR https://github.com/bitcoin/bitcoin/pull/21283 is merged?

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1979463960)
Re: https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1933698058

Earlier I thought we should add a test that checks the removal of `non_witness_utxo` in case of segwit v1 inputs only but I see there is one already here `test_utxo_conversion` in `rpc_psbt`. However, it works using `walletprocesspsbt` RPC only. I think this PR can test for the same functionality via `descriptorprocesspsbt` RPC too because it uses a different flow internally. Either of `test_utxo_conversion` or `test_sig
...

💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1979406143)
> The BIP states that the sighash field is merely a suggestion

Can you share which BIP is this that states it's a suggestion? I found a contradicting statement in [BIP 174](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki), which is confusing.

> Signatures for this input must use the sighash type, finalizers must fail to finalize inputs which have signatures that do not match the specified sighash type.

💬 rkrux commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1979526916)
> doesn't matter,

What immediately caught my eye was the unusual looking 0, which at first glance made me think it was zero. Nevertheless, I am not suggesting any changes related to this, it just made me curious.

📝 laanwj opened a pull request: "doc: Bring reduce-memory.md up to date"
(https://github.com/bitcoin/bitcoin/pull/31985)
Update default number of RPC threads to 8 (#31215) and remove reference to very old version of bitcoin core.

Let me know if you notice other mismatches with current defaults.

💬 laanwj commented on pull request "rpc: increase the defaults for -rpcthreads and -rpcworkqueue":
(https://github.com/bitcoin/bitcoin/pull/31215#issuecomment-2697832530)
@dergoegge Done in #31985