💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592946847)
nit: Not a fan of exceptions, and agree that it is a logic error to call `value()` or `error()` without first checking the state. But I think it might be slightly safer to `throw` here and below instead of asserting, so we have less of a behavior change once `util::Expected` is replaced by `std::expected`.

Otherwise users might experience changes under rare/improbable conditions where the process would shut down/crash before, and after the switch it would catch the exception and continue.

🚀 fanquake merged a pull request: "[30.x] Backports & 30.1rc1"
(https://github.com/bitcoin/bitcoin/pull/33997)

👍 darosior approved a pull request: "script: Add a separate ScriptError for empty pubkeys encountered in Tapscript"
(https://github.com/bitcoin/bitcoin/pull/33961#pullrequestreview-3545098441)
utACK 9d5021a05bd33c73276909eec961777867ddb412

💬 fanquake commented on issue "Minor Release 30.1":
(https://github.com/bitcoin/bitcoin/issues/34015#issuecomment-3617242317)
`v30.1rc1` tag is available: https://github.com/bitcoin/bitcoin/compare/v30.1rc1.

👍 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!