💬 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

💬 mercie-ux commented on pull request "test: interface_ipc.py minor fixes and cleanup":
(https://github.com/bitcoin/bitcoin/pull/34003#issuecomment-3619834301)
Concept ACK #34003

I ran the interface_ipc.py test locally and it passes. The PR cleans up the test by fixing broken checks, missing await calls, and variable naming.

I’m new to PR reviews, but from what I can tell, the changes seem correct and improve readability. I don’t see any issues with the changes.

💬 maflcko commented on pull request "scripted-diff: Use LogInfo over LogPrintf":
(https://github.com/bitcoin/bitcoin/pull/29641#issuecomment-3619835819)
> nit: would be nice to update the scripted-diff to also remove trailing newlines if that's not a huge pita

Thx, I can take a look, if I have to re-touch.

💬 arejula27 commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594702214)
Agree, I started with your review, and now I am doing #25665, the explanation I suggested feels more suitable in the other one; however, I would like you to consider adding **directly** as I mentioned, as you already did some small text corrections on `results.h`

⚠️ maflcko opened an issue: "ci: The bitcoincore.org depends fallback is missing sqlite-autoconf-3500400.tar.gz"
(https://github.com/bitcoin/bitcoin/issues/34021)
The depends fallback is down again?

```
$ curl --show-headers 'https://bitcoincore.org/depends-sources/sqlite-autoconf-3500400.tar.gz'
HTTP/2 404
server: nginx
date: Fri, 28 Nov 2025 09:21:07 GMT
content-type: text/html
content-length: 146
strict-transport-security: max-age=63072000; includeSubDomains; preload
```

_Originally posted by @maflcko in https://github.com/bitcoin/bitcoin/issues/32655#issuecomment-3588516475_

💬 arejula27 commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3619893884)
@maflcko So you think it will be interesting to follow the `std::expected` class methods and add:
- [value_or](https://en.cppreference.com/w/cpp/utility/expected/value_or.html)
- [error_or](https://en.cppreference.com/w/cpp/utility/expected/error_or.html)

I think it will be valuable and will give more flexibility in error handling cases

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3620016424)
> @maflcko Do you think it will be interesting to follow the `std::expected` class methods and add:
>
> * [value_or](https://en.cppreference.com/w/cpp/utility/expected/value_or.html)
>
> * [error_or](https://en.cppreference.com/w/cpp/utility/expected/error_or.html)
>
>
> I think it will be valuable and will give more flexibility in error handling cases

I don't see a valid use-case for `error_or`, but i guess value_or may come in handy possibly in the future. Other than that
...

💬 stickies-v commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594785458)
I haven't used `std::{un}expected` yet in practice, so I'm not really familiar with real life use cases yet. From research, one use case that seems plausible is to write a handler for failure cases (e.g. to log and then abort). Templating that function on `std::unexpected<E>` seems sensible? There are of course plenty of other ways to write that handler, but my point is that because it's public people _might_ do it, and then that'll break drop-in replacement. Regardless, it probably will be a ra
...

📝 sedited opened a pull request: "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders"
(https://github.com/bitcoin/bitcoin/pull/34022)
It is always set to true, so there is no point in keeping it.

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594802701)
> Regardless, it probably will be a rare occurence, and it should be an easy fix when we switch to `std::expected`, so I'm not worried either way.

Same. Will leave as-is for now, but happy to push any diff, or review a follow-up, if there is need.

💬 arejula27 commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3620230189)
This PR was started in 2022, before the C++23 standard introduced `std::expected`. I agree with @holdinator that this PR attempts to solve two different issues:
1. The error-handling workflow
2. The ability to merge results, which is useful for high-level functions (e.g., loading a wallet, connecting a block) that perform many internal operations and may want to return a cumulative status instead of a single one-shot value

I agree that these are interesting problems to address. However, I b
...

💬 sedited commented on pull request "p2p: avoid traversing blocks (twice) during IBD":
(https://github.com/bitcoin/bitcoin/pull/32730#issuecomment-3620304717)
> That part was merged with https://github.com/bitcoin/bitcoin/pull/32827

Ah right, forgot about that. Should have mentioned this directly, but the change I was looking for is in the txdownloadman. Was thinking that if this is rebased, we could add a patch for avoiding the additional iteration and filter calculation there in this pull request.