💬 theuni commented on pull request "build: Add missed definition for `AM_OBJCXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1938901924)
@epiccurious What specifically are you ACKing here? Have you read the conversation above?

💬 instagibbs commented on pull request "v3 followups":
(https://github.com/bitcoin/bitcoin/pull/29424#issuecomment-1938910571)
ACK 6b161cb82a9766ef814f05e5b8f019f15d5ee14d

💬 mzumsande commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1486367765)
> Would that execute `rpc_net.py` two times and both in v2 mode?

Yes, although it's not really related to this PR it's the same on master (just that `P2PConnection`s use v1 both times there).
I had thought about this before and was thinking of addressing it in a follow-up PR, which would also add a CI job for `--v2transport`.
The downside of dropping the `--v2transport` specific jobs from `test_runner.py` is that these would be executed a lot less by individuals running the test suite (even
...

💬 mzumsande commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1486374392)
yes. Fixed both.

💬 mzumsande commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1938941846)
[b9912e2 ](https://github.com/bitcoin/bitcoin/commit/b9912e2df9767540ab7f53cb3ba37fd2e2491806)to [bf5662c](https://github.com/bitcoin/bitcoin/commit/bf5662c678455fb47c402f8520215726ddfe7a94): just small fixes to the last commit message.

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1486385523)
> If not I think its okay like this.

removed comments, how it's precisely used is an implementation detail outside of this code

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1938973409)
rebased

🤔 sr-gi reviewed a pull request: "test: adds outbound eviction functional tests, updates comment in ConsiderEviction"
(https://github.com/bitcoin/bitcoin/pull/29122#pullrequestreview-1875503633)
Thanks for the review @tdb3

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1486346242)
That's covered in the last part of `test_outbound_eviction_unprotected`

https://github.com/bitcoin/bitcoin/blob/edc14bc9da332cae39f8803db559b532b3c74e16/test/functional/p2p_eviction.py#L202-L218

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1486369882)
Protected peer actually implies `outbound-full-relay`. The comment you mentioned may be outdated. I may add an update to it to the PR.

https://github.com/bitcoin/bitcoin/blob/edc14bc9da332cae39f8803db559b532b3c74e16/src/net_processing.cpp#L2875-L2885

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1486391103)
The limit is tested in this test. We create 8 connections, from which only 4 are protected. If you mean that there are no unprotected peers that behave properly, that could be added. e.g.

4 protected, send initial headers, do nothing, don't get evicted
2 unprotected, keep in sync, don't get evicted
2 unprotected, do not keep up, get evicted

👍 vasild approved a pull request: "test: use v2 everywhere for P2PConnection if --v2transport is enabled"
(https://github.com/bitcoin/bitcoin/pull/29358#pullrequestreview-1875604974)
ACK bf5662c678455fb47c402f8520215726ddfe7a94

See also https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1485626063

💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1486417918)
Correct. >= rather than >.

💬 vasild commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1486432039)
Alright, "it's the same on master" that is a good point!

So, if we have `--v1transport` (for each individual test and for `test_runner.py`?) then what would happen if ran without either one of `--v1transport` or `--v2transport`?

💬 LarryRuane commented on pull request "bitcoin-cli help detail to show full help for all RPCs":
(https://github.com/bitcoin/bitcoin/pull/29163#issuecomment-1939061997)
> Is there a better UX option here? What about `bitcoin-cli helpdetail`?

I agree your suggestion would be better, or maybe even `bitcoin-cli -helpdetail` (with a dash) because as it is now, "detail" seems like the name of an RPC. I'll look into what it would take to do either of those, thanks for the suggestion.

👍 vasild approved a pull request: "Release `LockData::dd_mutex` before calling `*_detected` functions"
(https://github.com/bitcoin/bitcoin/pull/26781#pullrequestreview-1875650020)
ACK 38f93fe0cb680425f5fec7c794b39c0e1795f9dc

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-1939071798)
I redefined how peers are split in `p2p_eviction:test_outbound_eviction_protected_mixed` to address https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1485375517. Both commits can be squashed (leaving them as they are for now in case someone does not agree with the approach)

💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1486449111)
Thank you, yes, I should have been clearer. The updated `test_outbound_eviction_protected_mixed` in 91005f7459993e1f7139e46b53eef1bb04860a7c looks great (includes 4 protected, 4 unprotected (2 honest, 2 misbehaving by sending no headers or old)). ACK

💬 mzumsande commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1486473051)
We'd have to make some kind of decision what to default to - maybe v1 now and switch to v2 after a majority of the network has upgraded?
My idea for now was to only add `--v1transport` in `test_runner.py` the few tests that are run twice, not everywhere.

💬 furszy commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1486494438)
Shouldn't be a problem but, just to keep this process consistent; as `UpdateWalletDescriptor` clears the SPKM internal scripts cache, shouldn't do the same for the general wallet cache too?
E.g. remove the scripts belonging to this spkm here, so the `TopUp` call that is executed few lines below (re)populates the scripts derived from the updated SPKM.