💬 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

📝 mzumsande opened a pull request: "test: fix intermittent failure in p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/pull/27864)
Fixes #27860

The problem was that the `inv` for `tx_b` would sometimes be sent out to the inbound peer after the `notfound`, so that the `notfound` wouldn't be the last message anymore and the test fails.

```
node0 2023-06-12T12:48:24.903204Z [msghand] [net.cpp:2856] [PushMessage] [net] sending notfound (73 bytes) peer=1
node0 2023-06-12T12:48:24.903423Z [msghand] [net.cpp:2856] [PushMessage] [net] sending inv (37 bytes) peer=1
```

Fix this by waiting for the `inv` before contin
...