🤔 jarolrod reviewed a pull request: "depends: Update libmultiprocess library to fix C++20 macos build error"
(https://github.com/bitcoin/bitcoin/pull/29276#pullrequestreview-1834644340)
ACK 5dfd24581a7c5497601966a5371d8c33eabcee8a

This fixes the build issue.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1902037507)
Thank you @ryanofsky,

Updated f442a3a5b2a0a58bc263fbb9c87e8e4715de103a -> 38e25f67b4b2aeb3dd13f4f9ffa689f47718aa17 ([noGlobalSignals_7](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_7) -> [noGlobalSignals_8](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_7..noGlobalSignals_8))

* Fixup scripted-diff
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discu
...

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1902062244)
Updated 38e25f67b4b2aeb3dd13f4f9ffa689f47718aa17 -> 22048c19e236e4b683f1c8192883545d5c68f793 ([noGlobalSignals_8](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_8) -> [noGlobalSignals_9](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_8..noGlobalSignals_9))

* Improved scripted-diff to exclude renaming a method.

💬 remyers commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1460372757)
Fixed in [39d8621](https://github.com/bitcoin/bitcoin/pull/29272/commits/39d8621b0f3e9b20898278be35922ddab09683a3). I also updated the description to make it clearer that the APS path is tested due to APS being initially disabled.

💬 remyers commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1460385240)
This seems like a good practice and is consistent with other TRACEx uses in the code where x > 1. This would also makes it easier to check the correct number of parameters is used if PR #26593 is adopted. Fixed in [a32af4c](https://github.com/bitcoin/bitcoin/pull/29272/commits/a32af4c2593aa2cff49b6c6afeeba6c1681b5b45).

💬 0xB10C commented on issue "Policy: disallow P2PK transactions from relaying by default":
(https://github.com/bitcoin/bitcoin/issues/29285#issuecomment-1902073997)
fwiw: here are some P2PK usage numbers https://transactionfee.info/charts/inputs-and-outputs-p2pk/?start=2019-01-01

The blue line (P2PK outputs being spent) is what's relevant here.

📝 hebasto opened a pull request: "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`"
(https://github.com/bitcoin/bitcoin/pull/29287)
The `--enable-debug` configure option for the SQLite package does two things:
```autoconf
#-----------------------------------------------------------------------
# --enable-debug
#
AC_ARG_ENABLE(debug, [AS_HELP_STRING(
[--enable-debug], [build with debugging features enabled [default=no]])],
[], [])
AC_MSG_CHECKING([Build type])
if test x"$enable_debug" = "xyes"; then
BUILD_CFLAGS="$BUILD_CFLAGS -DSQLITE_DEBUG -DSQLITE_ENABLE_SELECTTRACE -DSQLITE_ENABLE_WHERETRACE"
CFLAGS=
...

🤔 furszy reviewed a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1834670140)
Concept ACK, will review soon.

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