💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1195399114)
I don't think `upper_bound` or `equal_range` will work. They will find the upper bound to be the first item that is greater than our prefix, but any record with the right prefix and is longer than the prefix will be greater than it, so we end up actually only getting the record(s) that exactly match the prefix.

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1195406721)
Done

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1195406793)
Done

💬 hebasto commented on pull request "bench: Benchmark all `SHA256` implementations that are available on the system":
(https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1549995815)
Reworked according to @martinus comments.

💬 hebasto commented on pull request "bench: Benchmark all `SHA256` implementations that are available on the system":
(https://github.com/bitcoin/bitcoin/pull/27598#discussion_r1195418857)
@martinus

Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1549995815).

💬 hebasto commented on pull request "bench: Benchmark all `SHA256` implementations that are available on the system":
(https://github.com/bitcoin/bitcoin/pull/27598#discussion_r1195419757)
Thanks! That change has been [dropped](https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1549995815).

👍 fanquake approved a pull request: "qt, test: Add missed header"
(https://github.com/bitcoin-core/gui/pull/729#pullrequestreview-1428977049)
ACK 36e2d51b8f6bb0125911c831ba2b6fd022fca708

💬 hebasto commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1550004076)
Concept ACK.

💬 fanquake commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1550008836)
> check to verify that the minimum version is set to >=11.0 as we now have that as an assumption.

Note that you'll also need to update our minimum version check in:
https://github.com/bitcoin/bitcoin/blob/904631e0fc00ac9c8a03d1ce226d071bf88c00db/contrib/devtools/symbol-check.py#L235

💬 fanquake commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1550017272)
> no, I don't want to spend time on discussions on whether some change breaks user space or not.

@mzumsande fair enough. I don't quite understand what "user space" means, when it has been used in comments here (and in other PRs), as we are not the Linux Kernel, but I assume it's a somewhat less direct way of saying backwards compatibility/continuity?

In any case, as far as I'm aware, we offer no special "backwards compatiblity", or other, guarantees for our CLI tools, and I don't really th
...

💬 Sjors commented on issue "Allow getblockfrompeer to use any peer":
(https://github.com/bitcoin/bitcoin/issues/27652#issuecomment-1550028698)
If it fails you have to try again. It would presumably auto-pick the same peer, so you'd have to revert to manually picking one. Agree that's not ideal.

👍 ryanofsky approved a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1428804291)
Partial code review ACK 155f8de067db5d7b9ebf6de0b30729143cc9c87f for everything except the last commit (82ef31a5eb6094a94e05e44a658072f7afa08a47).

- [X] 2830b75b91b2b77814f5099f392bf0117b652d37 kernel: Add notification interface (1/9)
- [X] 234795b02db05eb4deaa20e514fe9ad7f8e9d7eb kernel: Add notifyHeaderTip to notifications (2/9)
- [X] dba41283095175620e78a5d72282bd6770b12040 kernel: Add notifyProgress to notifications (3/9)
- [X] 4b810032b09af76dd71aa605863da9ca2e6bb1c6 kernel: Add notif
...

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1195307708)
In commit "kernel: Add fatalError to notifications" (155f8de067db5d7b9ebf6de0b30729143cc9c87f)

It seems like this message should be called "notify fatal" to be consistent with other notification methods.

Alternately, it seems like word "notify" in all the method names is pretty name is pretty redundant and could be dropped. The methods are always called with `notifications.` or `GetNotifications()` prefixes so there is not really a context where repeating the word "notify" clarifies anythi
...

💬 furszy commented on issue "Allow getblockfrompeer to use any peer":
(https://github.com/bitcoin/bitcoin/issues/27652#issuecomment-1550066620)
> If it fails you have to try again. It would presumably auto-pick the same peer, so you'd have to revert to manually picking one. Agree that's not ideal.

If the block request timeouts (fails), we disconnect the peer. So, if someone wants to use this as is now, it's ok to do another RPC call after 10 minutes, when the remote peer is no longer in the `getpeerinfo` response.

Still, not ideal as it involves some extra RPC calls. Better to keep moving on providing a better tracking mechanism.

💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1195482361)
This seems fairly easy to refactor later.

💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1195472070)
058fecdc7d: could still move `if (can_serve` out of the loop to reduce brackets.

⚠️ sdaftuar opened an issue: "Proposal for a new mempool design"
(https://github.com/bitcoin/bitcoin/issues/27677)
@sipa and I gave a presentation to some other developers recently about some work we've been doing to redesign how the mempool works, and I wanted to share a writeup outlining our thoughts with a broader audience to further the discussion. I've also attached a PDF of slides that go along with this: [Reinventing the Mempool.pdf](https://github.com/bitcoin/bitcoin/files/11490409/Reinventing.the.Mempool.pdf).

--------

# Summary

The current mempool is primarily designed around maintaining
...

🤔 Sjors reviewed a pull request: "assumeutxo: net_processing changes"
(https://github.com/bitcoin/bitcoin/pull/24008#pullrequestreview-1429084243)
Happy with the improvements in ac9adf012925c770dfe523c5b57451f313cc8be5.

I wrote:

> The first commit tends to confuses me

Could use some thoughts on that comment (from anyone really).

@sdaftuar wrote:

> The first commit "validation: introduce `ChainstateManager::GetChainstateForNewBlock"` seems like something we don't conceptually need.

I'm pretty close to wrapping my head around it, so would be OK with merging it once I do _and then_ refactor it. Avoiding the complexity in the
...

💬 mzumsande commented on pull request "p2p: skip netgroup diversity follow-up":
(https://github.com/bitcoin/bitcoin/pull/27467#issuecomment-1550098252)
Code Review ACK 11bb31c1c43b5da36ca8509b5747abfb3278ffcd

This fixes an outdated doc, so it's an improvement.

> The naming is now misleading.

I don't think the previous naming was too bad: #27343 added an explanatory comment after all. If anything, it was much more misleading before #27343 when there was no comment, because the name `setConnected` doesn't imply "netgroups of outbound peers from all networks" any more than it implies "netgroups of ipv4/ipv6 outbound peers" - after all we
...

💬 MarcoFalke commented on pull request "ci: Run iwyu on all src files":
(https://github.com/bitcoin/bitcoin/pull/27571#discussion_r1195497290)
because it crashes. See:


```
# python3 "/include-what-you-use/iwyu_tool.py" -p . ./src/wallet/walletutil.cpp -j 9 -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="$PWD/contrib/devtools/iwyu/bitcoin.core.imp"
In file included from wallet/walletutil.cpp:5:
In file included from ./wallet/walletutil.h:8:
In file included from ./script/descriptor.h:8:
In file included from ./outputtype.h:9:
In file included from ./script/signingprovider.h:10:
In file included from ./key.h:10:
In file include
...

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1550107951)
Re https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1428804291
> I think the last commit could just be dropped, or I don't understand what problem it is solving.

The rationale for the commit was to preserve `bitcoin-chainstate` shutting down when a fatal error is encountered. I thought this through again, and I agree that this does not make a lot of sense. The function where this fatal error encountered should always be bubbling it up anyway, so this is not needed. Dropping th
...