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

💬 l0rinc commented on pull request "node: optimize CBlockIndexWorkComparator":
(https://github.com/bitcoin/bitcoin/pull/33334#discussion_r2335088222)
> but I genuinely don't know how this could be less than 2 branches

let's find out - can you create a godbold reproducer with latest compilers?

🤔 w0xlt reviewed a pull request: "RFC: coins: warn on oversized `-dbcache`"
(https://github.com/bitcoin/bitcoin/pull/33333#pullrequestreview-3203834761)
Approach ACK

I’d suggest:
. Logging the `else` condition as well, for consistency.
. Showing the percentage of RAM being used.

Example:

```diff
@@ -1736,9 +1736,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
const auto [index_cache_sizes, kernel_cache_sizes] = CalculateCacheSizes(args, g_enabled_filter_types.size());
if (auto total_ram{GetTotalRAM()}) {
size_t cap{(*total_ram < 2048_MiB) ? DEFAULT_KERNEL_CACHE : (*total_ra
...

💬 davidgumberg commented on issue "ci: derived LLVM version too new":
(https://github.com/bitcoin/bitcoin/issues/33345#issuecomment-3272864274)
In the apt.llvm.org packages `clang --version` includes the build commit hash, so we could do the following, although it is quite uqly:

```bash
git init llvm-project
cd llvm-project
git remote add origin https://github.com/llvm/llvm-project.git
LLVM_SHORT_HASH=$( clang --version | sed --silent 's@.*clang version [0-9.]* (++[0-9].*+\([0-9a-f]*\)-.*@\1@p')
${CI_RETRY_EXE} curl -s -H "Accept: application/vnd.github.v3+json" "https://api.github.com/repos/llvm/llvm-project/c
...

💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3273275956)
Just tried this on my rpi4b server which keeps enabling signature validation after a reindex. Restarting it with only the args `./build/bin/bitcoind -dbcache=5000 -datadir=$DATA_DIR -assumevalid=0000000000000000000087564caa77e7b3f29d0464256c04d5539e43663f8698` still shows heavy signature validation (based on `perf top`).
<img width="1000" alt="image" src="https://github.com/user-attachments/assets/b29b6bdd-6be2-4b05-98e3-82d60989c52c" />

💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3273277122)
For more context, even just restarting the node that was reindexed before (and is at around 200k blocks) with `-assumevalid=0000000000000000000087564caa77e7b3f29d0464256c04d5539e43663f8698` (block 914005), it still starts with signature verification enabled - using 70% of CPU when it should have been disabled in the first place, see:
<img width="1000" alt="image" src="https://github.com/user-attachments/assets/e6094caf-c0d3-40dd-8edd-92cbe76c3d81" />
This PR makes it at least obvious that some
...

💬 Sjors commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#issuecomment-3273503141)
@brunoerg thanks, I added your test against false positive warnings for `after()`, which passes.

📝 jalateras opened a pull request: "doc: Add documentation explaining different build types"
(https://github.com/bitcoin/bitcoin/pull/33355)
## Summary

This PR adds documentation explaining the different types of Bitcoin Core binaries available for download, addressing the confusion expressed in issue #33316.

## Problem

Users downloading Bitcoin Core encounter various file types (unsigned, signed, debug, codesigning) without clear documentation explaining what each version means or which one they should use.

## Solution

Added a new `doc/build-types.md` file that comprehensively explains:
- **Unsigned builds**: Raw build outputs
...

💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3273516645)
The "Unknown named parameter" [ci failure](https://github.com/bitcoin/bitcoin/actions/runs/17593154154/job/49978783184?pr=29675) might be a case of #33230 / #32821?

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2335830807)
I'm not sure I am following on the suggestion. That seems to revert to what we had before? How is the typedef used? The macros tripping up code generators is really unfortunate though. Maybe we can use C99's `static const` instead and hope that the compiler would usually optimize these away?