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

💬 ryanofsky commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1226706890)
In commit "util: Add SignalInterrupt class and use in shutdown.cpp" (3ce961f38df80843d01190589146088906819ea3)

Doesn't look like any optional objects are used here so probably not necessary to add

💬 ryanofsky commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1226757927)
In commit "util: Add SignalInterrupt class and use in shutdown.cpp" (3ce961f38df80843d01190589146088906819ea3)

I think it would be safer to do `m_flag = true` here and not use `memory_order_release`. The previous code was doing `fRequestShutdown = true;` and it would be nice to just keep behavior unchanged in this commit and avoid causing potential bugs.

💬 ryanofsky commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1226742797)
In commit "util: Add SignalInterrupt class and use in shutdown.cpp" (3ce961f38df80843d01190589146088906819ea3)

Would suggest replacing "sleep" with "wait" everywhere in this commit. The original `SignalInterrupt` class from https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1580779869 had a `sleep` method to be consistent with `CThreadInterrupt::sleep_for`. But now that the class no longer depends on `CThreadInterrupt`, better to go with post-c++11 terminology and call it `wait`.


...

💬 ryanofsky commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1226772621)
In commit "util: Add SignalInterrupt class and use in shutdown.cpp" (3ce961f38df80843d01190589146088906819ea3)

I think it would be safer to `return m_flag` and avoid using `memory_order_acquire`. The previous code was just doing `return fRequestShutdown` which uses a stricter memory ordering.

💬 ryanofsky commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1226800991)
In commit "kernel: Add fatalError method to notifications" (d60ab3b5099d3a348f3122b5d3207ac12f4a751c)

This option is unused so would be good to drop

💬 ryanofsky commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1226776471)
In commit "util: Add SignalInterrupt class and use in shutdown.cpp" (3ce961f38df80843d01190589146088906819ea3)

Previous code was `fRequestShutdown = false`, would use `m_flag = false` to avoid changing behavior instead of adding `memory_order_release`

💬 ryanofsky commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1226847296)
In commit "kernel: Add fatalError method to notifications" (d60ab3b5099d3a348f3122b5d3207ac12f4a751c)

This is calling `fatalError` and then continuing without returning an error or raising an exception, so I think the new code here is more confusing than the current code (even if strictly speaking it is a refactoring).

We could potentially change this code to treat failing flush as a normal error that gets returned to the caller. But I think if we are going to do that, it would be better t
...

💬 ryanofsky commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1226862636)
In commit "kernel: Add fatalError method to notifications" (d60ab3b5099d3a348f3122b5d3207ac12f4a751c)

No error is returned after this `fatalError` call, but this looks like a bug not directly related to this PR. Opened #27862 to address it.