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

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

> Also, it may be worth moving this right peer2 so inbounds and outbounds are grouped

didn't follow. i've renamed `peer7` to `peer1` since this is first peer in restarted node. is that what you meant?

🤔 murchandamus reviewed a pull request: "wallet: fix coin selection tracing to return -1 when no change pos"
(https://github.com/bitcoin/bitcoin/pull/29272#pullrequestreview-1834683381)
ACK d55fdb1a495190e213b1b5127f5d91e4a409765e

📝 TheeTw23 opened a pull request: "Merge bitcoin/bitcoin#29249: depends: add NM output to gen_id"
(https://github.com/bitcoin/bitcoin/pull/29288)
6ec2813cd88d5f0b955d746e4711a8ad256ea47f depends: add NM output to gen_id (fanquake)

Pull request description:

`NM` is part of the current toolset, and can be set by the user. Include it in `gen_id`.

ACKs for top commit:
TheCharlatan:
Re-ACK 6ec2813cd88d5f0b955d746e4711a8ad256ea47f

Tree-SHA512: 2ada61e03783f9eb441f285ef5da50557ad729cb52ce2d2c4b2c38103dab29920a26262d4545fd2ac7fbf1cedc4902cd2359833544fbc0debf829c12a63e9769

<!--
*** Please remove the following help text before submitti
...

✅ fanquake closed a pull request: "Merge bitcoin/bitcoin#29249: depends: add NM output to gen_id"
(https://github.com/bitcoin/bitcoin/pull/29288)

📝 fanquake locked a pull request: "Merge bitcoin/bitcoin#29249: depends: add NM output to gen_id"
(https://github.com/bitcoin/bitcoin/pull/29288)
6ec2813cd88d5f0b955d746e4711a8ad256ea47f depends: add NM output to gen_id (fanquake)

Pull request description:

`NM` is part of the current toolset, and can be set by the user. Include it in `gen_id`.

ACKs for top commit:
TheCharlatan:
Re-ACK 6ec2813cd88d5f0b955d746e4711a8ad256ea47f

Tree-SHA512: 2ada61e03783f9eb441f285ef5da50557ad729cb52ce2d2c4b2c38103dab29920a26262d4545fd2ac7fbf1cedc4902cd2359833544fbc0debf829c12a63e9769

<!--
*** Please remove the following help text before submitti
...

💬 0xB10C commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#issuecomment-1902355248)
ACK d55fdb1a495190e213b1b5127f5d91e4a409765e

The `int32_t` cast and the tracing related changes look good to me. Thanks for reformatting the tracepoints! I'm not familiar enough with coin selection to comment on these changes.