💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1270816338)
Done

💬 hebasto commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1645806444)
Putting new parsing helpers into the `libbitcoin_common` library is a bit questionable for me.

I was expecting to see them in the `libbitcoin_util`. However, it is not possible with the suggested approach as the `univalue_helpers.cpp` depends on `SIGHASH_DEFAULT`.

💬 TheCharlatan commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1645952454)
Concept ACK

💬 MarcoFalke commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#issuecomment-1645976254)
Did quick check to see how long it would take to find my injected bug with libfuzzer:

* `-use_value_profile=1`:

* `-use_value_profile=0`:

🤔 ryanofsky reviewed a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1541470201)
Rebased 28a954c7034077ac3a45083dd5e2b5cdb4d4cdde -> 40f09de73e61e7ae62d6639a49b7c7ac48d514d9 ([`pr/bresult2.33`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.33) -> [`pr/bresult2.34`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.34), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.33-rebase..pr/bresult2.34)) due to conflict with various PR and making many suggested changes from review #25722 (which is based on this PR)

re: https://github.com/bitcoin/
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1270866000)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1234264247

> Can you add an example for when the Result error type in a chain of function calls remains the same, but the value type changes? Is there a better way to do this conversion than manually constructing the result again (like below)?

Yes, there is definitely a better way to pass along errors and warnings from one result to another without calling `ErrorString` and flattening them. The `util::Result` constructor will mo
...

👍 MarcoFalke approved a pull request: "Fuzz: a more efficient descriptor parsing target"
(https://github.com/bitcoin/bitcoin/pull/27888#pullrequestreview-1541508735)
nice, lgtm ACK 84dee4fe690e08a5adaad1c78530666da07075d8 🦄

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: nice, lgtm ACK 84de
...

💬 MarcoFalke commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#discussion_r1270892632)
```suggestion
// The actual, descriptor string to be returned.
```

pretty sure it is not valid

💬 MarcoFalke commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#discussion_r1270890988)
Nit: If you make `key_data` `std::byte`, you can avoid the c-style cast:


```diff
diff --git a/src/test/fuzz/descriptor_parse.cpp b/src/test/fuzz/descriptor_parse.cpp
index 1d2b672260..84dc005575 100644
--- a/src/test/fuzz/descriptor_parse.cpp
+++ b/src/test/fuzz/descriptor_parse.cpp
@@ -36,16 +36,16 @@ public:
//! When initializing the target, populate the list of keys.
void Init() {
// The data to use as a private key or a seed for an xprv.
- uint8_t key_
...

🤔 ryanofsky reviewed a pull request: "refactor: Use util::Result class for wallet loading"
(https://github.com/bitcoin/bitcoin/pull/25722#pullrequestreview-1409772815)
Rebased f7d4451b98014176c083adc02bdf59e6dd2b9355 -> 17891a95ab5873aa978a5bf5ed8985ee1513e728 ([`pr/bresult-load.16`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-load.16) -> [`pr/bresult-load.17`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-load.17), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult-load.16-rebase..pr/bresult-load.17) making most of the suggested changes. I'm working on some more changes that should use `ResultPtr` from #26022 to simplify s
...

💬 ryanofsky commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1182978622)
re: https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1167981120

> Mayby explicitly mark the move-assignment as delete

Thanks, that changes error message from `error: object of type 'util::Result<int>' cannot be assigned because its copy assignment operator is implicitly deleted` to `error: overload resolution selected deleted operator=`, which should be more helpful now because a comment at the deleted `operator=` declaration explains why the operator is deleted and recommends usi
...

💬 ryanofsky commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1182977847)
re: https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1167821634

> It took me a while to figure out what's going on here with the recursive `Construct` calls and the lambda that's passed along. How about instead of passing a lambda along use a `bool` template argument like so:

Thanks, that's much better! Took suggestion jsut renaming argument to "Failure"

💬 ryanofsky commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1182977232)
re: https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1167765212

> Maybe it would be a bit clearer to use `(O&& other)` here?

Sure, changed this.

💬 ryanofsky commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1182978470)
re: https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1167765212

> Same here, I'd say `void MoveConstruct(Result<OT, OF>&& other)` is a bit clearer

Sure, changed

💬 ryanofsky commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1182978708)
re: https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1167981676

> nit: I don't think the `if (!src.m_info->errors.empty())` check is necessary.

It's not strictly necessary, but it avoids allocating an ErrorInfo object in the common case where there are no errors or warnings. Since this is called whenever result objects are moved, it's good to avoid unnecessary allocations here.

💬 ryanofsky commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1182977109)
> I'd add a check somewhere to ensure that the size of the Result is as claimed in the commit message, something like that:
>
> ```c++
> static_assert(sizeof(util::Result<int>) == sizeof(void*)*2);
> static_assert(sizeof(util::Result<void>) == sizeof(void*));
> ```

Thanks, added to result_tests.cpp

💬 ryanofsky commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1182978551)
re: https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1174414223

> move constructors should usually be `noexcept`, but then all methods it uses also have to be `noexcept`.

It would be possible to add a constructor that move constructs `Result<T, F>` from `Result<OT, OF>` and is noexcept. For this to work, move constructing `T` from `OT` would have to be noexcept, and `F` and `OF` types would need to be identical so `unique_ptr<ErrorInfo<F>>` could be moved without any allocations.
...

💬 ryanofsky commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1182978948)
re: https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1174415841

> Could be just `return util::Error();`

Thanks, simplified

💬 ryanofsky commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1269813522)
re: https://github.com/bitcoin/bitcoin/pull/25722#discussion_r1174297928

I agree with all of this and want to do more work to address it.

I think by default types convertible to bool should be disallowed with `Result`. I think `Result<bool>` or `Result<optional<T>>` types are inherently ambiguous and code accessing those return values would be fragile and prone to errors from dereferencing incorrectly.

But disallowing all types convertible to bool would go to far because being able to r
...

💬 darosior commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#discussion_r1270901466)
It's not avoiding them, it's hiding them behind `UCharCast`? :p

It boils down to choosing between casting to `uint8_t` or to `std::byte`, and i went for the option with less casts.