💬 Chand-ra commented on pull request "refactor: C++20 operators":
(https://github.com/bitcoin/bitcoin/pull/33771#issuecomment-3625675526)
tACK [`48840bf`](https://github.com/bitcoin/bitcoin/commit/48840bfc2d7beeac0ddf56a3c26b243156ec8936). Built the PR and ran unit tests; everything passes.

Verified that all instances of `operator!=` have been removed and `operator<=>` replaces combined implementations of `<`, `<=`, `>`, and `>=`. (Note: PR #33772 and not this one gets rid of `operator<` in [`src/prevector.h`](https://github.com/bitcoin/bitcoin/blob/89dc82295ebd8a959fb5f6e7a116b6ce9e44e340/src/prevector.h#L458).)

💬 Sjors commented on pull request "mining: add getTransactions(ByWitnessID) IPC methods":
(https://github.com/bitcoin/bitcoin/pull/34020#issuecomment-3625705058)
Thanks, I swapped in your commit (subtree linter is still expected to fail).

> So your workaround of interpreting empty Data values as null CTransactionRef is probably the best approach for now.

One reason I have some confidence in this approach is that the Python functional tests are able to read the result. Although it interprets the empty fields as `b''` rather than `None`, that shouldn't be an issue for client developers.

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

💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2597671961)
(Tried old-school `(void)connection_client.release();` and `[[maybe_unused]] (void)connection_client.release();` to avoid the noisy scope but they don't pass clang-tidy).

💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2597679959)
The above patch + `Assume()` would be nice but I won't insist if you prefer not to, in this case.

💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2597689237)
Great suggestion about `bugprone-unused-return-value`!

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2597786362)
There is an `AllowCastToVoid` option in https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unused-return-value.html. `(void)` would be shorter than `[[maybe_unused]] auto _`. No strong opinion, but maybe it can be changed in a follow-up?

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2597791146)
Will leave for a follow-up :)

⚠️ plebhash opened an issue: "consider adding a new `interface RawTxFeed` on Mining IPC"
(https://github.com/bitcoin/bitcoin/issues/34030)
Sv2 Job Declarator Server (a.k.a. JDS) needs to keep an in-memory representation of the mempool so it can validate the custom jobs it receives via `DeclareMiningJob` message

currently, SRI JDS polls some RPCs against Bitcoin Core, but polling is rather suboptimal

on https://github.com/stratum-mining/sv2-apps/issues/26 we considered switching to an approach where we subscribe for the `rawtx` ZMQ feed... JDS would have some task that's solely dedicated to consuming this feed

however, [I hear ZM
...

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