💬 sr-gi commented on pull request "test: fixes flaky wallet_import_rescan functional test":
(https://github.com/bitcoin/bitcoin/pull/29350#issuecomment-1917894121)
> See #29343.

Ups, I missed that. Closing

🤔 sr-gi reviewed a pull request: "test: fix wallet_import_rescan unrounded minimum amount"
(https://github.com/bitcoin/bitcoin/pull/29343#pullrequestreview-1852388419)
ACK [26ad2ae](https://github.com/bitcoin/bitcoin/pull/29343/commits/26ad2aeb29dd0875e8509917ddaa586997e977d2)

I found this and got to a similar patch independently

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1917933292)
> WIP branch

Is this consistently failing for you (or can you at least reproduce it)? I can see it also failing on some of the current open PRs but I can not get my head around why. This should only happen if `PeerEarlyKey` receives any data from the node before sending anything at all, or after sending the network magic but before sending the remaining initial bytes + garbage.

To my understanding, the node should not have been able to parse enough data to be replying (with either v1 or v2
...

🤔 mzumsande reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1852266688)
Light Code Review ACK 27f260aa6e04f82dad78e9a06d58927546143a27

I don't have a strong opinion on the interface issue (I read the discussion and can see both points of view), but I do think that having a global variable `g_initial_block_download_completed` that lives in `protocol.h` and is updated dynamically from `net_processing` is an ugly layer violation too, so removing that is an important improvement from the architectural point of view.

💬 mzumsande commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1471891912)
could also have a check for `HasAllDesirableServiceFlags` each time `GetDesirableServiceFlags` is checked.

💬 mzumsande commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1917980494)
> So it seems like there is a larger bug here

Yes, that could be the case. Since I don't see how this can correct without restarting after the blocks are connecte to the chain and no longer assumed valid, I wonder if that could make the existing check https://github.com/bitcoin/bitcoin/blob/cad2df24b396be403f13f372ec48ea14a90d7f06/src/validation.cpp#L4949 fail.
Since it is very slow to sync with `-checkblockindex=1` even on signet (though commit https://github.com/bitcoin/bitcoin/pull/28339/
...

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1472063375)
Sure, noted. As it is a nit and would love to move forward, will leave it for a small follow-up. Thanks!

📝 mzumsande opened a pull request: "test: fix intermittent failure in p2p_v2_earlykeyresponse"
(https://github.com/bitcoin/bitcoin/pull/29352)
The test fails intermittently, see https://cirrus-ci.com/task/6403578080788480?logs=ci#L3521 and https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1916996716.
I think it's because of a race between the python NetworkThread and the actual
test, which will both call `initiate_v2_handshake`. I could reproduce it by adding a sleep into `initiate_v2_handshake` after the line `self.sent_garbage = random.randbytes(garbage_len)`.

Fix this by waiting for the first `initiate_v2_handshake` to
...

💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1918064080)
see #29352 for a test (and a way to reproduce it).

💬 theuni commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1918078386)
> Yeah, that's exactly what I was expecting fwiw.

Done.

📝 theStack opened a pull request: "test: p2p: adhere to typical VERSION message protocol flow"
(https://github.com/bitcoin/bitcoin/pull/29353)
This PR addresses a quirk in the test framework's p2p implementation regarding the version handshake protocol:

Currently, the VERSION message immediately after an inbound connection (i.e. TestNode outbound connection) is made. This doesn't follow the usual protocol flow where the initiator sends a version first, the responder processes that and only then responds with its own version message. Change that accordingly by only sending immediate VERSION message for outbound connections (or after
...

👍 andrewtoth approved a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1852723966)
ACK 27f260aa6e04f82dad78e9a06d58927546143a27

💬 andrewtoth commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1472190098)
As noted in https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1454112543, stale tip checks don't factor into the current approach. This check seems redundant now with the one on [lines 54-56](https://github.com/bitcoin/bitcoin/pull/28170/files#diff-aafd7c9690634ecdad15d527c2bab6bffbcddd2db444a6099da0305d2d2271beR54-R56).

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1472203963)
> As noted in #28170 (comment), stale tip checks don't factor into the current approach.

Sure. As it is a nit, will update this comment in the follow-up PR. Thanks!.

> This check seems redundant now with the one on lines 54-56.

I think it's worth checking whether the services flags can still change after receiving blocks from the network. The lines 54-56 are checking updates prior receiving any block.

💬 hernanmarino commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-1918347305)
Rebased on top of master to fix CI errors.

🤔 stratospher reviewed a pull request: "test: fix intermittent failure in p2p_v2_earlykeyresponse"
(https://github.com/bitcoin/bitcoin/pull/29352#pullrequestreview-1852942477)
clever find! will ACK after this is fixed.

💬 stratospher commented on pull request "test: fix intermittent failure in p2p_v2_earlykeyresponse":
(https://github.com/bitcoin/bitcoin/pull/29352#discussion_r1472338409)
1d04d58: i think we should call it a different name - `self.connected` or something like that? i'm still getting a crash with the sleep statement because python is confusing `connection_made` variable and callback function.

🤔 BrandonOdiwuor reviewed a pull request: "debugwindow: update session ID tooltip"
(https://github.com/bitcoin-core/gui/pull/788#pullrequestreview-1853065866)
Concept ACK

✅ maflcko closed a pull request: "refactor: deduplicate AmountFromValue() functions"
(https://github.com/bitcoin/bitcoin/pull/28134)

🤔 S3RK reviewed a pull request: "Silent Payments: Implement BIP352"
(https://github.com/bitcoin/bitcoin/pull/28122#pullrequestreview-1853145226)
Started reviewing. Basic question first. Which RPCs are supposed to work at this stage? I was under impression, that none RPCs should understand SP addresses yet and the support come with send and receiving PRs. But since you've registered SP destination in `CTXDestination` variant, the SP addresses would be recognised by some RPCs. Is that intended? Should we verify that they return sane results?