💬 jonatack commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#issuecomment-1632990104)
Dropped the three commits relating to the test helpers per discussion at https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1250420188.

The remaining changes were ACKed above.

💬 MarcoFalke commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1261561436)
For context, the functions stem from a time when they actually had a multi-line implementation body. Then the implementation was removed and replaced to be an alias, but no one bothered to resolve the simple alias.

🤔 sipa reviewed a pull request: "util: Teach AutoFile how to XOR"
(https://github.com/bitcoin/bitcoin/pull/28060#pullrequestreview-1526966730)
Code review ACK

💬 sipa commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1261559740)
This reads to me like a multiplication between `std::FILE` and `rel`.

How about `if (auto rel{release()}) return std::fclose(rel);` ?

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1261583878)
Yeah, it is a clang-format-16 bug. Someone should try to fix or report it.

💬 amitiuttarwar commented on issue "Auto detect IPv6 connectivity":
(https://github.com/bitcoin/bitcoin/issues/28061#issuecomment-1633025258)
concept ACK. there have been several reports of nodes running in environments where IPV6 is unreachable. since the code considers it reachable by default, addrman currently persists the unusable addresses and the node periodically attempts to make connections even in those environments.

💬 recursive-rat4 commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1633048818)
Why not `CXXFLAGS='$CXXFLAGS -gdwarf-4'`?

💬 kristapsk commented on issue "Auto detect IPv6 connectivity":
(https://github.com/bitcoin/bitcoin/issues/28061#issuecomment-1633054751)
Concept ACK. Last time I checked a lots of ISPs in Latvia still don't support IPv6.

💬 sipa commented on issue "Update ChaCha20 to RFC 8439?":
(https://github.com/bitcoin/bitcoin/issues/19225#issuecomment-1633082036)
For posterity, this has been fully addressed by #27985.

🤔 furszy reviewed a pull request: "[WIP] descriptors: do not return top-level only funcs as sub descriptors"
(https://github.com/bitcoin/bitcoin/pull/28067#pullrequestreview-1527178771)
@sipa, how the allowance of internal `raw()` subscripts would play out with previous releases?.

I'm thinking on the current wallet migration process, which is broken for pubkey unknown `sh(pkh)` scripts (also for `wsh(pkh)`).

This PR could fix the bug and be backported. Then, after having all the pertinent discussions etc. We could implement the new feature (compat breaking change?) for the next release.

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1633198398)
@willcl-ark wow thank you so much for catching that. After all the refactoring, we had forgotten to actually *connect to the unix socket!* I added a functional test to ensure that the proxy works now and I have also tested in production with Tor locally.

This required a fairly intense refactor that included inserting 2 new commits and rebasing several others:

`git range-diff 32a5a20225 c3b0f137a9 b1199bce0f`

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1633199594)
@vasild Thanks for your work on this :-) I reorganized the extra commit you sent me. For now, still going without `variant` for `Proxy` just to keep the refator more simple, so leaving the i2p stuff mostly alone as well.

🤔 mzumsande reviewed a pull request: "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module"
(https://github.com/bitcoin/bitcoin/pull/28031#pullrequestreview-1527052773)
some more comments, not finished yet though.

💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261645151)
commit 543273d96e896adf5531ed961856aa0eb70cbe57:
nit: `uint256&` like in the line above?

💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261605505)
There should be a `setmocktime` call with the initial time somewhere before the first `fastforward`, otherwise this can fail intermittently.

💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261668254)
4cbb63175398d9b8afe755d2adb24edbd2b4913b:
nit: could use `orphan_wtxid`, here and in various other spots.

💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261779747)
I don't think there should ever be a `\n` prefix, why would we want to separate the meta information (timestamp, threadname etc.) from the actual log entry?

💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261690923)
29d9d326d5193bb9a410a8881eabc93de5dd6266:
`EraseOrphanOfPeer` looks like a rebase error (it's never implemented and removed in d92b017f6818c1de3286e5dbc35af3860fdf7547).

💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261786477)
nit: could use `orphan_wtxid` introduced in the previous commit, here and in various other places.

💬 achow101 commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261786736)
In 7601a60cddc4405f58f71879f996322329509dfe "crypto: add Poly1305 class with std::byte Span interface"

Use `UCharCast` cast here (and below)?