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

💬 Christewart commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#discussion_r1478815788)
here is where the previous hard coded value was that the parameter is replacing.

💬 mzumsande commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1927937515)
Will address feedback and https://github.com/bitcoin/bitcoin/pull/29356#discussion_r1478769478 after the merge of #29356

📝 mzumsande converted_to_draft a pull request: "test: use v2 everywhere for P2PConnection if --v2transport is enabled"
(https://github.com/bitcoin/bitcoin/pull/29358)
#24748 added v2 transport to the python `P2PConnection`, but so far each test that wants to make use of it needs to enable it on an individual basis.
This PR changes it so that if the test suite is run with `--v2transport` option, v2 is used in each test by default, not only for connections between two bitcoind instances as before, but also wherever `P2PConnection` is used. Individual tests can override this global option.

To do that, a few tests need to be adjusted, which is done in the fir
...

💬 tcharding commented on issue "`libbitcoinconsensus.a` is unusable":
(https://github.com/bitcoin/bitcoin/issues/28779#issuecomment-1928003536)
Thanks for considering us.

💬 mzumsande commented on pull request "p2p: Increase inbound capacity for block-relay only connections":
(https://github.com/bitcoin/bitcoin/pull/28463#issuecomment-1928020517)
[a9a64f9 ](https://github.com/bitcoin/bitcoin/commit/a9a64f990e1029ed89d2d08e76b96dcba6e8e91e)to [77a612d](https://github.com/bitcoin/bitcoin/commit/77a612dd75eb2383d62d2d21c182684ff2026416): rebased