👍 alfonsoromanz approved a pull request: "log mempool loading progress"
(https://github.com/bitcoin/bitcoin/pull/29227#pullrequestreview-1815715513)
Code Review ACK https://github.com/bitcoin/bitcoin/pull/29227/commits/6c3ca0cac38f3b9e307cd0522ba9e3b7a2bd05a5

I have not executed the code but I spent time reviewing the loop and the conditions within it, using your example provided in the PR description (401 txs). Looks good to me

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1887299001)
Rebased on top of adding CMake into all CIs, the newer upnpc / libnatmp / dropping libtool commits, and added a new commit to deduplicate usage of `-mmaxosx-version-min`.

💬 jamesob commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1448969580)
@stickies-v it's not about system resources; it's about allow the end user to articulate what they see. Not a big deal either way, but I think our current scheme is a little wonky if you compare it to more widely deployed systems e.g. the python logging module or log4j.

📝 Sjors opened a pull request: "Make (Read/Write)BinaryFile work with char vector"
(https://github.com/bitcoin/bitcoin/pull/29229)
ReadBinaryFile and WriteBinaryFile current work with `std::string`. This commit adds support for `std::vector<unsigned char>>`.

This is used in #28983 to store the static key for the Template Provider, in a manner very similar to how we store the Tor v3 and i2p key. However it made no sense to me to store a `CKey` as plain text.

It uses a template and leverages the fact that both `std::string` and `std::vector<unsigned char>>` have an `insert()` method that can take a char array.

The `u
...

💬 glozow commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#issuecomment-1887387038)
reACK 0eebd6fe7d01ddc7f6b7f13a6ed6e705c7aeae4e, only change is the doc suggestion

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1887392642)
cc @vasild

💬 maflcko commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1449027121)
If you change the return type, why not use `std::optional`? Otherwise, it seems better to keep it untouched?

💬 maflcko commented on issue "ci: failure in `wallet_basic.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1887419702)
Added milestone, because this seems like a blocker?

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1449031987)
I had to change it because AFAIK you can't differentiate functions only by their return type. It's also more readable imo, no more `->first` ... `->second`.

`std::optional<T>` won't work, unless someone can explain how...

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1449035810)
> However, functions can not be overloaded if they differ only in the return type.

https://www.geeksforgeeks.org/function-overloading-and-return-type-in-cpp/

💬 maflcko commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1449048489)
> `std::optional<T>` won't work, unless someone can explain how...

What about this:

```cpp
#include <vector>
#include <optional>
#include <string>
#include <cstdint>

template<class T>
std::optional<T> ReadBinaryFile()
{
return {};
}

int main()
{
auto a{ReadBinaryFile<std::string>()};
auto b{ReadBinaryFile<std::vector<uint8_t>>()};
}

👋 fanquake's pull request is ready for review: "[25.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/28768)

📝 jonatack opened a pull request: "doc: update -loglevel help following merge of PR 28318"
(https://github.com/bitcoin/bitcoin/pull/29230)
Commit [`ab34dc6` (#28318)](https://github.com/bitcoin/bitcoin/pull/28318/commits/ab34dc6012351e7b8aab871dd9d2b38ade1cd9bc) was an incomplete version of [`118c756` (#25203)](https://github.com/bitcoin/bitcoin/pull/25203/commits/118c7567f62df2b882877590f232242d7c627a05) from the Severity-based logging parent PR.

Add the missing text to update the `-loglevel` help doc.

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1887460716)
Added `extern template ...` to the header to prevent "implicit instantiation of undefined template" error on the "no wallet, libbitcoinkernel" CI (https://cirrus-ci.com/task/6254856449556480) (doesn't happen on my machine running macOS clang 15).

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1449063112)
I'll give that a try tomorrow.

📝 ajtowns opened a pull request: "Update to new logging API"
(https://github.com/bitcoin/bitcoin/pull/29231)
Updates various logging calls to the new API from #28318. All commits are scripted diffs, so should be easy to update if needed, and also easy to reuse the scripts to update other code if needed when rebasing it after this is merged. Changes all uses of `LogPrintLevel()` where the level is hardcoded, and changes all `LogPrintf` and `LogPrint` uses in init.cpp.

💬 ajtowns commented on pull request "Update to new logging API":
(https://github.com/bitcoin/bitcoin/pull/29231#issuecomment-1887484404)
Depending on how many conflicts drahtbot collects, maybe this should be targeting a merge around 27.0's feature freeze?

Many of the log statements in init.cpp could be done at a better level, eg `LogPrintf("Error: Out of memory. Terminating.\n");` should obviously be logging at the "error" level, not the "info" level. In order to keep this PR scripted-diff only, I won't fix that here; but it can fixed in an independent PR either before or after this PR is merged.

💬 jonatack commented on pull request "Update to new logging API":
(https://github.com/bitcoin/bitcoin/pull/29231#issuecomment-1887499306)
I don't think updating all the macros is a good idea until the API is finished.

📝 theuni opened a pull request: "depends: remove dependency on Darwin libtool"
(https://github.com/bitcoin/bitcoin/pull/29232)
Split out of #21778.

cctools' libtool (and llvm's clone) are unnecessary these days, and were only used (unnecessarily) in miniupnpc and libnatpnp. Both of those projects now have CMake support which eliminates their use.

This PR installs cmake as a base package for c-i which will be necessary in our near future anyway, switches those builds to use CMake, then removes our libtool machinery.

🚀 fanquake merged a pull request: "ci: move CMake into base packages"
(https://github.com/bitcoin/bitcoin/pull/29225)

👍 fanquake approved a pull request: "ci, iwyu: Drop backported mappings"
(https://github.com/bitcoin/bitcoin/pull/29186#pullrequestreview-1815973973)
ACK a395218d8cdffba375006590edca58c844126b16