👍 maflcko approved a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2987179699)
concept ack. Left some nits on a first pass

I haven't benchmarked this myself yet.

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2185334637)
nit in 79901734fa357c2449608f8648ab84daf1cccd8b: A span is just a pointer, so adding `const` before `std::span` doesn't really say much. In fact, it could be confusing, because `const std::span<std::byte>` actually points to mutable data. I'd just remove the `const` here.

If you really want to add it, it should probably go before `std::byte`?

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2185356900)
nit in https://github.com/bitcoin/bitcoin/commit/79901734fa357c2449608f8648ab84daf1cccd8b: Generally, I try to avoid double inversions. Could probably write this without any `!`:

```cpp
BOOST_CHECK_EQUAL(original == roundtrip, all_zeros);

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2185417749)
q: What does "Should wrap around" mean? The whole test is about key wrapping, so it seems redundant?

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2185364629)
same (about const)

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2185413089)
nit in https://github.com/bitcoin/bitcoin/commit/79901734fa357c2449608f8648ab84daf1cccd8b: This also checks alignment? I guess from https://github.com/bitcoin/bitcoin/runs/42787791556?

If yes, could clarify here "... of key wrapping and data alignment.", and below after `write_offset`.

Otherwise, someone could "optimize the test" and remove `write_offset`, because it seems unused.

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2185442080)
nit in 79901734fa357c2449608f8648ab84daf1cccd8b: looks unrelated and clang-format-wrong? Instead of changing whitespace on redundant comments, it would be better to remove the comments, or leave them as-is?

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2185483463)
nit in https://github.com/bitcoin/bitcoin/commit/8b03122ed8f6ef6955532d4499aeb66bdb12c24b: Why rename this? This just makes it harder to review via `--color-moved=dimmed-zebra`.

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2185524965)
nit in https://github.com/bitcoin/bitcoin/commit/8b03122ed8f6ef6955532d4499aeb66bdb12c24b: Any reason to include the `Bench` suffix. It is redundant and inconsistent, as none of the other benchmarks have it. Generally, it is best keep everything as-is, unless there is a reason to change it.

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2185480196)
nit in 8b03122ed8f6ef6955532d4499aeb66bdb12c24b: Why add an unused include in a commit that should be mostly move-only ?

💬 fanquake commented on pull request "p2p: add more bad ports":
(https://github.com/bitcoin/bitcoin/pull/32826#issuecomment-3036726862)
Backported to 29.x in #32863.

💬 HowHsu commented on pull request "index: Fix missing case in the comment in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#issuecomment-3036730579)



> You're correct that the current docstring is incorrect. I personally don't find your suggestion very clear either, as the two cases talk about different parts of the return statement.
>
> An alternative would be to keep the docstring as-is and just make the chaintip case more explicit in code:
>
> ```c++
> const CBlockIndex* pindex = chain.Next(pindex_prev);
> if (pindex || pindex_prev == chain.Tip()) {
> return pindex;
> }
> ```
>
> git diff on [d6bf3b1
...

🤔 ryanofsky reviewed a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2987635116)
Updated 03f585d58ccf4d5c02d621c5b6046d45807b3201 -> 6a6b6f9093c1f82b8ea05ad87f421eabc8849738 ([`pr/libexec.9`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.9) -> [`pr/libexec.10`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.10), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/libexec.9..pr/libexec.10)) with various suggestions updating signing & CI scripts and documentation

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185666814)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184561400

> That's because if a user starts with "bitcoin" then "this help message" implies there's nothing additional to see.

To be sure, the "show this help message" does not appear when you just run `bitcoin` with no arguments since I wanted "show this help message" to mean "show this exact help message". Happy to apply any changes, but I think this should already avoid the concern you described.

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185645268)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184938620

Good catch! Updated these as well

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185643632)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185068952

Thanks! Updated

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185640133)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184924462

Thanks! I implemented maflcko's suggestion:

```diff
- DIR_UNIT_TEST_DATA="${DIR_UNIT_TEST_DATA}" LD_LIBRARY_PATH="${DEPENDS_DIR}/${HOST}/lib" "${BASE_OUTDIR}"/libexec/test_bitcoin --catch_system_errors=no -l test_suite
+ DIR_UNIT_TEST_DATA="${DIR_UNIT_TEST_DATA}" LD_LIBRARY_PATH="${DEPENDS_DIR}/${HOST}/lib" "${BASE_BUILD_DIR}"/bin/test_bitcoin --catch_system_errors=no -l test_suite
```

But I am wondering if ma
...

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185626705)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184948130

> Q: slightly unrelated, but if testing should be available for the installed version, how come fuzzing isn't? Seems it should be mentioned with a 🛠 at least...

The table doesn't mention the fuzz binary just because the table is a list of installed files, and the fuzz binary is not installed before or after this change.

Installing the fuzz binary would seem to make sense for consistency. But one problem with insta
...

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185642980)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185083314

> nit: if you're already editing this file consider fixing line 86:
>
> > ... double-spends due to _the_ lack of synchronization

Thanks, added commit to fix

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185644085)
re: https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184944417

> nit: not everyone views the rendered view, we might want to align the source view as well:

Thanks! Updated