💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2597809962)
Oh, hm... it would be great to not need the scopes any longer. On the other hand `(void)expression` is more cryptic than `[[maybe_unused]]`.

Slight preference for enabling `AllowCastToVoid` if you retouch. Maybe if I notice these scopes becoming annoyingly frequent I would follow up.

🤔 rkrux reviewed a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3551060765)
light Code Review ACK faa23738fc2576e412edb04a4004fab537a3098e

Onboard with the intent of using (util/std)::Expected for the use cases mentioned in the PR description.

🚀 fanquake merged a pull request: "guix: reduce allowed exported symbols"
(https://github.com/bitcoin/bitcoin/pull/33950)

💬 Sjors commented on issue "consider adding a new `interface RawTxFeed` on Mining IPC":
(https://github.com/bitcoin/bitcoin/issues/34030#issuecomment-3626035899)
> JDS polls some RPCs against Bitcoin Core

Can you enumerate which RPC calls you're polling? And briefly explain why.

For streaming changes to the mempool, we'd probably want to introduce a whole new `Mempool` interface, since there are other use cases (wallets, block explorers). And such an interface might as well have methods to submit new transaction (packages).

We haven't used IPC for ZMQ-style streaming yet. In the Mining interface you call a method and get a result, immediately or after
...

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2597905915)
> Slight preference for enabling `AllowCastToVoid` if you retouch. Maybe if I notice these scopes becoming annoyingly frequent I would follow up.

Yeah, I tend to agree and will probably do this in a follow-up.

In Rust, one can use several `let _` in the same scope. However, in C++, using several `auto _` in the same scope is only possible in C++26, in which case the `[[maybe_unused]]` is redundant anyway.

So I think allowing `(void)` for now, until C++26, is probably best.

Ref: https
...

🤔 hodlinator reviewed a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3550963091)
Reviewed 262f4dfe691b12171a31c9219ba33f1fe55f67a7

Love how elegant 71c7b747091fce045dbf25e4e0cfc0e6a4ad6435 turned out!

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2597781433)
meganit: Prefer curly braces by convention in modern code
```suggestion
return util::Unexpected{ReadRawError::IO};
```

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2597756415)
nit in 7802c1f9ca09b5669b8400e96baeadc238fa9df2 "blockstorage: return an error code from `ReadRawBlock()`": The commit message should probably note that the benchmark performance decreases due to no longer reusing a `vector` with an unchanging capacity (but that it mirrors our production code behavior).

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2597862560)
nit: Might be a more appropriate error code?
```suggestion
return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Bad block part offset/size %d/%d for %s", block_part->first, block_part->second, hashStr));
```

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2597880475)
nit: Preference for the following in favor of smaller diff:
```C++
if (!tx_verbosity) {
return RESTERR(req, HTTP_BAD_REQUEST, "JSON output is not supported for this request type");
}
```

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2597817911)
The example in the developer notes linked above doesn't include the `default` case, but rather puts the `assert(false)` after the `switch`. This means the compiler will start warning if `enum` values are missing from the `switch`. (Only applicable when all cases `return`, which they do in this part of the code).

Please adjust the commits accordingly.

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2597870863)
`BadPartRange` should only happen when a block_part is specified, so I would prefer an `assert()` over this contrived error condition:

```C++
case node::ReadRawError::BadPartRange:
assert(block_part);
return RESTERR(req, HTTP_NOT_FOUND, strprintf("Bad block part offset/size %d/%d for %s", block_part->first, block_part->second, hashStr));
```

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2597901554)
(Don't see the value in `const` here as we return it directly, but the noise isn't too bad).

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2597911471)
Would be nice to include something like this in unit tests.

```diff
--- a/src/test/blockmanager_tests.cpp
+++ b/src/test/blockmanager_tests.cpp
@@ -188,6 +188,9 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_part_error, TestChain100Setup)
}};

expect_part_error(0, 0);
+ expect_part_error(std::numeric_limits<size_t>::max(), 1);
+ expect_part_error(0, std::numeric_limits<size_t>::max());
+ expect_part_error(std::numeric_limits<size_t>::max(), std::numeric_limits
...

✅ fanquake closed an issue: "depends: Freetype and xcbproto version in depends are too new"
(https://github.com/bitcoin/bitcoin/issues/29977)

🚀 fanquake merged a pull request: "depends: update freetype and document remaining `bitcoin-qt` runtime libs"
(https://github.com/bitcoin/bitcoin/pull/33952)

💬 Sjors commented on pull request "mining: getCoinbase() returns struct instead of raw tx":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3626113055)
Trivial rebase after #29641.

💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2597954811)
> Yeah, I tend to agree and will probably do this in a follow-up.

:heart:

> In Rust, one can use several let _ in the same scope.

Well, Rust practically encourages shadowing variables. :vomiting_face:

> However, in C++, using several auto _ in the same scope is only possible in C++26, in which case the [[maybe_unused]] is redundant anyway.

Nice!

💬 fanquake commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#issuecomment-3626116939)
Probably also related: https://codeberg.org/guix/guix/pulls/4737

💬 Sjors commented on pull request "refactor: disentangle miner startup defaults from runtime options":
(https://github.com/bitcoin/bitcoin/pull/33966#issuecomment-3626123055)
Holding off on trivial #29641 rebase until #33965 is merged or needs changes.