💬 theStack commented on pull request "wallet: check for `agg_pub` validity in MuSig2 signing":
(https://github.com/bitcoin/bitcoin/pull/34010#issuecomment-3617176769)
Oh right, the crash already happens in the `std::span{pubkey}.subspan(1, 32)` expression (if `std::span{pubkey}` is empty due to being invalid), before the first constructor is called. It seems we should detect invalid participant pubkeys as early as possible and not allow them to be added to the wallet state in the first place, I suspect a better place to fix this would be `DeserializeMuSig2ParticipantPubkeys`, where a validity check is currently missing.

⚠️ fanquake opened an issue: "test: `interface_ipc.py` (might?) start skipping if installed capnp version changes"
(https://github.com/bitcoin/bitcoin/issues/34016)
Noticed this on a Fedora (Rawhide) Box that has capnp `1.2.0` (recently updated from 1.1.0) and `pycapnp` `2.1.0` installed, but was skipping `interface_ipc.py` as if the Python module was not available. The failure was that pycanp `2.1.0` tries to load an older shared lib, and fails:
```bash
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py failed, Duration: 0 s

stdout:


stderr:
Traceback (most recent call last):
File "/root/ci_scratch/build/test/functional/interface_ipc.py", line 2
...

👍 ryanofsky approved a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3545066334)
Code review ACK fa76a6620012fb738639d8fd7ce17b185bfd376c

🤔 darosior reviewed a pull request: "test: Add DERSIG unit tests to script_tests.json"
(https://github.com/bitcoin/bitcoin/pull/33973#pullrequestreview-3545073630)
Looks good to me, just want to confirm my understanding of one of the test cases.

💬 darosior commented on pull request "test: Add DERSIG unit tests to script_tests.json":
(https://github.com/bitcoin/bitcoin/pull/33973#discussion_r2592933311)
So here the correctly-DER-encoded signature is not a valid signature for the correctly-encoded public key, is that right?

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