💬 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

💬 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-1927837780)
I'm sorry for not being clear in my comment.
I tried that with your code, and it runs something that passes (see below).

My point is that running `rpc_setban.py` in 5b8c59 does not seem to fail in any way in my box.
The mentioned commit passed the CI tests.
I understand I'm a newcomer, and I'm trying to learn how the tests work by reproducing the error you found instead of just reading the code in the PR.

Yet, I don't see this as a problem to merge.
It might be an issue triggered by
...

💬 theuni commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1927863174)
It'd be really nice if we could avoid including `bitcoin-config.h` from headers at all, that would pretty much solve the problem.

But even simple examples like [this one](https://github.com/bitcoin/bitcoin/blob/master/src/crypto/muhash.h#L27) make that difficult without reorganizing code (and this kind of thing shouldn't dictate code imo) :(

💬 sipa commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1927923770)
> But even simple examples like this one make that difficult without reorganizing code (and this kind of thing shouldn't dictate code imo) :(

That example works without config.h or configure at all, `__SIZEOF_INT128__` is set by the compiler.

💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1927928419)
reACK https://github.com/bitcoin/bitcoin/pull/26836/commits/238993ea40c1c384e9fbcf83bfcf45efc73ad1bb

verified the diff, thanks for taking the suggestions!

🤔 mzumsande reviewed a pull request: "test: make v2transport arg in addconnection mandatory and few cleanups"
(https://github.com/bitcoin/bitcoin/pull/29356#pullrequestreview-1863617748)
Code Review ACK e7fd70f4b6b163f4ad5b25b4da7fa79899245235

Nit, only if you need to re-push:
The sentence "`TestNode::add_outbound_p2p_connection()` is the only place where
addconnection test-only RPC is used." in the commit message is not strictly true because of the other spot in `feature_anchors.py`.

💬 mzumsande commented on pull request "test: make v2transport arg in addconnection mandatory and few cleanups":
(https://github.com/bitcoin/bitcoin/pull/29356#discussion_r1478769478)
Unrelated to this PR, but it could be changed from `False` to `self.options.v2transport` in the future (e.g. in #29358)