💬 maflcko commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#issuecomment-3052786718)
lgtm ACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50 🌌

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 54f9cb85c4be2c30
...

💬 darosior commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-3052905168)
> A user has reported their router is (probably) incompatible: https://bitcointalk.org/index.php?topic=5538256.msg65285896#msg65285896

I'm not even sure what to record it as on the table. MT-PON-AT-4?

📝 Sjors opened a pull request: "test: add logging to mock signers"
(https://github.com/bitcoin/bitcoin/pull/32928)
This will hopefully aid in debugging #32855.

Because `stdout` and `stderr` are consumed by the node, the mock external signers can't use them for logging.

Instead have them print directly into `test_framework.log`, which can then be be retrieved via `combine_logs.py`.

In preparation (and addtion) this PR starts with a few refactors:

- move `mock_signer_path` helpers to `util` and reuse them between `rpc_signer.py` and `wallet_signer.py`
- move the mocks from `test/functional/mocks`
...

💬 Sjors commented on issue "intermittent timeout in wallet_signer.py : 'createwallet' RPC took longer than 1200.000000 seconds":
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3052940079)
@maflcko that's easier said than done, but see #32928!

💬 danielabrozzoni commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2182856283)
A couple of questions regarding these two:
- REDOWNLOAD_BUFFER_SIZE: can we add a comment explaining why subtracting MAX_HEADERS_RESULT + 123? Copy-pasting what's in the commit description is ok
- COMMITMENT_PERIOD = 600: I suppose this is just mimicking the mainnet value?

🤔 danielabrozzoni reviewed a pull request: "headerssync: Preempt unrealistic unit test behavior"
(https://github.com/bitcoin/bitcoin/pull/32579#pullrequestreview-2983172052)
Review 78503a41546f6432045d3f9fd39860ead11d5c95

Code looks good to me, tests are passing locally. Thanks for picking up my comments! I left a couple more.

I did some additional testing by cherry-picking the latest commit of this PR and applying it to master, setting REDOWNLOAD_BUFFER_SIZE to >15000, and verifying that the warning was logged as expected.

<details>
<summary> git diff: </summary>

```diff
diff --git a/src/headerssync.cpp b/src/headerssync.cpp
index 9e8b190516..0d0a26d
...

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