💬 sipa commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1632959304)
It's fair that this is perhaps not a real concern right now; I don't know. I find it slightly unsatisfying that we can't break any 8-byte repetitive pattern (see e.g. the picture [here](https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation#Electronic_codebook_(ECB)) for a visual illustration), while computationally speaking doing so should not be any concern.

Regarding external software... I'm not sure. It wouldn't surprise me that literally everyone who is interested in reading these
...

💬 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_r1261522071)
It was just one example. To explain it more generally: Any interface change to the underlying class will have to be done twice (or left inconsistent), which doesn't seem like an improvement (at least to me).
Also there is nothing that prevents a developer from directly calling the underlying class, so adding two different ways to achieve the same or similar thing is confusing as well (at least to me).

> What would be your suggestion regarding how to proceed here? (Thanks!)

My general sugg
...

💬 jonatack commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1261545511)
Yes, but until such time that the approach is revamped, using the existing helpers consistently would have been beneficial in that it aids development and review by being more clear. That said, it's orthogonal to this pull and I'm dropping the commits that made them consistent.

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1632985858)
I could write the 128MiB XOR data to the key file in full. Complexity would be 2x, because each `std::fread` will be done twice (once to read the XOR key section, and once to read the XOR'd data section)?

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