💬 hebasto commented on issue "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc":
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1902083289)
> Can you update [bitcoin-core/crc32c-subtree#6](https://github.com/bitcoin-core/crc32c-subtree/pull/6) so that the commit includes a description of the problem, an explanation of the fix, and any further information. We should probably add a comment inline explaining why a memcpy is being added to that code.

Thanks! Done.

🤔 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-1834678471)
ACK, 350606d6093563e93b8faa059af92e19f9d439e5. The final code LGTM.

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.

💬 delta1 commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#issuecomment-1902098796)
concept ACK 4658c83

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