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

💬 S3RK commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1472465541)
Not sure protocol implementation needs to now about the position in tx.vout array. AFAIK, output calculation doesn't depend on the position. Have you considered passing destinations one by one together with the counter if a destination is repeated?

💬 S3RK commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1472466841)
I'm too dumb to understand why do we need to return sum on input pubkeys together with the input hash. Does it have something to do with index and light clients?

👍 maflcko approved a pull request: "test: fix wallet_import_rescan unrounded minimum amount"
(https://github.com/bitcoin/bitcoin/pull/29343#pullrequestreview-1853179128)
lgtm

💬 maflcko commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1472497733)
You can't serialize a non-fixed-size vector as a span. This will lead to errors when deserializing. For example, a default constructed empty vector, serialized, can not be serialized into a vector which has been resized to 64 bytes.

💬 glozow commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472578055)
Yes I'd expect so

🚀 fanquake merged a pull request: "test: fix wallet_import_rescan unrounded minimum amount"
(https://github.com/bitcoin/bitcoin/pull/29343)