👍 darosior approved a pull request: "Fix 11-year-old mis-categorized error code in OP_IF evaluation"
(https://github.com/bitcoin/bitcoin/pull/32143#pullrequestreview-3545111160)
utACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43

🤔 sipa reviewed a pull request: "script: Add a separate ScriptError for empty pubkeys encountered in Tapscript"
(https://github.com/bitcoin/bitcoin/pull/33961#pullrequestreview-3545113694)
ACK 9d5021a05bd33c73276909eec961777867ddb412

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3617315125)
I've generated some flamegraphs from perf data recorded during IBD for ~10k blocks between 850-900k for both master and this branch.

The 4 worker threads can be seen clearly on the left of the graph. The binary search through short txids is barely noticeable, which confirms our approach to not use an unordered_set + salted hasher.

Looking at `b-msghand` (main) thread, we see `ConnectBlock` dominating it on master while block serialization becomes the dominant factor on this branch. The mai
...

🤔 furszy reviewed a pull request: "wallet, test: Ancient Wallet Migration from v0.14.3 (no-HD and Single Chain)"
(https://github.com/bitcoin/bitcoin/pull/33186#pullrequestreview-3545108565)
> The node v0.14.3 cannot be synced because it doesn't have the syncwithvalidationinterfacequeue RPC implemented (required by test_framework.py), so the solution is to copy the block folder to the modern node and start it with -reindex-chainstate. Because of this additional complexity, this testing is best managed in a separate file rather than in the existing migration test files.

In order to know when the node is synced, cannot you just wait for a specific log line? See `busy_wait_for_debug
...

💬 furszy commented on pull request "wallet, test: Ancient Wallet Migration from v0.14.3 (no-HD and Single Chain)":
(https://github.com/bitcoin/bitcoin/pull/33186#discussion_r2593023838)
Could check the wallet version here.
The non-hd one should have version 6000 (`WalletFeature::FEATURE_COMPRPUBKEY`) and the hd one should have version 130000 (`WalletFeature::FEATURE_HD`)

💬 furszy commented on pull request "wallet, test: Ancient Wallet Migration from v0.14.3 (no-HD and Single Chain)":
(https://github.com/bitcoin/bitcoin/pull/33186#discussion_r2593032651)
rescanning after migration shouldn't be needed

💬 furszy commented on pull request "wallet, test: Ancient Wallet Migration from v0.14.3 (no-HD and Single Chain)":
(https://github.com/bitcoin/bitcoin/pull/33186#discussion_r2592962526)
@w0xlt can you please check this ^^ ?

Also, aren't we essentially removing the unicode coverage from the CI with this change? `wallet_ancient_migration.py` test should always be enabled there.

💬 jurraca commented on pull request "contrib: Count entry differences in asmap-tool diff summary":
(https://github.com/bitcoin/bitcoin/pull/33939#issuecomment-3617365793)
utACK `fd4ce55121`

tested several different diff runs.

🚀 fanquake merged a pull request: "contrib: Count entry differences in asmap-tool diff summary"
(https://github.com/bitcoin/bitcoin/pull/33939)

💬 ryanofsky commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3617394520)
re: https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3617021123

Thanks for taking a look at this.

> I think `util::Result` is conflating 2 related but orthogonal needs:
>
> * Returning a successful value or error(s).
> * Returning multiple structured errors/warnings, not just one.

The result class is handling both of these things (since c++ works most conveniently with a single return value), but I don't see a sense in which it is conflating them. The implementation handle
...

💬 rkrux commented on pull request "wallet: check for `agg_pub` validity in MuSig2 signing":
(https://github.com/bitcoin/bitcoin/pull/34010#issuecomment-3617404481)
I did think about adding it there but decided against it to keep the change related to the crash site. But I don't feel strongly about it and will update the PR to add the check in PSBT deserialization.

📝 Chand-ra opened a pull request: "fuzz: Add a test case for `ParseByteUnits()`"
(https://github.com/bitcoin/bitcoin/pull/34017)
`ParseByteUnits()` is the only parsing function in `strencodings.cpp` lacking a fuzz test. Add a test case to check the function against arbitrary strings and randomized `default_multiplier`.

💬 ryanofsky commented on issue "test: `interface_ipc.py` (might?) start skipping if installed capnp version changes":
(https://github.com/bitcoin/bitcoin/issues/34016#issuecomment-3617445384)
Maybe this would get the failure to be reported properly:

```diff
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -19,7 +19,7 @@ from test_framework.wallet import MiniWallet
# Test may be skipped and not have capnp installed
try:
import capnp # type: ignore[import] # noqa: F401
-except ImportError:
+except ModuleNotFoundError:
pass

@asynccontextmanager
```

💬 hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3617452151)
@maflcko

Your suggestions have been taken. Thank you!

📝 stickies-v opened a pull request: "log: exempt all category-specific logs from ratelimiting"
(https://github.com/bitcoin/bitcoin/pull/34018)
Previously, running with `-debug=<category>` would exempt the debug and trace log messages in that category from rate limiting, but not the info/warning/error category-specific messages (which are rare). This is unintuitive and unnecessary.

When users run with `-debug`, we already assume they are power users and that they will have significantly higher log volumes, so there is no real benefit from suppressing info log messages in that category.

Fix this by applying ratelimiting exceptions
...

💬 stickies-v commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#issuecomment-3617473369)
> I probably won't get to picking that up anytime soon though, so if you want to open an alternative to this PR, feel free to do so!

I've opened #34018 as an alternative!

💬 fanquake commented on issue "test: `interface_ipc.py` (might?) start skipping if installed capnp version changes":
(https://github.com/bitcoin/bitcoin/issues/34016#issuecomment-3617499308)
@ryanofsky yes, that works.

👍 dergoegge approved a pull request: "fuzz: Add a test case for `ParseByteUnits()`"
(https://github.com/bitcoin/bitcoin/pull/34017#pullrequestreview-3545393178)
utACK 57b888ce0ebdeb34d866fd1511052fd740cc5ab8

Thank you for your interest in contributing to our fuzzing efforts! This looks fine to me.

`ParseByteUnits` is not publicly exposed, i.e. it doesn't handle untrusted inputs, and I would not consider adding fuzz tests for this type of function as a priority. As the in-repo fuzz tests are pretty saturated, it can be hard to spot valuable areas to improve (especially if you are new to the code base). A good path for making valuable contributions
...

👍 stickies-v approved a pull request: "scripted-diff: Use LogInfo over LogPrintf"
(https://github.com/bitcoin/bitcoin/pull/29641#pullrequestreview-3545434794)
ACK fa4395dffd432b999002dfd24eb6f8d7384fbcbe

nit: would be nice to update the scripted-diff to also remove trailing newlines if that's not a huge pita

⚠️ darosior opened an issue: "RFC: randomize over netgroups in outbound peer selection"
(https://github.com/bitcoin/bitcoin/issues/34019)
The current mechanism for choosing outbound peers picks one at randoms among known-reachable addresses, with the caveat that we do not connect twice to the same netgroup (by default /16's and, if an ASmap is configured, by AS's). A more robust mechanism for preventing an attacker to control all of a node's outbound connections would first randomize over netgroups and then pick a known-reachable address within that netgroup.

This alternative mechanism would make the probability for an attacker t
...