💬 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

💬 mzumsande commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-1928027472)
[87d6155 ](https://github.com/bitcoin/bitcoin/commit/87d6155b91797d795433c9c8bca5da20c74f810c)to [efdc8e3](https://github.com/bitcoin/bitcoin/commit/efdc8e3a0ee81b143ee17a578f1e27b1c427a912): rebased

🤔 sr-gi reviewed a pull request: "test: p2p: adhere to typical VERSION message protocol flow"
(https://github.com/bitcoin/bitcoin/pull/29353#pullrequestreview-1854629637)
tACK utACK https://github.com/bitcoin/bitcoin/commit/c340503b67585b631909584f1acaa327f5af25d3

As a side note, I found the newly added comments regarding inbounds and outbounds to be a bit confusing. This is due to them being written from the POV of the Python node instead of the TestNode's, which is the logic used by `add_p2p_connection` and `add_outbound_p2p_connection`

💬 sr-gi commented on pull request "test: p2p: adhere to typical VERSION message protocol flow":
(https://github.com/bitcoin/bitcoin/pull/29353#discussion_r1473340868)
This is really just calling `super().send_version`. We could get rid of the whole class given no other method is being overwritten (or leave the alias if we want it for readability, but get rid of the method)

💬 sr-gi commented on pull request "test: p2p: adhere to typical VERSION message protocol flow":
(https://github.com/bitcoin/bitcoin/pull/29353#discussion_r1473341794)
Same as for `p2p_add_connections::P2PFeelerReceiver`

💬 edilmedeiros commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#discussion_r1478875833)
I see that you don't change the hardcoded value `0xc0`, just pass it with function calls.
It does make sense to generalize `taproot_tree_helper` and `taproot_construct`.

💬 edilmedeiros commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#discussion_r1478878383)
I'm not familiar with taproot, but do you think it does make sense to add a test to check if someone is passing the correct version constants to the functions?

📝 brunoerg opened a pull request: "fuzz: remove unused `args` and `context` from `FuzzedWallet`"
(https://github.com/bitcoin/bitcoin/pull/29388)
`ArgsManager args` and `WalletContext context` were previously used to create the wallet into `FuzzedWallet`. After fa15861763df71e788849b587883b3c16bb12229, they are not used anymore. This PR removes them.

💬 theuni commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1928047502)
@sipa Huh, thanks.

I could've sworn I saw an autoconf check for that that defined it when I was grepping around last week, but I guess I got it mixed up in my head with another. The lack of `HAVE_` or `USE_` should've been a giveaway :)

💬 brunoerg commented on pull request "fuzz: remove unused `args` and `context` from `FuzzedWallet`":
(https://github.com/bitcoin/bitcoin/pull/29388#issuecomment-1928050767)
cc: @maflcko