👍 hernanmarino approved a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1863286073)
re ack f822ac9a24d684937f1258da89812e99c4b205ba

👍 brunoerg approved a pull request: "Fuzz: extend CConnman tests"
(https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-1863342508)
utACK 5e3c80da1473f90406b84c1ba14dc563ce4d2853

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478561706)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998490

> Any reason to change the sequence id? I think this isn't changed in `loadtxoutset` either?

I'll double check and add a comment. I think this might be needed to satisfy an assert that ensures nChainTx is set if nSequenceId is set, since these are both set when a block is received and all blocks before it have also been received.

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478549351)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477981888

> Any reason to touch this LOC?

No, will restore. I only meant to update the comments around it.

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478567668)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477996772

> isn't this always true? Maybe you meant to type `-` instead of `<`?
>
> In any case, the test seems to be passing either way, so this is "dead" code, similar to a code comment.

I think it was supposed to be i < last_assumed_valid_idx - 1, to preserve nChainTx in the snapshot block, and only set nChainTx to 0 before the snapshot block, so the test matches what happens in reality. But it does seem like this is not
...

🤔 ryanofsky reviewed a pull request: "assumeutxo: Get rid of faked nTx and nChainTx values"
(https://github.com/bitcoin/bitcoin/pull/29370#pullrequestreview-1863282117)
Thanks for the close review! Will work on your suggestions

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478584012)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478006900

> Is removing the validity check intended?

It was intended, but on second thought it is probably better to add back. The reason for removing it is that the block reaching VALID_TRANSACTIONS level is not really significant, because what is actually needed is for the block and all its ancestors back to the snapshot block or genesis block to reach VALID_TRANACTIONS level, and HaveNumChainTxs() is the direct and efficient
...

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478594492)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478022794

> Any reason to remove this check?

No, good catch, I'll add it back. I was thinking it wasn't really related to the other checks here, and trying to eliminate unnecessary uses of BLOCK_ASSUMED_VALID / IsAssumedValid in general. But as long as we have the assumed valid flag, might as well check this.

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478604893)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478018977

> Can this be further limited to only mark the snap_block if it has ASSUMED_VALID set? Otherwise, nodes which never used loadtxoutset will assume that some block is a "snapshot block", even though they never used that feature.

I'm pretty sure I can do that, but it should not change anything. If loadtxoutset is not used, GetSnapshotBaseBlock will return null and snap_block should already be false. The only case where t
...

📝 brunoerg converted_to_draft a pull request: "wallet, rpc: add BIP44 `account` in `createwallet`"
(https://github.com/bitcoin/bitcoin/pull/29129)
This PR adds an `account` parameter in `createwallet` RPC. It's the
BIP44 account that will be used in `SetupDescriptorScriptPubKeyMans`
to fetch the descriptors from the external signer.

⚠️ dpc opened an issue: "Default rpcthreads value (40 is way too small"
(https://github.com/bitcoin/bitcoin/issues/29386)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

I keep having to increase `rpcthreads` about everywhere I'm using bitcoind because without it it's not very robust/performant.

Pasting verbatim my comment in [bdk that just hit the same issue](https://github.com/bitcoindevkit/bdk/pull/1320#issuecomment-1926231768):


> From a perspective of most workloads e.g. asking for blockchain data, `bitcoind` (and `electrs`) acts very much like
...

💬 edilmedeiros commented on pull request "test: fix intermittent failure in `rpc_setban.py --v2transport`, run it in CI":
(https://github.com/bitcoin/bitcoin/pull/29372#issuecomment-1927616766)
I don't think this is something to stall a merge, but I could not make the test fail in master.

How are you calling it?

```
❯ python test/functional/rpc_setban.py
2024-02-05T17:48:04.516000Z TestFramework (INFO): PRNG seed is: 2620849867157253531
2024-02-05T17:48:04.517000Z TestFramework (INFO): Initializing test directory /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_tqvcwu8u
2024-02-05T17:48:06.209000Z TestFramework (INFO): Test that a non-IP address can be bann
...

💬 mzumsande commented on pull request "test: fix intermittent failure in `rpc_setban.py --v2transport`, run it in CI":
(https://github.com/bitcoin/bitcoin/pull/29372#issuecomment-1927633640)
> I could not make the test fail in master

> ❯ python test/functional/rpc_setban.py

You have to call it with v2, see PR title: `rpc_setban.py --v2transport`.

📝 BrandonOdiwuor opened a pull request: "test: fix RPC coverage check"
(https://github.com/bitcoin/bitcoin/pull/29387)
Fixes https://github.com/bitcoin/bitcoin/issues/27593

Currently, the RPC coverage check in functional tests doesn't include a list of all RPCs. This fix enables wallet RPCs to be included in the `rpc_interface.txt` used in the coverage check

💬 edilmedeiros commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#issuecomment-1927700389)
Tested ACK.

I've run `test/functional/feature_taproot.py` and `test/functional/wallet_taproot.py`.

The changes make sense in preparation for future expansions, as said.
They don't alter current test logic.

💬 BrandonOdiwuor commented on issue "test: RPC coverage check doesn't work?":
(https://github.com/bitcoin/bitcoin/issues/27593#issuecomment-1927701210)
I have created a draft PR https://github.com/bitcoin/bitcoin/pull/29387 enabling the wallet in the `create_cache.py`. The list of RPCs is more comprehensive - check the [rpc_interface.txt](https://github.com/bitcoin/bitcoin/files/14169914/rpc_interface.txt) generated

💬 sr-gi commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478574051)
nit: missing whitespace here

💬 sr-gi commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478689644)
I think this should be 16 though, not 15 as the comment claims:

{} {A} {AB} {ABC} {ABCD} {AB_D} {A_C} {A_CD} {A\__D} {_B} {_BC} {_BCD} {_B_D} {\_\_C} {\_\_CD} ~{\_\_C}~{\_\_\_D} ~{\_\_\_\_}~

💬 sr-gi commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1478573734)
"has not selected enough to fund a transaction" -> enoguh "elements", "coins", ...?

💬 sr-gi commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1476745673)
Isn't this equivalent to:

```
for (int i = utxo_pool.size()-1; i >= 0 ; --i)
```

I guess it's a matter of personal taste though, so feel free to disregard