💬 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.

💬 rkrux commented on pull request "test: Use rpc_deprecated only for testing deprecation":
(https://github.com/bitcoin/bitcoin/pull/31977#discussion_r1979608022)
This is the final comment after this change:

> This test should be used to verify correct behaviour of deprecated RPC methods without the -deprecatedrpc flag.

I think it can be reworded to be more explicit in the intention. I'm assuming the deprecated RPCs will just throw errors, please correct me if I'm wrong.

```
This test should be used to verify the errors of the currently deprecated RPC methods (without the -deprecatedrpc flag) until such RPCs are fully removed.
```

Suggesting
...

💬 rkrux commented on pull request "test: Use rpc_deprecated only for testing deprecation":
(https://github.com/bitcoin/bitcoin/pull/31977#discussion_r1979568355)
Couple typos
`depgrecatedrpc`
`anther`

💬 rkrux commented on pull request "test: Use rpc_deprecated only for testing deprecation":
(https://github.com/bitcoin/bitcoin/pull/31977#discussion_r1979609096)
Nit:
```
self.log.info("No tested currently deprecated RPC methods")
```

💬 fanquake commented on pull request "doc: Bring reduce-memory.md up to date":
(https://github.com/bitcoin/bitcoin/pull/31985#issuecomment-2697910206)
>Let me know if you notice other mismatches with current defaults.

In this file "The minimum value for `-dbcache` is 4." However it's entirely possible to run bitcoind with `-dbcache=1`. Maybe that line can just be dropped?

💬 darosior commented on issue "test: intermittent issue in p2p_1p1c_network.py":
(https://github.com/bitcoin/bitcoin/issues/31946#issuecomment-2697912165)
Another instance occurred in #31603 https://github.com/bitcoin/bitcoin/actions/runs/13498602529/job/37711361328.

💬 fanquake commented on pull request "kernel: pre-29.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/31978#discussion_r1979618887)
Yea, `700` or even more here, seems fine here.

💬 laanwj commented on pull request "doc: Bring reduce-memory.md up to date":
(https://github.com/bitcoin/bitcoin/pull/31985#issuecomment-2697915217)
> In this file "The minimum value for -dbcache is 4." However it's entirely possible to run bitcoind with -dbcache=1. Maybe that line can just be dropped?

Could be removed if it's not longer correct, but to be clear: 4 here was meant here as the minimum effective value, not the minimum value that would be accepted as command line argument.

💬 glozow commented on pull request "kernel: pre-29.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/31978#discussion_r1979622063)
fwiw I also have 12GB for chainstate, but I didn't think it made much sense to decrease the number.