💬 TheCharlatan commented on pull request "refactor: rpc: Pass CBlockIndex by reference instead of pointer":
(https://github.com/bitcoin/bitcoin/pull/29021#discussion_r1419294898)
Right, you only added checks to the lines you already were touching, so I guess that makes sense.

👍 TheCharlatan approved a pull request: "refactor: rpc: Pass CBlockIndex by reference instead of pointer"
(https://github.com/bitcoin/bitcoin/pull/29021#pullrequestreview-1770634925)
ACK fa5989d514d246e56977c528b2dd2abe6dc8efcc

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1419299493)
I believe I (finally) actually fixed this behavior to count the number of direct conflicts.

💬 maflcko commented on issue "fuzz: Fix stability, determinism issues":
(https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-1845718292)
Yeah, or alternatively steps to reproduce the stability output with afl, so that non-afl people can have some fun, too.

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1419303051)
Should be fixed now.

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1845736014)
re-ACK [f56ec8a](https://github.com/bitcoin/bitcoin/pull/28765/commits/f56ec8a3b086b0f2f8a0fbde861447e40ffbc3d9)

💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1419345302)
That's why it says "This changes after a new descriptor is imported".

But right now the logic behind populating this field is not documented. Without reading a bunch of pull request comments, one might wonder why we don't just store the xpriv, why we go through all descriptors instead of one, etc.

👍 pablomartin4btc approved a pull request: "refactor: rpc: Pass CBlockIndex by reference instead of pointer"
(https://github.com/bitcoin/bitcoin/pull/29021#pullrequestreview-1770724880)
tACK fa5989d514d246e56977c528b2dd2abe6dc8efcc

I've re-tested #29003 & `getblockchaininfo` on `mainnet`.

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1419392876)
Sure, but that's not what your suggested text documents. I've added a comment earlier during the actual loading that describes it.

💬 maflcko commented on issue "./contrib/guix/guix-build does not work on riscv64":
(https://github.com/bitcoin/bitcoin/issues/29020#issuecomment-1845865501)
Sure, sent the fix: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=67697

💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1419441534)
It is enforced by the line
`if (!node_to_evict.has_value() || *node_to_evict < node->GetId()) {` (second condition),
not through `m_connected` but by picking the eligible node with the highest NodeId. I'll add to the comment that "newest" means "highest `Node Id`", do you think that would be sufficient?

👍 stickies-v approved a pull request: "mempool: Don't sort in entryAll"
(https://github.com/bitcoin/bitcoin/pull/29019#pullrequestreview-1770813594)
ACK 87fa444a04854a5d3a9ff283a8b6c34587e8430f

💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1419470209)
I agree that it makes no sense, if you want just one connection you should pick a node you trust and do `-connect`. Using automatic connections for that means eclipsing yourself - maybe we should warn about that in general?
That being said, this PR does log disconnection to `-blocksonly` peers, although only in `BCLog::NET` - do you suggest to make it unconditional based on `-maxconnections`?

💬 murchandamus commented on pull request "Avoid changeless input sets when SFFO is active":
(https://github.com/bitcoin/bitcoin/pull/28985#issuecomment-1845984159)
Rebased on top of #28994

🤔 sipa reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1770812972)
Halfway through.

💬 sipa commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1419512254)
In commit "[test] Introduce EncryptedP2PState object in P2PConnection"

Nit: I'd find it a bit cleaner to *not* give this variable such an accurate name for reasons of abstractions. The code here shouldn't care about *what* it is that the `EncryptedP2PState` tells us to send; it should just be sent. Maybe call it "send_handshake_bytes" ?

💬 sipa commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1419473256)
In commit "[test] Construct class to handle v2 P2P protocol functions"

It seems a bit strange to always send exactly 10 decoy packets before the version packet. Maybe randomize that too?

💬 sipa commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1419447076)
In commit "[test] Construct class to handle v2 P2P protocol functions"

It would be helpful to explain what the return value of the various functions in this class means. In several cases the return value seems to be bytes-that-should-be-sent-to-the-peer, but there are exceptions (-1 meaning downgrade for `respond_v2_handshake`, and `v2_ecdh` returning the shared secret).

💬 sipa commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1419502857)
In commit "[test] Construct class to handle v2 P2P protocol functions"

As I understand it, this function does not distinguish between "valid packet with 0-byte contents" and "decoy packet", which I believe is incorrect (the version packet can have 0-byte contents, but if it's a decoy, we should still listen for more packets). This isn't caught because Bitcoin Core's V2Transport implementation never sends decoys, but if it did, I believe the tests would break.

My suggestion would be to retu
...

💬 sipa commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1419509492)
In commit "[test] Construct class to handle v2 P2P protocol functions"

I don't think we should be hardcoding this assumption. Decoy packets with 0-byte contents are very much correct and possibly useful. See my other comment on `v2_receive_packet` about distinguishing decoys from 0-byte content packets.