👋 fanquake's pull request is ready for review: "[24.1] Final Changes"
(https://github.com/bitcoin/bitcoin/pull/27660)

🤔 fanquake reviewed a pull request: "docs: fix spelling errors"
(https://github.com/bitcoin/bitcoin/pull/27664#pullrequestreview-1428104324)
ACK e9dcac1ec7b4e00a08a943caa250c4ff00981b8b

🚀 fanquake merged a pull request: "docs: fix spelling errors"
(https://github.com/bitcoin/bitcoin/pull/27664)

💬 josibake commented on pull request "[24.1] Final Changes":
(https://github.com/bitcoin/bitcoin/pull/27660#issuecomment-1549292232)
ACK https://github.com/bitcoin/bitcoin/pull/27660/commits/89a5a416deac060fed8c21d4381c8f59da4f4187

📝 fanquake opened a pull request: "guix: document when certain patches can be dropped"
(https://github.com/bitcoin/bitcoin/pull/27668)
Additional notes for when patches can be dropped.

💬 stickies-v 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_r1193951897)
I'm not sure this is correct, it ignores the checking of `predicate`?

💬 stickies-v 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_r1194860446)
nit (and in quite a few other places):
```suggestion
const std::optional<NodeId> node_id_to_evict{SelectNodeToEvict(std::move(vEvictionCandidates), force)};
```

💬 stickies-v 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_r1194849842)
An alternative approach to the duplication of these lines is to just store all the return values in a vector, like:

```c++
std::vector<std::optional<NodeEvictionCandidate>> removed;
...
removed.emplace_back(EraseLastKElements(vEvictionCandidates, CompareNetGroupKeyed, 4));
```

and then just return the last one at the end?

💬 stickies-v 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_r1194857629)
Returning just the last removed element feels like a very bespoke and perhaps unintuitive interface to me. I know we don't currently need it, but perhaps it's worth generalizing this a bit more and returning all the removed elements instead of just the last one? For the callsite, it's trivial to keep just the last one? Just thinking out loud, perhaps this is difficult to do without an unacceptable overhead in terms of allocations etc.

💬 stickies-v 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_r1194866624)
All the `last_out` assignments are behind an `if` condition, I think this theoretically can be a zero-initialized `NodeEvictionCandidate`?

💬 stickies-v 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_r1194850365)
An alternative approach to the duplication of these lines is to just store all the return values in a vector, like:

```c++
std::vector<std::optional<NodeEvictionCandidate>> removed;
...
removed.emplace_back(EraseLastKElements(vEvictionCandidates, CompareNetGroupKeyed, 4));
```

and then just return the last one at the end?

💬 dergoegge commented on issue "--with-sanitizers=float-divide-by-zero crash with -debug=bench in Chainstate::ConnectTip":
(https://github.com/bitcoin/bitcoin/issues/27635#issuecomment-1549300086)
cc @Sjors @jonatack @theStack

👍 fanquake approved a pull request: "ci: Remove unused errtrace trap ERR"
(https://github.com/bitcoin/bitcoin/pull/27667#pullrequestreview-1428123758)
ACK fad09b703f5c6d8524a09eef771eb4525f9f3225

🚀 fanquake merged a pull request: "[24.1] Final Changes"
(https://github.com/bitcoin/bitcoin/pull/27660)

💬 MarcoFalke commented on issue "--with-sanitizers=float-divide-by-zero crash with -debug=bench in Chainstate::ConnectTip":
(https://github.com/bitcoin/bitcoin/issues/27635#issuecomment-1549322541)
I think #2416 should just be reverted. It doesn't make sense to assume that the number of all connected blocks is equal to the number of blocks read from disk. In fact, the most common case, calling ProcessNewBlock from net processing will have the block already in memory (read from the network).

An alternative might be to use a dedicated read counter for it.

💬 MarcoFalke commented on pull request "ci: Remove unused errtrace trap ERR":
(https://github.com/bitcoin/bitcoin/pull/27667#issuecomment-1549324838)
Can be tested with a diff injecting a fault:


```diff
diff --git a/src/init.cpp b/src/init.cpp
index 52c5780..95839c0 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -891,6 +891,9 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb

// Signal NODE_COMPACT_FILTERS if peerblockfilters and basic filters index are both enabled.
if (args.GetBoolArg("-peerblockfilters", DEFAULT_PEERBLOCKFILTERS)) {
+ int a{-1};
+ unsigned b =
...

💬 stickies-v 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_r1194897930)
I think it would be nice to document the effect of `force` both here and for `SelectNodeToEvict`

💬 fanquake commented on issue "Release schedule for 25.0":
(https://github.com/bitcoin/bitcoin/issues/26549#issuecomment-1549341168)
`rc2` binaries are available: https://bitcoincore.org/bin/bitcoin-core-25.0/test.rc2/.

👍 stickies-v approved a pull request: "[23.2] Final Changes"
(https://github.com/bitcoin/bitcoin/pull/27663#pullrequestreview-1428185374)
ACK 7ae937326ae009aa825c5d855b20e79ab1a7e5dc

👋 fanquake's pull request is ready for review: "[23.2] Final Changes"
(https://github.com/bitcoin/bitcoin/pull/27663)