💬 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

💬 fjahr commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r1979583535)
Thanks @marcofleon , that the code doesn't run seems to be because we don't add enough transactions to hit the limit, see `LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100)` and in the coverage you linked the `break` on line 165 is never hit as well. This requires a bit of additional conceptual review but for now I have increased the loop limit to 5000 and I hope we should see some hits with that. Will the link get updated with the new numbers when I visit it again tomorrow?

💬 fjahr commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r1979591922)
Thinking about it a bit more, this was probably an optimization to make the test faster. Not sure if increasing the number that much is seen as a downside. I could also add something so that we usually have 0-100 but then with a 10% chance or so I increase the number dramatically so that we hit the number max block limit with some regularity as well. There my understanding performance requirements on fuzz tests is limited so not sure which approach would be preferred.

👍 dergoegge approved a pull request: "doc: Bring reduce-memory.md up to date"
(https://github.com/bitcoin/bitcoin/pull/31985#pullrequestreview-2657838684)
ACK 4bb2eae061c9ed0c20e2a435145bf77c9b3601cc - 16 matches the `DEFAULT_HTTP_THREADS` constant.

👍 brunoerg approved a pull request: "doc: Bring reduce-memory.md up to date"
(https://github.com/bitcoin/bitcoin/pull/31985#pullrequestreview-2657863389)
ACK fff4f93dff8ba67689e43929615e3c63c67015e4

👍 TheCharlatan approved a pull request: "doc: Bring reduce-memory.md up to date"
(https://github.com/bitcoin/bitcoin/pull/31985#pullrequestreview-2657872493)
ACK fff4f93dff8ba67689e43929615e3c63c67015e4

🤔 rkrux reviewed a pull request: "test: Use rpc_deprecated only for testing deprecation"
(https://github.com/bitcoin/bitcoin/pull/31977#pullrequestreview-2657788839)
Concept ACK c5e384de6c0b92692021272a7464cd26a54d298c

New to this test, suggested some verbiage change. PLMK if it doesn't fit.