💬 Sjors commented on pull request "test: interface_ipc.py minor fixes and cleanup":
(https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2598844209)
In fac12f7cf9ebc0b906bbb20ae126cf7e7552fdbb _test: fix interface_ipc.py template destruction_:

I think the original intention here was to demonstrate that `generate` interrupts an ongoing `waitNext()`.

Now it only shows that `waitNext()` returns immediately if the tip has changes before calling. Though perhaps it always did that by accident.

💬 Sjors commented on pull request "test: interface_ipc.py minor fixes and cleanup":
(https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2598858386)
In https://github.com/bitcoin/bitcoin/commit/fac12f7cf9ebc0b906bbb20ae126cf7e7552fdbb _test: fix interface_ipc.py template destruction_: similar to my comment above, this only shows that `waitNext()` returns immediately if fees have gone up before the call, but the original intent was to show that a fee increase causes the wait to be interrupted.

💬 Sjors commented on pull request "test: interface_ipc.py minor fixes and cleanup":
(https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2598816528)
In fac12f7cf9ebc0b906bbb20ae126cf7e7552fdbb _test: fix interface_ipc.py template destruction_: `await stack.enter_async_context(destroying((` is hard to read, so maybe move it into a helper:

```diff
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 3d28bba136..e01c753d24 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -96,6 +96,14 @@ class IPCInterfaceTest(BitcoinTestFramework):
tx.deserialize(coinbase
...

💬 ryanofsky commented on issue "consider adding a new `interface RawTxFeed` on Mining IPC":
(https://github.com/bitcoin/bitcoin/issues/34030#issuecomment-3627326527)
> 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 a delay. So this might be quite involved.

This is true for the mining interface, but the other interfaces in #29409 and #10102 do used streamed notifications. The wallet uses `Chain.handleNotifications` to start receiving notifications and transactions and blocks:

https://github.com/bitcoin/bitcoin/blob/d9efd1e49d1df154970b6a60229eedde3ba7cffe/src/ipc/capnp/chain.
...

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