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

💬 pinheadmz commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1587736907)
@vasild @mzumsande Added a commit that restricts the aggressive eviction to new net permission `ForceInbound` which leaves `NoBan` unchanged.

👍 ryanofsky approved a pull request: "kernel: Add interrupt class"
(https://github.com/bitcoin/bitcoin/pull/27861#pullrequestreview-1474986679)
Code review ACK d60ab3b5099d3a348f3122b5d3207ac12f4a751c. Overall looks great! Feel free to ignore most of comments I left. The only thing I think really should be addressed is the two `BlockManager::Flush` calls aborting without returning errors. I think this is confusing and potentially could lead to bugs, but since the changes here don't actually affect the way the code run in practice, it doesn't need to block the PR.