💬 TheBlueMatt commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587650914)
> None of these deficiencies require the full StratumV2 approach to address;

This protocol basically is a protocol designed exactly for this use-case. The fact that it comes under a "StratumV2" banner is somewhat unrelated, there's only some common framing. It's also not a substantial protocol, arguably simpler than getblocktemplate in full.

This protocol exists basically to create an alternative to getblocktemplate which solves key problems in it, and can be used with or without any other Str
...

💬 ryanofsky commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1226913429)
> FWIW, this could be de-globalized again by the interrupt class introduced in #27861.

Yes #27861 replaces the `AbortNode` function with a `KernelNotifications::fatalError` method that can access whatever state it needs through the `KernelNotifications` class without needing any globals. `AbortNode` is the only function using `g_exit_status` so it should just disappear then.

💬 ryanofsky commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1226914676)
> Does this make `InitShutdownState()` required before calling `AbortNode()` ? I suppose that could be added to the comment here.

It could be mentioned but `AbortNode` is about to be removed in https://github.com/bitcoin/bitcoin/pull/27861, so it isn't too important

💬 brunoerg commented on pull request "rest: bugfix, fix crash error when calling `/deploymentinfo`":
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226915435)
> Also just querying the previous hash seems more efficient than running generate?

For sure, and for what we'd like to test may be enough, I'm gonna change it.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1587664581)
Rebased on master, incorporated outstanding follow-ups from https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1554675156

📝 ryanofsky opened a pull request: "validation: Stricter assumeutxo error handling when renaming chainstates"
(https://github.com/bitcoin/bitcoin/pull/27862)
There are two places in assumeutxo code where it is calling `AbortNode` to trigger asynchronous shutdowns without returning errors to calling functions.

One case, in `LoadChainstate`, happens when snapshot validation succeeds, and there is an error trying to replace the background chainstate with the snapshot chainstate.

The other case, in `InvalidateCoinsDBOnDisk`, happens when snapshot validatiion fails, and there is an error trying to remove the snapshot chainstate.

In both cases the
...

💬 brunoerg commented on pull request "rest: bugfix, fix crash error when calling `/deploymentinfo`":
(https://github.com/bitcoin/bitcoin/pull/27853#issuecomment-1587672780)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/27853/files#r1226690678

Thanks @stickies-v for the review.

🤔 TheBlueMatt reviewed a pull request: "[WIP] add a stratum v2 template provider"
(https://github.com/bitcoin/bitcoin/pull/27854#pullrequestreview-1475323709)
Just a few random notes, I'm not qualified to review modern C++ anymore :joy:

💬 TheBlueMatt commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#discussion_r1226927976)
Why is this a config flag? The message future_template bit indicates that its a predicted template, and presumably we'll have a stream of both predicted and current templates on the wire, not something in the options?

💬 TheBlueMatt commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#discussion_r1226928617)
Is there a reason not to hard-code this and optional_features until we actually have a need for them? Just less code if we do.

💬 TheBlueMatt commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#discussion_r1226922409)
Why bother making this optional? Can just have a config flag.

💬 TheBlueMatt commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#discussion_r1226924386)
Shouldnt this be non-0? 1KB would match the current GBT logic. More generally, there doesn't seem to be a way to set this, somehow I'd thought it was in the exposed protocol, but if not it probably should be.

💬 dimitaracev commented on pull request "validation: Replace MinBIP9WarningHeight with MinBIP9WarningStartTime":
(https://github.com/bitcoin/bitcoin/pull/27427#issuecomment-1587686012)
Went with the approach proposed by @ajtowns . Commit 8d39803

👍 theStack approved a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1475379282)
Code-review ACK 61c569ab6069d04079a0831468eb713983919636

💬 theStack commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1226949554)
nit, could take the opportunity to change to f-strings here (and below), as [suggested in the functional test style guidelines](https://github.com/bitcoin/bitcoin/blob/361a0c00b3fdac7149c027c2c69e4db60ee5aa86/test/functional/README.md?plain=1#L39):
```suggestion
f"Node returned unexpected exit code ({return_code}) vs ({expected_ret_code}) when stopping")
```

🚀 ryanofsky merged a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708)

📝 brunoerg opened a pull request: "net: do not `break` when `addr` is not from a distinct network group"
(https://github.com/bitcoin/bitcoin/pull/27863)
When the address is from a network group we already caught,
do a `continue` and try to find another address until conditions
are met or we reach the limit (`nTries`).

💬 ccdle12 commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587728576)
> What are the reasons for not having this as a separate piece of software that calls the Bitcoin Core RPCs? From glancing at the code here it seems like `getblocktemplate` and `submitblock` would be the only needed RPCs. I've seen the discussion about separation (multi-process) in #23049 but wasn't convinced by the arguments there (e.g. [#23049 (comment)](https://github.com/bitcoin/bitcoin/pull/23049#issuecomment-926009122)). I am worried about the maintenance burden of this and would prefer it
...

💬 achow101 commented on pull request "wallet: Migrate wallets that are not in a wallet dir":
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1226982269)
It can't use `GetName()` directly since that is just the name of the wallet, not the path to where we want it to be stored as `db_dir` is in this case.

💬 achow101 commented on pull request "wallet: Migrate wallets that are not in a wallet dir":
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1226982417)
Will do if I need to push again.