💬 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

🚀 fanquake merged a pull request: "ci, iwyu: Drop backported mappings"
(https://github.com/bitcoin/bitcoin/pull/29186)

💬 fanquake commented on pull request "depends: remove dependency on Darwin libtool":
(https://github.com/bitcoin/bitcoin/pull/29232#issuecomment-1887516591)
Looks good. Have tested this in the LLD PR. Will ACK after rebase and adding the PIC fix for libnatpmp.

💬 jonatack commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1449103019)
ab34dc6012351 The loglevel help wasn't updated with this change (note that this commit is an incomplete copy of [`118c756` (#25203)](https://github.com/bitcoin/bitcoin/pull/25203/commits/118c7567f62df2b882877590f232242d7c627a05) from the Severity-based logging parent PR). Fixed in #29230.

💬 theuni commented on pull request "depends: remove dependency on Darwin libtool":
(https://github.com/bitcoin/bitcoin/pull/29232#issuecomment-1887518447)
Rebased on master and fixed up pic.

💬 achow101 commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#issuecomment-1887519279)
ACK 997b9a73e5166b4244f7c5b4fe144d524f3005f4