💬 maflcko commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3616982740)
> > shasum of the full content can be used as an id for the translation string
>
> @maflcko we could of course do that, but it would likely invalidate every single text in the next release. We could of course do it step-by-step and only add hashes for the new entries (instead of max-id + 1, as I did in the script here). Note that hashes would prohibit same-text-with-different-translations, e.g. `Clear` could mean "delete" or "agree", based on context.

In that case, the context should be ha
...

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