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

👍 fanquake approved a pull request: "Build: Improve handling of suppressed logging in Makefiles"
(https://github.com/bitcoin/bitcoin/pull/27041#pullrequestreview-1428196233)
ACK 1b1ffbd014b931afb9435ec10911b9a7c130d3e5

💬 hebasto commented on pull request "guix: document when certain patches can be dropped":
(https://github.com/bitcoin/bitcoin/pull/27668#discussion_r1194905748)
According to https://sourceware.org/bugzilla/show_bug.cgi?id=26332:
> Fixed for glibc 2.32.

🤔 hebasto reviewed a pull request: "guix: document when certain patches can be dropped"
(https://github.com/bitcoin/bitcoin/pull/27668#pullrequestreview-1428176348)
Concept ACK.

💬 hebasto commented on pull request "guix: document when certain patches can be dropped":
(https://github.com/bitcoin/bitcoin/pull/27668#discussion_r1194915493)
As the `--with-nonshared-cflags` was introduced in glibc 2.29, does it mean we can more explicit about "newer version"?

💬 hebasto commented on pull request "guix: document when certain patches can be dropped":
(https://github.com/bitcoin/bitcoin/pull/27668#discussion_r1194925154)
https://sourceware.org/bugzilla/show_bug.cgi?id=24022:
> Target Milestone: | 2.29

💬 hebasto commented on pull request "guix: document when certain patches can be dropped":
(https://github.com/bitcoin/bitcoin/pull/27668#discussion_r1194927519)
If "2.31+" means ">=2.31", right?

⚠️ Sjors opened an issue: "Use muhash for assumeUTXO snapshot "
(https://github.com/bitcoin/bitcoin/issues/27669)
Currently `dumptxoutset` uses `hash_serialized_2` for its `txoutset_hash`. The proposed `loadtxoutset` (#27596) then checks this against the hash we hardcode in `CChainParams`.

Unfortunately the only way to check this hash is to rollback the chain to the assume-utxo snapshot height, call the (slow) `gettxoutsetinfo` and then replay to the tip.

If we used the MuHash instead then any user with `-coinstatsindex` can verify it with a simple `gettxoutsetinfo muhash HEIGHT`.

For good measure
...

💬 fanquake commented on pull request "guix: document when certain patches can be dropped":
(https://github.com/bitcoin/bitcoin/pull/27668#discussion_r1194935777)
> According to https://sourceware.org/bugzilla/show_bug.cgi?id=26332:

According to the same thread it was backported to 2.31. Which is all that matters for us.

> "2.31+" means ">=2.31", right?

Yes.