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

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

This is the other misleading `fatalError` call (see comment above)

💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226992419)
done

💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226992717)
done

💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226993131)
done

💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226993867)
It was need for the old approach, but not anymore I guess

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1227001555)
thanks 👍

🤔 LarryRuane reviewed a pull request: "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full"
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1475451194)
utACK 2a6a981e4d892c3d7196ddce54471350950affc7
These latest changes look good! I'll test later in the week.

💬 LarryRuane 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#discussion_r1227005332)
(I didn't test that this is okay)
```suggestion
const std::optional<NodeId> node_to_evict = SelectNodeToEvict(std::move(eviction_candidates), /*force=*/fuzzed_data_provider.ConsumeBool());
```

💬 TheCharlatan commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1587753463)
> The only thing I think really should be addressed is the two BlockManager::Flush calls aborting without returning errors.

I was about to open a separate PR from this branch here: https://github.com/TheCharlatan/bitcoin/tree/kernelReturnOnAbort . I think this should go in a separate PR for ease of review. I'll also provide a list of all the other non-immediate returns that should be fine to do.