📝 theuni opened a pull request: "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM"
(https://github.com/bitcoin/bitcoin/pull/27676)
This (I believe) resolves the last of the blockers for [switching us away from cctools and instead using out-of-the-box llvm and lld](https://github.com/bitcoin/bitcoin/pull/21778) for building Darwin binaries.

For now, we continue building with a pre-packaged llvm and cctools, but after this PR the clang+lld combo should just work for anyone trying it. Additionally after this PR, the new runtime `fixup_chains` behavior will be in-use, as ld64 uses it as well.

The commits may seem unrelate
...

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#issuecomment-1549941663)
> This is probably not an issue, but it seems worth noting that a change of the underlying mock implementation had such an impact on the performance of legacy wallet loading.

It changed the underlying db used for this benchmark from the BDB in-memory database to the MockableDatabase. However I think the goal of this benchmark is to measure `LoadWallet` specifically and not necessarily the database engine, so I don't think it particularly matters.

💬 theuni 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-1549943895)
Ping @fanquake and @hebasto . There's not much happening here technically, but a few big-ish conceptual changes.

Also, we may want to go one step further in configure and do an actual compile check to verify that the minimum version is set to >=11.0 as we now have that as an assumption.

💬 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-1549945366)
Concept ACK - note that this also needs a change to `clang-toolchain-11` in Guix, which should be available without a time-machine change.

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1195390653)
The [docs](https://sqlite.org/c3ref/clear_bindings.html) don't say what the return values may be. I think SQLite itself will also log an error using the `ErrorLogCallback` that we during initialization.

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