💬 jonatack commented on pull request "util: Allow FastRandomContext::randbytes for std::byte, Allow std::byte serialization":
(https://github.com/bitcoin/bitcoin/pull/28012#discussion_r1261497511)
Why this change? It is currently unused, "I need this for some stuff" is vague, and this change is being cited in https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1257088164 as a reason not to use rand test helpers consistently.

💬 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_r1261505790)
> The reason why I left the comment is that `randbytes` is a template function

Hm, it became a template function only a few hours before the time of the comment, due to the merge of #28012.

> It would be good to mention at least one benefit.

Consistent use of the rand test helpers for developer quality-of-life might be a benefit.

#28012 was merged without the template actually being used or any clear reason for the change.

What would be your suggestion regarding how to proceed h
...

💬 MarcoFalke commented on pull request "util: Allow FastRandomContext::randbytes for std::byte, Allow std::byte serialization":
(https://github.com/bitcoin/bitcoin/pull/28012#discussion_r1261507530)
`std::byte` is used in many places to pass bytes. Adding this helper makes it easier to fill a vector with `std::byte`s. For example, it is used in 9999a49b3299bd25dde4805f5c68adef3876057f (https://github.com/bitcoin/bitcoin/pull/28060). But the helper can also be used in other places.

💬 jonatack commented on pull request "util: Allow FastRandomContext::randbytes for std::byte, Allow std::byte serialization":
(https://github.com/bitcoin/bitcoin/pull/28012#discussion_r1261511420)
Thank you for clarifying for me.

💬 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_r1261513559)
Seems best to drop the last two commits here. Implementing the 3rd suggestion above might make the diff much larger and seems out of scope for this pull.

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