💬 danielabrozzoni commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2182784980)
It took me a while to figure this out - I was initially wondering why the chains have `TARGET_BLOCKS - 1` and `TARGET_BLOCKS - 2` blocks, until I realized that I wasn't counting the genesis block 😅

I'm not sure if it might be worth it to add a comment like
`// The first chain will contain TARGET_BLOCKS, the second one TARGET_BLOCKS - 1`

It might be obvious already, feel free to drop the suggestion

💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#issuecomment-3053003087)
> Sorry for the force-pushes, but this uncovered pre-existing issues in the fuzz tests that were not using mocktime during init. So I went ahead and fixed those as well in a separate commit. Should be good now, hopefully 😅

Went ahead and cherry-picked out the fuzz-fixes into https://github.com/bitcoin/bitcoin/pull/32927. Should be easy to (re-)review that one.

💬 stratospher commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#issuecomment-3053468148)
> I guess that means we could, instead of calc_flags_and_header, just use a bool calc_flags (for which we can't do a similar check). Do you think that would be clearer?

yes sounds good! (whenever you retouch next)

Ah missed the fact that iteration of entire block index is done in `SetBlockFailureFlags()` as well. I originally thought that iteration of entire block index is done only for best header calculation and `calc_flags_and_header` wasn't useful. Hence the confusion.

🤔 stickies-v reviewed a pull request: "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs"
(https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-3002523441)
I think the proposed fix for `getdescriptoractivity` is not robust, and the `GetWalletNameFromJSONRPCRequest()` overload confusing, as detailed in comments.

💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2195638276)
I think these overloads are very confusing, and as such at higher risk for introducing bugs. Overloading on `std::string& wallet_name` vs `const std::string* wallet_name` when one is an out-parameter returning the wallet endpoint name, and the other an in-parameter representing a query parameter is not great. One overload throwing, and the other not, is even worse.

I'm still considering what the better alternatives would be - it seems like doing it properly would probably add quite a bit, but
...

💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2195680365)
> Do we actually need to ensure that at least one such (valid) parameter is provided? If so, we should probably check that `blockindexes_sorted` and `scripts_to_watch` are not both empty. Otherwise, I think just making sure we only execute the `request.params[n].get_array().getValues()` statements if we've first checked that `!request.params[n].isNull()` should do the trick?

I don't think my comment was addressed, and e.g. this test still failing. According to the PR description and code chan
...

👍 stickies-v approved a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-3002650511)
re-ACK 4c772cbd83, no changes except release notes update

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2195710943)
nit: Conditional logs (or rather, everything with `should_ratelimit=false`) don't get the prefix. Perhaps the better/easier UX is to just apply it to all logs?

```suggestion
higher than debug, that is `info`, `warning`, and `error`. All unconditional logs will be
```

💬 glozow commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2195734934)
fwiw I didn't mean for this to hold up the PR, but thanks for taking the suggestion!

💬 glozow commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-3053633853)
reACK 4c772cb

🚀 glozow merged a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604)

💬 instagibbs commented on pull request "p2p: Relax BlockRequestAllowed to respond to advertised blocks":
(https://github.com/bitcoin/bitcoin/pull/32869#issuecomment-3053653904)
@davidgumberg for completeness, the other solution, as stated in the issue tracker, would be to send a `notfound` in response to requests for invalid blocks. I'm not sure where people landed on that as that discussion was a long time ago

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2195750958)
I could use a better naming for this function, not being an overload. My idea was to fix the issue without touching much and later refactor both `GetWalletNameFromJSONRPCRequest` and `GetWalletForJSONRPCRequest` on a separate PR. Currently `GetWalletForJSONRPCRequest`, which is used broadly, calls internally `GetWalletNameFromJSONRPCRequest` and wanted to keep them untouched for now.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2195765195)
I prefer it this way.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2195765353)
Done.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2195765533)
Done.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2195766179)
Done. Thanks for the screenshot.

🚀 glozow merged a pull request: "mempool: use `FeeFrac` for ancestor/descendant score comparators"
(https://github.com/bitcoin/bitcoin/pull/32799)

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2195916152)
I've changed this test to import `TX_MAX_STANDARD_VERSION` instead.

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2195917838)
Done