💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261440012)
Would you mind adding some spacing around logical blocks in your code? I find a lot of the new code hard to read with a lack of spacing

e.g.:
```suggestion
{
AssertLockNotHeld(m_mutex);
LOCK(m_mutex);

std::vector<std::pair<NodeId, GenTxid>> expired;
auto tracker_requestable = orphan_request_tracker.GetRequestable(nodeid, current_time, &expired);
for (const auto& entry : expired) {
LogPrint(BCLog::TXPACKAGES, "\nTimeout of infli
...

👍 jamesob approved a pull request: "util: Teach AutoFile how to XOR"
(https://github.com/bitcoin/bitcoin/pull/28060#pullrequestreview-1526786597)
ACK fadb32556f64a72377c82be71a207195e6fcf68b ([`jamesob/ackr/28060.2.MarcoFalke.util_add_xorfile`](https://github.com/jamesob/bitcoin/tree/ackr/28060.2.MarcoFalke.util_add_xorfile))

Looks good! More concise change now that we're just adding XOR capability to existing classes.

<details><summary>Show signature data</summary>
<p>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK fadb32556f64a72377c82be71a207195e6fcf68b ([`jamesob/ackr/28060.2.MarcoFalke.util_add_xorfile`](https
...

💬 MarcoFalke commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1632883877)
Yes.

From:
```
Run asmap_direct with args ['valgrind', '--quiet', '--error-exitcode=1', '/root/bitcoin/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/root/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/asmap_direct')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 98738393
INFO: Loaded 1 modules (248427 inline 8-bit counters): 248427 [0x2781858, 0x27be2c3),
INFO: Loaded 1 PC tables (248427 PCs): 248427 [0x27be2c8,0x2b8
...

🚀 achow101 merged a pull request: "Add support for RFC8439 variant of ChaCha20"
(https://github.com/bitcoin/bitcoin/pull/27985)

💬 sipa commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1632907785)
(Slight tangent, not necessarily for this PR)

I was wondering if it wouldn't be better to XOR with a proper (but non-cryptographic) RNG output instead of a fixed byte pattern, as it'd avoid repetitive patterns remaining in the file, or weak keys that could accidentally be selected.

E.g. Xoshiro256++ is extremely fast (multiple GiB per second), see https://prng.di.unimi.it/. However, it's nontrivial to seek in, which would be needed here. I got a bit carried away and wrote code to do that,
...

💬 mzumsande commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1261480112)
I looked into this a bit deeply and did some test runs on signet, and now I'm no longer sure that this is a behavioral change requiring a release note in the first place:

My understanding is that the IBD behavior on master is:
* `ActivateBestChainStep()`  will connect only one block if we made progress
* The inner do / while loop of `ActivateBestChain` also will only run once unless the chain has less work than intitially (only relevant in a reorg).
* so during IBD we'll connect one block,
...

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1632931622)
Interesting. Right now, it is trivial to undo the XOR in python with something like:

```py
def util_xor(data):
with open("xor.key", "rb") as xor_f:
key = deser_string(xor_f)
for i in range(len(data)):
data[i] ^= key[i % len(key)]
return bytes(data)
```

If it requires users to instead implement a PRNG in python, they (who want to read the dat files for whatever reason) would most likely disable the XOR featur
...

💬 jamesob commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1632935482)
If the motivating reason for this feature is to prevent systematized, spurious detection of things in blocks (whether via antivirus software or human), isn't it sufficient to keep the XOR key very simple? Especially, as Marco says, anything more sophisticated would complicate consuming blockfiles from external software.

💬 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);` ?