💬 adamandrews1 commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-3627356662)
I've addressed all nits and rebased. Thank you to each of you who left thoughtful commentary.

💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3627362057)
I've pulled in a commit from @laanwj that is a partial reversion of https://github.com/gcc-mirror/gcc/commit/1d82fc2e6824bf83159389729c31a942f7b91b04.

💬 sipa commented on pull request "doc: corrected lockunspent rpc quoting":
(https://github.com/bitcoin/bitcoin/pull/31275#issuecomment-3627367982)
Weak Concept NACK.

Given how broken the examples are, I think we should fix them throughout and add testing to prevent regressions, or get rid of them entirely.

👍 pablomartin4btc approved a pull request: "netbase: Remove "tor" as a network specification"
(https://github.com/bitcoin/bitcoin/pull/34031#pullrequestreview-3552560740)
ACK 7efb18c38150344ca6f7efbcd8441792a2ea921f

💬 Sjors commented on issue "consider adding a new `interface RawTxFeed` on Mining IPC":
(https://github.com/bitcoin/bitcoin/issues/34030#issuecomment-3627477785)
A pool's JDS processes proposed templates from multiple miners, which will have divergent mempools. So until a block is mined, some templates may still have transaction that the pool itself threw out of its mempool.

In order for the JDS to prune its pseudo-mempool it could track which inputs are spent in the new block and (recursively) delete entries that spend from it.

💬 darosior commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2599070189)
Should we explicitly document "this function will not check pow!" then?

💬 sipa commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r2599076399)
It's a bit strange to use an imperative form in release notes, given that it's describing changes that have been made already.

Suggestion:

> CLI `-addrinfo` now returns the full set of known addresses. In previous versions (v22.0 - v30.0) the set of returned addresses was filtered for quality and recency. This was changed since it does not match the logic for selecting peers to connect to, which does not filter.

---

Also worth mentioning that the CLI feature is now imcompatible with
...

💬 sipa commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-3627526721)
Concept/Approach ACK. I think it's fine to break compatibility with unmaintained node versions, given that there is a good error message for this case.

💬 sedited commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2599118974)
Do you mean "this function will not check for a minimum proof of work threshold"? But as I said [here](https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2596217297), I think the potential for misuse at this point is low. I'd gladly add a text if you want to suggest one though.

💬 pablomartin4btc commented on pull request "argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)":
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3627591458)
-<ins>_**Updates**_</ins>:
- Added @w0xlt's [fix](https://github.com/bitcoin/bitcoin/pull/33540#pullrequestreview-3490620415) on long (double dash --) options that [worked](https://github.com/bitcoin/bitcoin/pull/33540#pullrequestreview-3490254189) on `master`, adding a new test for it (the test would pass in `master` but fail without @w0xlt's changes).
- Refactored both `ArgsManager::ProcessOptionKey` (new function added in this PR) and `ArgsManager::ParseParameters` in separated commits maki
...

👍 ryanofsky approved a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3552578391)
Code review ACK faa23738fc2576e412edb04a4004fab537a3098e, only adding `value_or` implementation and `CheckedReturnTypes` clang-tidy commit since last review.

I think this is ok to merge as-is but I left some suggestions about `value_or`, and I think the `[[maybe_unused]] auto _{...}` uses in the second commit are pretty ugly and it would be nice if they could be changed to use `(void)` instead.

It could also be nice make the other suggested improvements if there is not a hurry:

- Avoidi
...

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2599049418)
In commit "Add util::Expected (std::expected)" (fa114be27b17ed32c1d9a7106f313a0df8755fa2)

Not very important, but since value_or methods are template methods it would make sense for them to return `T` instead of `ValueType` to avoid returning std::monostate.

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2599097104)
re: https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592820086

> You are right, but I can't see this going wrong in practise, so I'll leave this as-is for now and postpone to a follow-up. (The solution would probably be to specialize, but that requires writing a lot more code)

I think hodlinator's solution adding requires clauses might avoid the need to specialize https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592454740

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2599070287)
In commit "Add util::Expected (std::expected)" (fa114be27b17ed32c1d9a7106f313a0df8755fa2)

Not very important, but it's not consistent for `std::move(result).value_or(...)` to move from the result when `std::move(result).value()` does not move from it.

The upstream `std::expected` seems to move in both cases by defining a `value() &&` overload. (If you added that overload this line could also be simplified changing `std::move(value())` to `value()`)

Would suggest making `value()` and `va
...

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2599018832)
re: https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2597786362

Very much agree with having this code use `(void)` not `[[maybe_unused]] auto _{}`. I don't understand why clang-tidy would encourage the latter.

🤔 furszy reviewed a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31668#pullrequestreview-3552684128)
Instead of using `timestamp=never`, which is not really descriptive. What about using `new`?

💬 furszy commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2599106467)
In cda5cb3ec27ac9bf15b947eff0c5cc87cc835c48:

Instead of returning -1, could change the return value to optional, and here return `std::nullopt`.

💬 furszy commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2599095627)
In cda5cb3ec27ac9bf15b947eff0c5cc87cc835c48:

This change should be in a different commit.

💬 furszy commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2599145016)
In https://github.com/bitcoin/bitcoin/commit/cda5cb3ec27ac9bf15b947eff0c5cc87cc835c48:

Need to unload the wallets after using them; otherwise they stay loaded for other tests in this file as well.
Also, you can do this in fewer lines of code with a loop. As far as I can see, the only per-round variants are the timestamp, the balance, and the tx list.

💬 maflcko commented on pull request "refactor: C++20 operators":
(https://github.com/bitcoin/bitcoin/pull/33771#issuecomment-3627658898)
review ACK 48840bfc2d7beeac0ddf56a3c26b243156ec8936 🌖

<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: review ACK 48840bfc2d7b
...

💬 stickies-v commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3627705445)
ACK faa23738fc2576e412edb04a4004fab537a3098e

Current code seems fine to merge as-is, but I would prefer addressing more of the outstanding comments (from myself and other reviewers) first.