🤔 furszy reviewed a pull request: "wallet: fix coin selection tracing to return -1 when no change pos"
(https://github.com/bitcoin/bitcoin/pull/29272#pullrequestreview-1834680484)
> It seems to me that the first three commits are iterating on the same conceptual change.

Also, to keep a clean commits history, each commit should pass the CI tasks independently. And the first one alone is not passing.

💬 remyers commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#issuecomment-1902105124)
> Nit: It seems to me that the first three commits are iterating on the same conceptual change. I would prefer if those three commits were squashed into one so we immediately arrive at the final version. The refactor of the trace output should stay a separate commit, though.

Good point, I meant to do that; squashed in [d55fdb1](https://github.com/bitcoin/bitcoin/pull/29272/commits/d55fdb1a495190e213b1b5127f5d91e4a409765e).

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460466364)
nice! done. removed default value for `reconnect` in `peer_accept_connection` too since that is also always passed from `add_outbound_p2p_connection`

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460466533)
great catch! i've changed the `authenticate_handshake` function to first deal with garbage terminator detection before processing decoy packets and version packet. hopefully this is not a concern now?

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460466611)
done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460466749)
true! done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460467080)
nice catch! done. i've changed `authenticate_handshake` function a bit as discussed in https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1447657783.

> I think it would be even cleaner to change v2_receive_packet to also return contents = None in case no complete packet was received. In that case, the whole elif length == 0: branch can go away.

i think we need the `elif length == 0:` now so that we can exit out of the `while not self.tried_v2_handshake:` loop in case we don't have s
...

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460467126)
much simpler! done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460467188)
done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460467324)
makes sense! I've changed `respond_v2_handshake` and `complete_v2_handshake` to return the bytes consumed info too.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460467380)
true! done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460467496)
oh i like this! done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460467897)
hmm you're right. i used the idea of a queue initially because i liked the abstraction implied by queuing unsent messages to be sent after v2 handshake/reconnection.

since `on_connection_send_msg` is always the version message, i've changed the code to work with not clearing `on_connection_send_msg` before v2 handshake/reconnection is complete.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460468062)
removed this variable as discussed in https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460467897.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460468098)
done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460468173)
done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460468220)
done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460468278)
makes sense! i've changed `respond_handshake` and `complete_handshake` to return length consumed too.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460468407)
yes! i've simplified the comments. hopefully it's better now?

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1460468434)
done.