💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2185441225)
@stratospher the test completes much faster on my system, but by checking the timing of other tests, I put it among "Tests less than 2m" section because it completes faster than `mining_getblocktemplate_longpoll.py` than slower than `mempool_persist.py`

💬 maflcko commented on issue "test: break `feature_block` into subtests?":
(https://github.com/bitcoin/bitcoin/issues/32877#issuecomment-3036407180)
> These things are not necessary coupled

Are you sure? I'd say the reorg test is useful to perform on a "dirty" state.

Other than that, I think the reorg test is the slowest part of the test, so if you want to run the test twice (once with reorg and once without reorg), it should be fine and also solve your issue?

🤔 stickies-v reviewed a pull request: "index: Fix missing case in the comment in NextSyncBlock()"
(https://github.com/bitcoin/bitcoin/pull/32875#pullrequestreview-2987509048)
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:

```cpp
const CBlockIndex* pindex = chain.Next(pindex_prev);
if (pindex || pindex_prev == chain.Tip()) {
return pindex;
}
```

<details>
<summary>git diff on d6bf3b1cc2</summar
...

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-3036620662)
> Left a few nits/test comments that can be done in a follow-up

I will make a follow-up PR post-merge to address the comments (which are all reasonable, just want to limit re-review)

💬 stratospher commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2185605623)
ah I see, but that's on mainnet and not on regtest, so comment might need an edit. agree that the value doesn't affect the test!

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2185605781)
I had thought about that as well. I looked for patterns like this, but I did not look very hard. Do you think this deserves a comment in a follow-up to warn against behavior like this?

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2185609148)
Will address in a follow-up, I think this is from an older version of the PR where the functions used in location 2/3 were not caught by the rate limit.

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2185611861)
`Stats` is a better name, `LogRateLimiter::LogLimitStats` is a bit verbose / stutter-y. Will address in a follow-up, I am not so good with naming.

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