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

💬 achow101 commented on pull request "doc: add historical release notes for 26.0":
(https://github.com/bitcoin/bitcoin/pull/29023#issuecomment-1846005450)
ACK ca5937553b4b4dde53995d0b66e30150401023eb

🚀 achow101 merged a pull request: "doc: add historical release notes for 26.0"
(https://github.com/bitcoin/bitcoin/pull/29023)

💬 brunoerg commented on pull request "contrib: add test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1846105723)
Force-pushed addressing ASMap health check. cc: @fjahr

💬 Retropex commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1846169419)
I understand your argument.

However, now `OP_RETURN`, baremultisig, and other flaws such as `OP_0` `OP_IF`, are used to spam the chain. So what do you think we should do? Let them spam the chain by what we can't give them a toy on which to spam?

🤔 achow101 reviewed a pull request: "wallet, rpc: add anti-fee-sniping to `send` and `sendall`"
(https://github.com/bitcoin/bitcoin/pull/28944#pullrequestreview-1771199234)
ACK f348eadadd6099e7763f04eb3d42cfd8bba33146

💬 achow101 commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1419702642)
In f348eadadd6099e7763f04eb3d42cfd8bba33146 "test: test sendall and send do anti-fee-sniping"

nit: `verbose` here too.