💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3052764983)
> Made a Java wrapper library for the Java folks out there! https://github.com/yuvicc/java-bitcoinkernel

Cool!

> While playing with the API I also wrote a [benchmarking test](https://github.com/yuvicc/bitcoin/tree/2025-06-kernel_benchmarking) for the script validation using the API vs the internal code just to see the performane overhead

It looks like your benchmark includes serialization in its hot loop, which is not the case for our internal `VerifyScriptBench`. I'd be surprised if th
...

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