💬 hebasto commented on pull request "cmake: Install `bitcoin` manpage":
(https://github.com/bitcoin/bitcoin/pull/33407#issuecomment-3298763878)
> > This PR is an amendment to #33348.
>
> Not sure this is related to #33348. It should have been part of #31375?

The PR description has been updated.

💬 fanquake commented on pull request "WIP: Backport Cirrus runners to 29.x":
(https://github.com/bitcoin/bitcoin/pull/33403#issuecomment-3298778883)
Looking at the bottom of https://github.com/bitcoin/bitcoin/actions/runs/17765745116?pr=33403, I see
```bash
x86_64-w64-mingw32-executables-17765745116 ... 4.12 KB
```
Looking in the tarball, all the executables are 200 bytes of shell script:
```bash
cat x86_64-w64-mingw32-executables-17765745116/bin/bench_bitcoin.exe
#!/usr/bin/env bash
( wine "/home/admin/actions-runner/_work/_temp/build/bin/bench_bitcoin.exe_orig" "$@" ) || ( sleep 1 && wine "/home/admin/actions-runner/_wo
...

💬 hebasto commented on issue "oss-fuzz: build failing":
(https://github.com/bitcoin/bitcoin/issues/33366#issuecomment-3298802545)
> [google/oss-fuzz#13987](https://github.com/google/oss-fuzz/pull/13987)

... or github.com/google/oss-fuzz/pull/14015.

📝 Sjors converted_to_draft a pull request: "mining: add applySolution() to interface"
(https://github.com/bitcoin/bitcoin/pull/33374)
Unlike the `submitblock` RPC which takes a fully serialized block, when a block solution is received via IPC the client only provides the nonce, coinbase and a few other fields. It may not have all the information it needs to reconstruct the block. This makes debugging difficult when the block is invalid.

This commit adds `applySolution()` which returns the reconstructed block and can be used by the client for debugging. It's assigned `@11` in the `.cap` file, and will not break existing clie
...

💬 hMsats commented on issue "bitcoind 29.0 much slower than 28.0 on my system: cause found":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-3298839070)
@martinatime An I/O issue is consistent with what I found and wrote above (could have used `-blocksdir` and ` -datadir`):

> The -reindex has finished but I had to put the chainstate and indexes directories on the internal disk of the laptop I
> used because the sync became painfully slow after a while with those directories on the external SSD (USB 3.0).
> After putting those directories on the internal SSD, syncing became 16 times faster!

6 days is acceptable for a RPi with an USB SSD

💬 purpleKarrot commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3298876285)
Code review ACK. I very much welcome the reduced complexity in the CMake configuration. Thanks for working on this!

I haven't tested actually running the scripts yet.

💬 willcl-ark commented on pull request "WIP: Backport Cirrus runners to 29.x":
(https://github.com/bitcoin/bitcoin/pull/33403#issuecomment-3298895762)
> Looking at the bottom of [bitcoin/bitcoin/actions/runs/17765745116?pr=33403](https://github.com/bitcoin/bitcoin/actions/runs/17765745116?pr=33403), I see
>
> ```shell
> x86_64-w64-mingw32-executables-17765745116 ... 4.12 KB
> ```
>
> Looking in the tarball, all the executables are 200 bytes of shell script:
>
> ```shell
> cat x86_64-w64-mingw32-executables-17765745116/bin/bench_bitcoin.exe
> #!/usr/bin/env bash
> ( wine "/home/admin/actions-runner/_work/_temp/build/bi
...

🤔 rkrux reviewed a pull request: "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys"
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3230143685)
Reviewed commit 7c085554dce336eb1597ab2fc482163876a49270 with the aim of deduplicating MuSig2 signing flow.

💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2352608586)
In https://github.com/bitcoin/bitcoin/commit/7c085554dce336eb1597ab2fc482163876a49270 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"

I have noticed there is lot of duplication in MuSig2 signing flows done in the key path and script path spending. A common `SignMuSig2` function can help in removing duplication and risks of introducing discrepancies in the two spending paths - I checked that the tests pass. Also, should make the review easier by reducing diff.

<details open
...

💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2352617109)
This^ diff also uses the function from this earlier suggestion https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2336962962.

💬 ismaelsadeeq commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2352347809)
In "txgraph: Make level of Cluster implicit (optimization)" 22f5fe833fcf3da6a4180d181ff68d6da34f40db

Should also add coverage for `GetLevel`
```diff
diff --git a/src/txgraph.cpp b/src/txgraph.cpp
index ef7b6568107..0e7e5d22947 100644
--- a/src/txgraph.cpp
+++ b/src/txgraph.cpp
@@ -2421,6 +2421,8 @@ void TxGraphImpl::SanityCheck() const
if (cluster.GetTxCount() != 0) {
actual_clusters.insert(&cluster);
}
+
+ asse
...

💬 ismaelsadeeq commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2352500751)
In "txgraph: keep track of Cluster memory usage (preparation)" 7974ed311c1843d548d98e9eae8fed71a42a1026

Should also count the sequence number of the cluster?
```suggestion
// Memory usage of the cluster m_sequence.
+ sizeof(uint64_t);
```

💬 ismaelsadeeq commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2352615744)
In "txgraph: Make level of Cluster implicit (optimization)" https://github.com/bitcoin/bitcoin/commit/22f5fe833fcf3da6a4180d181ff68d6da34f40db

Instead of using an int for level here, should we relax the `Level` `enum` from a scoped `enum` to an unscoped one to allow implicit conversion and unified input? This would also apply in cases where we want to pass a specific `enum` value, e.g. in e.g `CopyToStaging`

💬 fjahr commented on pull request "rpc: add scan_utxoset option to getbalance(s) to verify wallet balance accuracy":
(https://github.com/bitcoin/bitcoin/pull/33392#issuecomment-3298984427)
It seems kind of weird to me to add this as an option to `getbalance. The problem is in the `importdescriptors` call, which is using the wrong birthdate. Have you looked into making this an option to `importdescriptors`? If the performance isn't too bad, this could even be on by default. If that is possible that would seem preferred, since you also write "However, users may not realize that their wallet balance is incomplete.", if they don't know they likely also won't call the `getbalance` with
...

💬 glozow commented on pull request "rpc: followups for min fee changes":
(https://github.com/bitcoin/bitcoin/pull/33189#discussion_r2352734299)
Btw, I added this to the release notes draft: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/v30.0-Release-Notes-Draft#updated-rpcs

💬 ryanofsky commented on issue "RFC: Bitcoin Core Node `BlockTemplateManager`":
(https://github.com/bitcoin/bitcoin/issues/33389#issuecomment-3299100671)
Interesting discussion. Especially interesting to know about possibility of leaking transaction information if creation times aren't tracked and used correctly. And to know how this could take more advantage of cluster mempool. I also like idea of calling this BlockTemplateCache instead of BlockTemplateManager to give it more focus.

Overall, notion of a shared cache seems useful. In terms of the implementation approach, I think it'd be nice if new code were integrated with existing code sooner
...

💬 Sjors commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3299129244)
On second thought I don't think mutating the block is a problem.

> I think this assumes that the provided coinbase tx includes the coinbase witness, which is different to how `submitblock` behaves. (In particular `submitblock` includes a call to `chainman.UpdateUncommittedBlockStructures`, whereas `AddMerkleRootAndCoinbase` doesn't) That seems like it might be a source of bugs -- might at least be worth adding a check and having an explicit log message if the coinbase requires a witness but d
...

💬 Raimo33 commented on pull request "key: use static context for libsecp256k1 calls where applicable":
(https://github.com/bitcoin/bitcoin/pull/33399#issuecomment-3299155068)
Concept ACK

🤔 marcofleon reviewed a pull request: "p2p: Correct unrealistic headerssync unit test behavior"
(https://github.com/bitcoin/bitcoin/pull/32579#pullrequestreview-3230419017)
code review ACK cc5dda1de333cf7aa10e2237ee2c9221f705dbd9

I don't have a strong opinion on the refactors or commit order, although I can see how this PR might not be the most straightforward to review. Overall, the core changes in 7b00643ef5f932116ee303af9984312b27c040f1 and cc5dda1de333cf7aa10e2237ee2c9221f705dbd9 lgtm. Making the headers sync params configurable ensures that the new test can cover the redownload buffer boundary logic.

💬 TheCharlatan commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2352862088)
I made a small fuzz test now. If you think it is useful, and want to add it:

```c++
// Copyright (c) 2025-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <util/threadpool.h>

#include <test/fuzz/FuzzedDataProvider.h>
#include <test/fuzz/fuzz.h>

namespace {

struct ExpectedException : std::runtime_error {
using std::runtime_error::runtime_er
...