💬 joostjager commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1549871285)
Even just backporting the first two commits could be helpful, because it at least reduces the size of the patch that a miner needs to run.

📝 ajtowns converted_to_draft a pull request: "net_processing: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675)
This PR replaces the `m_recently_announced_invs` bloom filter with a simple timestamp of the last time we considered sending an INV message to a node. This saves 33kB per peer (or more if we raise the rate at which we relay transactions over the network, in which case we would need to increase the size of the bloom filter proportionally).

The philosophy here (compare with #18861 and #19109) is that we consider the rate limiting on INV messages to only be about saving bandwidth and not protect
...

💬 MarcoFalke commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195220741)
nit: Looks like this will make inbound peers be treated identical to outbound ones for the purposes of getdata, as long as mapRelay is still around. For in-mempool txs it may be possible to fix by moving the time check outside of `info_for_relay` into this function and returning a nullptr early if the tx is known to not be announced. For non-mempool txs, I don't know.

💬 MarcoFalke commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195225774)
nit: My recommendation would be to not rename the method, or if you want, to pick `GetEntryTime()` over `GetNodeTime()`, because the function doesn't return the current node time but the time of transaction entry.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1549933311)
Thanks for the review! I'm done with nits on this one, and assumeutxo in general.

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