💬 arejula27 commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2593700975)
I wouldn’t modify `src/net_processing.h` in this PR. Even though it’s a good showcase of the improvement, there are other functions that still use std::optional for errors, for example, line 828 in `src/netbase.cpp`. Updating only these functions and not the others feels arbitrary and unrelated to the scope of this PR. I suggest moving these changes to a separate PR and making the update more exhaustive.

🤔 Crypt-iQ reviewed a pull request: "net: make m_nodes_mutex non-recursive"
(https://github.com/bitcoin/bitcoin/pull/32394#pullrequestreview-3546352523)
crACK 1a9274f391df0465c17b6b6664988af70910e39b

Needs a small rebase since #33956 added 2 annotations.

💬 Crypt-iQ commented on pull request "net: make m_nodes_mutex non-recursive":
(https://github.com/bitcoin/bitcoin/pull/32394#discussion_r2593910296)
I was initially concerned that maybe `lNodesAnnouncingHeaderAndIDs.front()` could have been removed from in between releasing the lock and acquiring the lock again. It seems fine since if we do not find `lNodesAnnouncingHeaderAndIDs`, we don't need to set `m_bip152_highbandwidth_to` false and we still remove it from the list. There was some previous discussion about the locking [here](https://github.com/bitcoin/bitcoin/pull/22147#issuecomment-855904105).

💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2594031239)
I like this. The only downside is that moving the functions from `Chainstate` to `TestChainstateManager` is less flexible if ever we wanted to have multiple chainstates.

I did a similar same approach also for blockmanager etc., so that now we only need to move a few things from `private` to `protected` in the production part of the code.

💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#issuecomment-3618658872)
Thanks for the reviews - with the most recent push I addressed https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2571742382 - will address the comments by @Crypt-iQ a bit later.

💬 billymcbip commented on pull request "test: Add DERSIG unit tests to script_tests.json":
(https://github.com/bitcoin/bitcoin/pull/33973#discussion_r2594168890)
That's exactly right, but I think there was a mistake in the test. It should use `OP_1` for the second signature instead of `OP_0`, because `IsValidSignatureEncoding` is not run for empty signatures.

💬 billymcbip commented on pull request "test: Add DERSIG unit tests to script_tests.json":
(https://github.com/bitcoin/bitcoin/pull/33973#issuecomment-3618890756)
Updated `CHECKMULTISIG NOT` tests:
- Removed a comment referencing a test file that no longer exists.
- Improved test descriptions.
- Added negative cases to make the tests easier to understand.
- Changed invalid signature push from `OP_0` to `OP_1` because `IsValidSignatureEncoding` isn't called for empty sigs.

💬 billymcbip commented on pull request "test: Add DERSIG unit tests to script_tests.json":
(https://github.com/bitcoin/bitcoin/pull/33973#issuecomment-3618909039)
On second thought, I think it's better to simply replace the `STRICTENC` flag with `DERSIG` when applicable instead of creating new tests. It makes the test file easier to parse.

💬 fjahr commented on pull request "wallet: check for `agg_pub` validity in MuSig2 signing":
(https://github.com/bitcoin/bitcoin/pull/34010#issuecomment-3618988487)
Could you add a simple test for this?

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594622794)
> I wouldn’t modify `src/net_processing.h` in this PR. Even though it’s a good showcase of the improvement, there are other functions that still use `std::optional` for errors, for example, line 828 in `src/netbase.cpp`.

No, it is not. This is a valid and intended use of `std::optional`:

```
std::optional<CNetAddr> addr{LookupHost(str_addr, /*fAllowLookup=*/false)};
if (addr.has_value()) {
```

I'll leave as-is for now. Also, I'll keep touching the file, so that a valid and in
...

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594623314)
> However, Result in our codebase isn’t limited to reporting errors to end users. As @ryanofsky noted in PR #25665, there are broader use cases. One strong example is:
>
> > Result may also be a better choice than std::expected when returning pointer values. (Followup #26022 adds a ResultPtr specialization that avoids double dereferencing.)

Thx, this is a good point. However, I think modifying the Result and Expected docs for ResultPtr is unrelated to this pull and can be done in the pull
...

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594623940)
I know, and this was intentional. I can't imagine a real code snippet in this code-base that uses `.err` or `.error()`. The only use-case of Unexpected is to serve as a marker, which Expected constructor to pick internally. Am I missing something?

💬 Chand-ra commented on pull request "fuzz: Add a test case for `ParseByteUnits()`":
(https://github.com/bitcoin/bitcoin/pull/34017#issuecomment-3619725059)
> `ParseByteUnits` is not publicly exposed, i.e. it doesn't handle untrusted inputs, and I would not consider adding fuzz tests for this type of function as a priority.

Makes perfect sense. But I wonder why fuzz tests for these internal utilities (like `ToUpper()`, `ToLower()`, etc.) were introduced in the first place? Is it being thorough just for the sake for being thorough?

> As the in-repo fuzz tests are pretty saturated, it can be hard to spot valuable areas to improve (especially if
...

💬 Sjors commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594671525)
`FetchBlock()` is only used by the `getblockfrompeer` RPC (which I introduced in #20295), so it's nice and fairly isolated example to use.

Typically we gradually replace old code with more modern patterns while editing something in the same area, so the other examples will follow later.

👍 sedited approved a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3547326027)
ACK fa76a6620012fb738639d8fd7ce17b185bfd376c

💬 Chand-ra commented on pull request "fuzz: wallet: add target for tx scanning":
(https://github.com/bitcoin/bitcoin/pull/32993#issuecomment-3619800359)
tACK [747d094](https://github.com/bitcoin/bitcoin/pull/32993/commits/747d0942413b26f0260126dfecf4f960a5e53a38)

The target runs without crashing.

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594681281)
I see your point, but exactly the same scenario can happen without this pull, when an `util::Result` is replaced by `std::expected`. (And the inverse can happen when going the other way)

I don't feel strongly, but I think it is minimally safer to assert here, because for our code-base it is a programming logic error to call `value` when a value does not exist. So it seems minimally better to be able to detect those cases more easily while we have Expected (before switching to `std::expected`)
...

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594681480)
Thanks. Very nice, pushed a commit to do that.

💬 maflcko commented on pull request "fuzz: Add a test case for `ParseByteUnits()`":
(https://github.com/bitcoin/bitcoin/pull/34017#issuecomment-3619815099)
lgtm ACK 57b888ce0ebdeb34d866fd1511052fd740cc5ab8

👍 rkrux approved a pull request: "scripted-diff: Use LogInfo over LogPrintf"
(https://github.com/bitcoin/bitcoin/pull/29641#pullrequestreview-3547344071)
lgtm ACK fa4395dffd432b999002dfd24eb6f8d7384fbcbe

Thanks for the cleanup, best to remove the deprecated `LogPrintf` alias.

> It wastes review cycles, because reviewers sometimes point out that it is deprecated.

I reviewed #34008 where exactly this^ happened: https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2592395736