💬 fanquake commented on pull request "doc: document capnproto and libmultiprocess deps in 29.x":
(https://github.com/bitcoin/bitcoin/pull/33623#issuecomment-3616956249)
@ryanofsky can you circle back here?

⚠️ fanquake opened an issue: "Minor Release 30.1"
(https://github.com/bitcoin/bitcoin/issues/34015)
🚧 Milestone: https://github.com/bitcoin/bitcoin/milestone/79

Backports and other changes
- #33609
- #33997


RC 1:
- [ ] final changes #33997
- [ ] tag
- [ ] upload binaries
- [ ] announcements

<!--
RC
- [] final changes
- [] tag
- [] upload binaries
- [] announcements
-->

See [release process docs](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md) for more details. This issue can be used to track status updates.

⚠️ fanquake pinned an issue: "Minor Release 30.1"
(https://github.com/bitcoin/bitcoin/issues/34015)
🚧 Milestone: https://github.com/bitcoin/bitcoin/milestone/79

Backports and other changes
- #33609
- #33997


RC 1:
- [ ] final changes #33997
- [ ] tag
- [ ] upload binaries
- [ ] announcements

<!--
RC
- [] final changes
- [] tag
- [] upload binaries
- [] announcements
-->

See [release process docs](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md) for more details. This issue can be used to track status updates.

👍 ryanofsky approved a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3544661442)
Code review ACK fa80cca5dd193668300df891876287dacca6159e. Looks great! This is a minimal implementation of the std::expected class that may be marginally less efficient (doesn't have a void specialization, may do some unnecessary moves/copies) and lacks some features, but is usable and can be expanded, and is basically as simple as possible. I left some comments and suggestions below but none are important. I also updated the #25665 PR description to compare the `Result` and `Expected` classes.

...

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592606248)
In commit "Add util::Expected (std::expected)" (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Looks like comment needs to be updated

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592667468)
In commit "Add util::Expected (std::expected)" (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Normally T is a template parameter and internal types like this have more descriptive names. Would suggest renaming V to T to match std::expected [documentation](https://en.cppreference.com/w/cpp/utility/expected.html) and T to ValueType or similar

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592682040)
In commit "Add util::Expected (std::expected)" (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Maybe add a test for a default constructed instance (if this will be supported)

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592628772)
In commit "Add util::Expected (std::expected)" (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Is there a reason to use std::monostate{} here instead of T{}? Actual `std::expected` class seems to allow default construction

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592674188)
In commit "Add util::Expected (std::expected)" (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Might be better to use `V` instead f T here. My concern is just that with `Expected<void>` this will leak std::monostate and monostate should be a private implementation detail. Similarly for other accessors below returning `T`

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592702057)
In commit "Add util::Expected (std::expected)" (fa1eee4f46fd089d6e19b124addee7c7679585f4)

IMO std::optional and std::variant are still appropriate to use in many cases and it would nice to keep mentioning them.

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592686682)
In commit "Add util::Expected (std::expected)" (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Should these have LIFETIMEBOUND annotations?

💬 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