💬 HowHsu commented on pull request "index: Fix missing case in the comment in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#issuecomment-3036657452)
> 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](http
...

💬 waketraindev commented on pull request "rpc: add optional nodeid param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2185644531)
Added line to help text, preferred using peer id naming instead of index, to avoid confusion with the array index

💬 waketraindev commented on pull request "rpc: add optional nodeid param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2185650755)
First condition checks if the parameter is defined so it returns all results even if peer_id is 0

💬 waketraindev commented on pull request "rpc: add optional nodeid param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3036685462)
Rebased on top of master, updated help text

👍 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