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

📝 hebasto converted_to_draft a pull request: "Use `int32_t` type for most transaction size/weight values"
(https://github.com/bitcoin/bitcoin/pull/23962)
From bitcoin/bitcoin#23957 which has been incorporated into this PR:
> A file-wide suppression is problematic because it will wave through future violations, potentially bugs.
>
> Fix that by using per-statement casts.
>
> This refactor doesn't change behavior because the now explicit casts were previously done implicitly.
>
> Similar to commit 8b5a4de904b414fb3a818732cd0a2c90b91bc275

👍 stickies-v approved a pull request: "rest: bugfix, fix crash error when calling `/deploymentinfo`"
(https://github.com/bitcoin/bitcoin/pull/27853#pullrequestreview-1475484670)
ACK 7d452d826a7056411077b870efc3872bb2fa45e4