💬 MarcoFalke commented on issue "Drop support for g++-8?":
(https://github.com/bitcoin/bitcoin/issues/27537#issuecomment-1549169559)
Closing for now. Let's continue discussion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109871 and #https://github.com/bitcoin/bitcoin/pull/27662

✅ MarcoFalke closed an issue: "Drop support for g++-8?"
(https://github.com/bitcoin/bitcoin/issues/27537)

📝 MarcoFalke opened a pull request: "ci: Remove unused errtrace trap ERR"
(https://github.com/bitcoin/bitcoin/pull/27667)
This was added in commit 069752b72613b772a9536a3e7f15fa75097f2946, presumably at a time when the functional tests wouldn't capture stderr.

Now that all tests capture and print stderr on failure, it can be removed. Reference:

* Unit tests capture via `2>&1`:

https://github.com/bitcoin/bitcoin/blob/d7700d3a26478d9b1648463c188648c7047b1c60/src/Makefile.test.include#L421

* Functional tests capture as well:

https://github.com/bitcoin/bitcoin/blob/d7700d3a26478d9b1648463c188648c7047b1c6
...

💬 Sjors commented on issue "Allow getblockfrompeer to use any peer":
(https://github.com/bitcoin/bitcoin/issues/27652#issuecomment-1549226748)
I would like this as well but I was too lazy to implement it.

The downside is that it would make the call asynchronous and potentially very long (needing `-rpcclienttimeout=0`). But this would only be the case if the user _doesn't_ specify a peer.

It would also need a progress indicator, which our RPC doesn't really offer. The debug log can of course be used for that. Or you could go all fancy and have the node track the status of the block fetch, including the ability to cancel it.

The
...

💬 MarcoFalke commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1194846344)
nit: Any reason to use r-strings when there is no need to and they don't contain any `\` chars?

🚀 fanquake merged a pull request: "walletdb: Remove unused CreateMockWalletDatabase"
(https://github.com/bitcoin/bitcoin/pull/27665)

💬 MarcoFalke commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1194857223)
q: Can you explain why it is fine to drop the translation?

💬 MarcoFalke commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1194858242)
Not sure if we want to use `Result` before it supports `<void>`, but I guess it will be less code to change in the future, compared to `std::optional<bilingual_str>`, so I guess it is fine for now.

👋 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