💬 l0rinc commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3271990036)
reACK 794a17186d3019713d29213bedd866baa1c81378 - the only change since last ACK was the span nits being applied

💬 hodlinator commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2334562200)
Hasn't become intuitive to me yet that the first parameter to `subspan()` is an offset and not a size, but appreciate not having to repeat the identifier. Taken in latest push, including the other transform of the same kind.

Thanks for the review!

👍 brunoerg approved a pull request: "wallet: warn against accidental unsafe older() import"
(https://github.com/bitcoin/bitcoin/pull/33135#pullrequestreview-3202996953)
ACK fe06b92429206aee66d33c843195235f4bb27181 - This new approach of just warning is better than the previous one, nice! I haven't reviewed the code in depth, just did a light code review but tested the behavior in practice and worked fine.

Also, I ran a mutation analysis on these changes and the only uncaught mutant is:
```diff
// Traverse miniscript tree for unsafe use of older()
miniscript::ForEachNode(*m_node, [&](const miniscript::Node<uint32_t>& node) {
-
...

💬 l0rinc commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2334579755)
yeah, that confused me as well - if you need to touch again, we could add a [name hint](https://en.cppreference.com/w/cpp/container/span/subspan) to it:
```C++
std::span{first_chain}.subspan(/*offset=*/1)
```

💬 mzumsande commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3272066604)
> Concept ACK, but I think unconditional logging in the ctor is not the right approach, suggested alternative:

I like the move of the Sqlite log - it seems suficient to log the sqlite version just once instead for each wallet load.

In the latest push I just removed the second log:

I doubt that logging the full path in the success case adds much, while creating anonymization efforts in case of sharing logs publicly. The wallet name is already being logged multiple times:

```
2025-09
...

📝 sipa opened a pull request: "txgraph: use enum Level instead of bool main_only"
(https://github.com/bitcoin/bitcoin/pull/33354)
Part of #30289. Inspired by https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2331387778.

Since there has been more than one case in the development of #28676 of calling a `TxGraph` function without correctly setting the `bool main_only` argument that many of its interface functions have, make these mandatory and explicit, using an `enum class Level`.

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2334636695)
See https://github.com/bitcoin/bitcoin/pull/33354.

💬 achow101 commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2334647787)
Yes, the effect it has is on the named parameter. `rollback` cannot actually be passed as a positional parameter since it is actually part of the options object which would have to be passed in position 2. However, if `rollback` is passed by name, then we need to apply the conversion.

Order does matter here in that the `options` parameter needs to come first so that the positional is not interpreted as a string.

💬 davidgumberg commented on pull request "Avoid pathological QT text/markdown behavior...":
(https://github.com/bitcoin-core/gui/pull/886#issuecomment-3272271535)
CI Failure is unrelated to this PR: https://github.com/bitcoin/bitcoin/issues/33345

🤔 l0rinc requested changes to a pull request: "node: optimize CBlockIndexWorkComparator"
(https://github.com/bitcoin/bitcoin/pull/33334#pullrequestreview-3202982209)
Concept ACK - please consider my suggestion

💬 l0rinc commented on pull request "node: optimize CBlockIndexWorkComparator":
(https://github.com/bitcoin/bitcoin/pull/33334#discussion_r2334837666)
do these need to stay pointers (in which case we should hanlde `nullptr`) ir

💬 l0rinc commented on pull request "node: optimize CBlockIndexWorkComparator":
(https://github.com/bitcoin/bitcoin/pull/33334#discussion_r2334808680)
All this does is compare by 3 parameters - wouldn't it be simpler to use `std::tie` for lexicographical comparison instead?
```suggestion
bool CBlockIndexWorkComparator::operator()(const CBlockIndex* pa, const CBlockIndex* pb) const
{
// First sort by most total work (descending), then by earliest activatable time (ascending), then by pointer value (ascending)
return std::tie(pa->nChainWork, pb->nSequenceId, pb) < std::tie(pb->nChainWork, pa->nSequenceId, pa);
}
```

*Note that `pa` & `p
...

💬 achow101 commented on pull request "wallet, refactor: Remove Legacy check and error":
(https://github.com/bitcoin/bitcoin/pull/33082#issuecomment-3272332928)
ACK d3c5e47391e2f158001e3e199d625852c7f18998

💬 stickies-v commented on pull request "cmake: make missing Python interpreter behaviour more explicit":
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2334869398)
That's interesting. The https://cmake.org/cmake/help/v3.14/module/FindPython.html docs for `Python::Interpreter` state "Target defined if component Interpreter is found.", so I'm not sure why that's not failing for you.

Since the behaviour of relying on `Python3::Interpreter` is not changed in this PR, I suspect you won't get the "Minimum required Python not found." warning (at the very end of the configure step) that's removed in this PR, since that relies on the same mechanism? Correct?


...

🚀 achow101 merged a pull request: "wallet, refactor: Remove Legacy check and error"
(https://github.com/bitcoin/bitcoin/pull/33082)

💬 achow101 commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#issuecomment-3272399795)
ACK 4d4789dffad55b96f1cb96b718cc6923f5344454

🚀 achow101 merged a pull request: "net: Prevent node from binding to the same `CService`"
(https://github.com/bitcoin/bitcoin/pull/33231)

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2334276827)
I like the improved portability of this new approach, but the use of macros makes things significantly more cumbersome for FFI bindings. E.g. using ctypes / clang2py, I have to manually define the enum values, which is error-prone.

An alternative approach that keeps the portability and FFI compatibility could be a `btck_<Enum>` `btck_<Enum>Value` pairing, where the public API only uses `btck_ChainType`:

```cpp
typedef uint8_t btck_ChainType;
enum btck_ChainTypeValue {
btck_ChainType
...

💬 Raimo33 commented on pull request "node: optimize CBlockIndexWorkComparator":
(https://github.com/bitcoin/bitcoin/pull/33334#discussion_r2334980346)
I experimented with `std::tie` before. whether it is more readable is debatable. I'd say they're the same in terms of readability (one might not be familiar with std::tie). I ran the benchmarks on my x86 with gcc laptop (not the same machine as my original benchmarks) and got different results from yours.

'''shell
taskset -c 0 ./bin/bench_bitcoin --filter="(CheckBlockIndex|LoadExternalBlockFile)" --min-time=10000
'''

> Master:

| ns/op | op/s | err% |
...

💬 BenWestgate commented on issue "Linux download needs installation instructions":
(https://github.com/bitcoin/bitcoin/issues/32097#issuecomment-3272601034)
If we added to the tarball:
```
./share/applications/bitcoin-qt.desktop
./share/icons/bitcoin128.png
./share/mime/packages/x-scheme-handler-bitcoin.xml
./share/doc/bitcoin-core/README.md
./share/doc/bitcoin-core/bitcoin.conf
```
Then a local install can be performed by:
`tar xzf bitcoin-29.1-x86_64-linux-gnu.tar.gz --strip-components=1 --directory=$HOME/.local`
and a system wide install by:
`sudo tar xzf bitcoin-29.1-x86_64-linux-gnu.tar.gz --strip-components=1 --directory=/usr/local`

We have t
...