💬 t-bast commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1887226712)
Another thing to note about this attack is that the attacker would be losing funds in the process if the attacked node simply spends the revoked commitments outputs (instead of trying to replace it). Whenever one side publishes a revoked commitment, they will lose any funds that they had in the channel (which is at least their channel reserve, thus 1% of the channel). So the attacked node would pay a large on-chain fee, but would also earn the funds that the attacker loses. In the worst case sce
...

💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1448932594)
updated in cfdbcd19b32fd63954d7947dcc639aef291fb6b2

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