💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2739574777)
@willcl-ark

Could you take a look at this PR?

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2005061988)
:+1: I think it makes sense for TxGraph to take care of all the topology checks and feerate comparison decisions, which includes finding the best txs (block building), finding the worst txs (eviction) and finding good subsets when limits would otherwise be exceeded (reorgs, "rbf"/"carve out" cases maybe). So this doesn't feel like a design break at all to me, but rather pretty much exactly parallel to `GetWorstMainChunk()` -- "here's a bunch of txs that you want to remove".

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2739603182)
Rebased and fix comment by @yancyribbens

💬 ismaelsadeeq commented on pull request "test: Add test coverage for rpcwhitelistdefault when unset":
(https://github.com/bitcoin/bitcoin/pull/32079#discussion_r2005076794)
There is no unintended white space in the current list hence it is redundant not needed see my comment here https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1927220284

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005082494)
The dependencies required to build `depends` are listed in [`/depends/README.md`](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md), and the second commit already updates them to include the `ninja-build` package.

As for mentioning the CRB repository, [`/depends/README.md`](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md) does not have a CentOS-specific section.

🤔 ismaelsadeeq reviewed a pull request: "test: Add test coverage for rpcwhitelistdefault when unset"
(https://github.com/bitcoin/bitcoin/pull/32079#pullrequestreview-2701745045)
Thanks for following up!

cb5e26d6f2dafa150d87545890afaeeceb5c610a is doing multiple things.

Can you make the commits atomic just as you did in the PR description?

💬 musaHaruna commented on pull request "test: Add test coverage for rpcwhitelistdefault when unset":
(https://github.com/bitcoin/bitcoin/pull/32079#discussion_r2005097804)
Thanks for the clarification! Yeah the previous is making it redundant. Hence please @naiyoma ignore my nit

🤔 ajtowns reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2701725334)
ReACK 63b8f96e67f9ad649070a231532d48fb6c3573e4 . Needs a rebase anyway though.

The main/staging changes read better to me fwiw. The level stuff in the locators could be ungeneralised as well I suppose, but that doesn't seem likely to be much of an improvement.

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2005075531)
Could tidy this up:

```c++
if (!m_main_clusterset.empty()) return;
if (m_staging_clusterset.has_value() && !m_staging_clusterset->empty()) return;

bool ClusterSet::empty() {
return m_deps_to_add.empty() && m_to_remove.empty() && m_removed.empty();
}
```

💬 l0rinc commented on pull request "leveldb: pull upstream C++23 changes":
(https://github.com/bitcoin/bitcoin/pull/31766#issuecomment-2739679633)
`reindex-chainstate` finished successfully

<details>
<summary>Details</summary>

```bash
COMMITS="5e934ebc82ca239cd98eed1689f7cdc2c672a9ae"; \
STOP_HEIGHT=888000; DBCACHE=5000; \
C_COMPILER=gcc; CXX_COMPILER=g++; \
git fetch --all -q && (for c in $COMMITS; do git log -1 --oneline $c || exit 1; done) && \
hyperfine \
--export-json "/mnt/my_storage/rdx-${COMMITS// /-}-${STOP_HEIGHT}-${DBCACHE}-${C_COMPILER}.json" \
--runs 1 \
--parameter-list COMMIT ${COMMITS// /,} \
--prepa
...

💬 l0rinc commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r2005143296)
Related to https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1926634436 - it's more uniform now

💬 l0rinc commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#issuecomment-2739703340)
ACK c6eca6f3961d780d6a395c3346e44be1a0072f08

💬 Sjors commented on issue "Linux download needs installation instructions":
(https://github.com/bitcoin/bitcoin/issues/32097#issuecomment-2739711382)
That's a useful reference. But the concern there is about security, depending on operating system libraries. Here I'm just worried about the practically of installing. As linux desktop gets more user friendly, I expect fewer people to know how to figure out which libraries they're missing and how to install them.

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2005190423)
I was thinking that this might be interesting to add to the getchainstates rpc call.

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2005207380)
"acceptable quality" is more than just a topologically valid order.
It refers to a linearization that may not be optimal but is the best we can achieve for that cluster within resource limitations.

💬 Sjors commented on pull request "cmake: Add `NO_CACHE_IF_FAILED` option for checking linker flags":
(https://github.com/bitcoin/bitcoin/pull/32027#issuecomment-2739809540)
This seems useful, but I'm having a hard time reproducing the issue, or something similar, on macOS, because XCode 15.2 ships with most of the sanitizers.

I can hit an earlier error:

```
cmake -B build -DSANITIZERS=memory
...
-- Performing Test CXX_SUPPORTS__FSANITIZE_MEMORY - Failed
CMake Error at CMakeLists.txt:362 (message):
Compiler did not accept requested flags.
```

But then I'd probably have to recompile LLVM.

Might be easier to reproduce on a linux distro, like the or
...

💬 Sjors commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#issuecomment-2739873757)
ACK d190f0facc8379da7610d7161e532d57c6a7eb96

I verified that the commits here match 095801b8999851b10e43567389cd293fd7957497 and 45d439dab13153a3b570957a9eab63e3e6274611 in #31375 where I reviewed them.

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2005273964)
In 101a8ee3280e50c3272a80939b46a67faca838e4 "txgraph: (optimization) avoid per-group vectors for clusters & dependencies"
```suggestion
/** Where the dependencies for this cluster group in ClusterSet::m_deps_to_add start. */

```

👍 pablomartin4btc approved a pull request: "refactor: Remove redundant and confusing calls to IsArgSet"
(https://github.com/bitcoin/bitcoin/pull/31896#pullrequestreview-2702084258)
re-ACK fa0cf040baa74b161b4b00ddea95bfb5aac0bb53

Since my last review: outdated TODO [comment](https://github.com/bitcoin/bitcoin/pull/31896#discussion_r1997633914) was removed (on new commit) - no impact.

💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2739919199)
re-ACK a9935d4dae8847d0549ec51ddc4349c7e8c61763

Briefly tested another guix build on Windows.