👍 l0rinc approved a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2783771949)
ACK ea3cc8388b4aefcaee61f75e8bf1bc03281a0c91

LGTM, left a few nits, but none are blockers.
Before merging someone with more experience should also review to be sure.

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053942125)
I'm fine with this as well, but to avoid complexity on declaration site and usage site (and to skip the "weak" part which seems out-of-place), we could try:
```suggestion
hash_id = int(sha256(expected_exception.encode("utf-8")).hexdigest()[:8], 16)
if self.options.tmpdir:
args.append(f"--tmpdir={self.options.tmpdir}/{hash_id}")
if self.options.randomseed is not None:
args.append(f"--randomseed={self.options.randomseed ^ hash_id}")
```

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053935108)
Maybe we could make the tenses more uniform here:
```suggestion
# Launches a child test process that runs this same file, instantiates
# a child test, and verifies that it raises only the expected exception.
```

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053946928)
nit:
I know I wrote `.*` but we might want `.+` instead, since we likely don't support empty ones

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053917393)
yes, I meant either print + raise or just `raise Exception("Should have failed earlier during startup")`, as you've mentioned.
`assert False` feels like a hack...

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053944080)
nit: '\n' seems less cryptic:
```suggestion
assert not errors, f"Child test didn't contain (only) expected errors:\n{'\n'.join(errors)}\n<CHILD OUTPUT BEGIN>\n{output}\n<CHILD OUTPUT END>\n"
```

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053912193)
Indeed, thanks

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053925442)
I think it applies to the other `findall` instances after this

🚀 fanquake merged a pull request: "ci: Drop no longer necessary `-Wno-error=array-bounds`"
(https://github.com/bitcoin/bitcoin/pull/32308)

🚀 fanquake merged a pull request: "doc: Add deps install notes for multiprocess"
(https://github.com/bitcoin/bitcoin/pull/32293)

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2053495512)
Unused?

🤔 hodlinator reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2783071854)
Code Review 46854038e7984b599d25640de26d4680e62caba7

Didn't get much deeper than surface level yet, but sharing what I found so far.

My primary suggestion is to change the `Obfuscation`-ctors to take static-extent `span`s to prevent accidental out-of-bounds access and also to clarify that we don't consume bigger `vector`s (see inline comment). `std::array` of matching size maps well to such `span`s.

#### Branch with suggestions

https://github.com/bitcoin/bitcoin/compare/master...hodl
...

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2053502958)
nit: Could declare default initialization value for `m_obfuscation` in header to avoid touching these lines.

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2053511581)
There's no `return` before the assignment on line 271, and we already set this field to 0 during initialization, so could be removed?

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2053535171)
2009?

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2053538744)
```suggestion
// Cached key rotations for different offsets.
std::array<uint64_t, SIZE_BYTES> m_rotations;

```
Most important for me is `m_`-prefix, newline and comment are nits.

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2053529374)
(Thanks for restoring sanity with the `m_`-prefix).

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2053627072)
util/check.h, random and cstring seem unused?
bit-header seems required for `rotr` and `std::endian`.

```suggestion
#include <serialize.h>
#include <span.h>

#include <array>
#include <bit>
#include <cassert>
#include <climits>
#include <cstdint>
```

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2053635433)
nit:
```suggestion
BOOST_CHECK_EQUAL(obfuscate, dbwrapper_private::GetObfuscation(dbw));
```
Here and above on line 30.

💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2053648375)
nit: This is inherited from stream.h, but could change to "target"/"dest" to make it more of a noun?
```suggestion
void operator()(std::span<std::byte> target, const size_t key_offset_bytes = 0) const
```
Same in `xor_roundtrip_random_chunks` and `xor_bytes_reference` test cases.