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