💬 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

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