💬 MarcoFalke commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1270790467)
Missing newline after self-include?

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1270799400)
Ups, done.

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1645757452)
Updated 0498ea61f4ce1d73e84f866f420a2afb209fb01c -> 05477b5566dc6f9c3d5c9609268002c0fbe1e1aa ([kernelRmUnivalue_1](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_1) -> [kernelRmUnivalue_2](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmUnivalue_1..kernelRmUnivalue_2))

* Addressed @MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1270790467), fixed include spacin
...

💬 fanquake commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1645764514)
Concept ACK. cc @theuni

💬 fanquake commented on pull request "util: Don't derive secure_allocator from std::allocator":
(https://github.com/bitcoin/bitcoin/pull/27930#issuecomment-1645766067)
@CaseyCarter did you also want to pickup the change to `zero_after_free_allocator` ?
Otherwise, we can have someone cherry-pick you're commit into a new PR, and make the additional changes.

👋 dergoegge's pull request is ready for review: "util: Type-safe transaction identifiers"
(https://github.com/bitcoin/bitcoin/pull/28107)

💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1645768047)
Ready for review

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

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