💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592820086)
You are right, but I can't see this going wrong in practise, so I'll leave this as-is. (The alternative would be to specialize, but that requires writing a lot more code)

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592820508)
thx, done

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592820693)
thx, used T{}

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592820889)
thx, removed this line

💬 rkrux commented on pull request "wallet: check for `agg_pub` validity in MuSig2 signing":
(https://github.com/bitcoin/bitcoin/pull/34010#issuecomment-3617074954)
Yes, the second constructor appears to be involved. This comment here suggests where the error lies because of an empty aggregate pubkey added in the fuzz input: https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2585106930

I believe that skipping the loop iteration before the following line hits should help in fixing IIUC?
https://github.com/bitcoin/bitcoin/blob/b8e66b901d563d7ba6042e6be54d7e48b9fffc83/src/script/sign.cpp#L299

🤔 rkrux reviewed a pull request: "test: interface_ipc.py minor fixes and cleanup"
(https://github.com/bitcoin/bitcoin/pull/34003#pullrequestreview-3544944968)
Concept ACK 6ef092c0e4343fc421c323ff09d3c791fb4bc33a

I'm new to this test, I built the `bitcoin-node` executable using `-DENABLE_IPC=ON` and ran the test using `venv` as mention in the readme file. I spent some time trying to understand the interfaces involving capnp.

The changes seem to clean the test quite a bit and makes it easier to go through.

💬 rkrux commented on pull request "test: interface_ipc.py minor fixes and cleanup":
(https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2592834249)
In 6ef092c0e4343fc421c323ff09d3c791fb4bc33a "test: interface_ipc.py minor fixes and cleanup"

In commit message:
> Destroy calls were being made at the end of the test instead of after
templates were no longer needed. The `template` variable was assigned twice
but only (attempted to be) destroyed once. Fix these problems by using
python `with:` blocks

Wasn't this done in the first commit fac12f7cf9ebc0b906bbb20ae126cf7e7552fdbb?

💬 rkrux commented on pull request "test: interface_ipc.py minor fixes and cleanup":
(https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2592842483)
In 6ef092c0e4343fc421c323ff09d3c791fb4bc33a "test: interface_ipc.py minor fixes and cleanup"

> A lot of result accesses like `template.result` `mining.result` were repeated
unnecessarily because variables like `template` and `mining` were assigned
response objects instead of result objects. These variables are now changed
to point directly to results.

Along the same lines:

```diff
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 90e22285e
...

💬 maflcko commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3617133344)
> > I presumed that the majority of texts were invalidated anyway
>
> This PR restores the IDs of all unchanged values so the translators should only see the changed entries.

I understand, but my point is that there is no value in trying to keep the exact old IDs.

Basically, every update invalidates all of them:

* 5c9513ece92 in 2023 invalidated down to msg 16
* be419674da6 in 2024 invalidated down to msg 61
* 656e16aa5e6 in 2025 invalidated down to msg 168

💬 darosior commented on pull request "test: Add DERSIG unit tests to script_tests.json":
(https://github.com/bitcoin/bitcoin/pull/33973#issuecomment-3617141084)
Thanks!

💬 ryanofsky commented on pull request "doc: document capnproto and libmultiprocess deps in 29.x":
(https://github.com/bitcoin/bitcoin/pull/33623#issuecomment-3617172074)
Code review 1d2251850dda2a2ae773029aeb32128b72c53f0b. Thanks for the update! This is mostly ok, but sorry my earlier suggestions were a little off, and I think this would be better to improve before merging. Would suggest:

```diff
--- a/doc/dependencies.md
+++ b/doc/dependencies.md
@@ -37,6 +37,6 @@ Bitcoin Core requires one of the following compilers.
| Python (scripts, tests) | [link](https://www.python.org) | N/A | [3.10](https://github.com/bitcoin/bitcoin/pull/30527) | No |
| [syst
...

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