💬 pablomartin4btc commented on pull request "Fix: Ensure 'Transaction View' remains disabled if no wallet is selected":
(https://github.com/bitcoin-core/gui/pull/780#issuecomment-1845700681)
> I'm not sure if this is the right approach: user might want to enable `Mask values` before opening his wallet?

I was just giving it a second thought and if we follow the original approach a user could still have the desired behaviour as the following:

<details>
<summary>check display here</summary>


</details>

First time the user opens or creates a wallet can tick
...

💬 dergoegge commented on issue "fuzz: Fix stability, determinism issues":
(https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-1845701335)
> Nice. Though, I wonder if there is something public available.

I'm not aware of something like that hosted by oss-fuzz, all the per fuzzer stats seem to require auth.

I've been primarily fuzzing with afl++ lately, I can look at hosting some stats from that.

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1419291403)
Chunks for a given cluster are already sorted in descending feerate order, is that what you're asking about or is there another issue I'm overlooking?

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