💬 janb84 commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3225229760)
I have done a review on the code and tried to understand what to look for, to improve my PR reviewing in the future.
I'm missing some experience to give "judgement" on this PR.

The PR restores code to undo some "optimisations" that had a negative impact on the performance (restores caching ) and adds some extra benchmarks.

results of benchmarking this PR (both reverted):
| ns/op | op/s | err% | total | benchmark
|--------------------:|---------------
...

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301786086)
I think now I got what you are trying to say. So the things like `blockhash`, `dummy`, `sighashtype` etc are included because we are not mainly targeting these params; instead, we are targeting methods which might contain a `=` character. For example, `listsinceblock` contains a param named `label` which might contain an `=` char but according to the rule for consistency, we need to include all other string params related to the same method `(listsinceblock)`. That's why I included `blockhash` h
...

💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301789533)
I think it conceptually makes more sense to be passing paths as ` fs::Path`, rather than strings. Ideally, `BackupWallet` should be taking a `fs::Path` instead of a string, and the other conversions are only necessary for logging.

💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301791098)
Done

💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301791322)
Done

💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301791661)
Done

💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301792083)
Done

💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301792295)
Done

💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301792510)
Done

💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301792754)
Done

🤔 l0rinc reviewed a pull request: "net: Prevent node from binding to the same `CService`"
(https://github.com/bitcoin/bitcoin/pull/33231#pullrequestreview-3156667375)
I like this simpler version more, left a few more comments

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301687712)
nit: This seems to be used a single time, we might as well inline it

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301692222)
What's the advantage of duplicating the exact error here, wouldn't this suffice?
```suggestion
"Error: Duplicate binding configuration",
match=ErrorMatch.PARTIAL_REGEX)
```

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301697493)
The doc at the top is slightly off now:
```
"""
Test starting bitcoind with -bind and/or -bind=...=onion, confirm that
it binds to the expected ports, and verify that duplicate or conflicting
-bind/-whitebind configurations are rejected with a descriptive error.
"""
```

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301722439)
we could make this manual iteration more pythony with something like:
```python
from itertools import combinations_with_replacement
addr = f"127.0.0.1:11012"
for opt1, opt2 in combinations_with_replacement([f"-bind={addr}", f"-bind={addr}=onion", f"-whitebind=noban@{addr}"], 2):
print(f"opt1 = {opt1}, opt2 = {opt2}") # replace with assert, I just used this to validate if the iteration is the same
```

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301731934)
I personally consider these comments noisy - all of this is already evident from the code. If it's not, I'd rather change the code. Seeing the unupdated comment below, we shouldn't just add extra work for our future selves.

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301789293)
Is it expected that I'm seeing this twice?
```
2025-08-26T18:23:45Z [error] Duplicate binding configuration for address 127.0.0.1:11012. Please check your -bind, -whitebind, and onion binding settings.
2025-08-26T18:23:45Z tor: Thread interrupt
2025-08-26T18:23:45Z torcontrol thread exit
2025-08-26T18:23:45Z Shutdown in progress...
Error: Duplicate binding configuration for address 127.0.0.1:11012. Please check your -bind, -whitebind, and onion binding settings.
```

And if we're not ac
...

🤔 janb84 reviewed a pull request: "ci: use LLVM 21"
(https://github.com/bitcoin/bitcoin/pull/33258#pullrequestreview-3156831746)
ACK 4cf0ae474ba03830c86653f1abae4ab4d38c94e4

LLVM 21 has been released (on 26 August 2025 ~14:19 UTC) ✅
Changed CI's are running fine, they can download and run the new LLVM binaries. ✅

💬 polespinasa commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3225288108)
lgtm code review and tested ACK b7b249d3adfbd3c7b0c4d0499a86300f57982972
nothing to add that has not already been said in other comments

💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301826701)
What do you mean by "for consistency"? It should work fine without specifying the other parameters. Requiring that all string parameters need to be added "for consistency" just makes it more annoying to add new RPCs and parameters.