💬 ryanofsky commented on issue "mptest hangs, when run in a loop":
(https://github.com/bitcoin/bitcoin/issues/34014#issuecomment-3616984522)
> so i guess it is the gcc+clang mixing 🤷

Oh, fun

💬 l0rinc commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3616995793)
> 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.

💬 hodlinator commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3617021123)
At this point I would prefer `util::Expected` be merged first (#34006).

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.

Maybe it would be clearer to use something like `util::Expected<V, util::ErrorReport>` directly in "user code". With...
```C++
class ErrorReport
{
std::unique_ptr<detail::Messages> m_data;
...
```
...to fulfill the wish to keep
...

💬 fanquake commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#issuecomment-3617039726)
A commit has landed upstream: https://codeberg.org/guix/guix/commit/bd2edc9e435402b48fd201b56ab486151512717a.

💬 theStack commented on pull request "wallet: check for `agg_pub` validity in MuSig2 signing":
(https://github.com/bitcoin/bitcoin/pull/34010#issuecomment-3617044635)
The line leading to the crash involves the following two constructors
https://github.com/bitcoin/bitcoin/blob/b8e66b901d563d7ba6042e6be54d7e48b9fffc83/src/pubkey.h#L256-L260
where cryptographic validity of the pubkey isn't checked at any point, so I don't think this PR fixes anything?

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

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

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

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