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

💬 zaidmstrr commented on pull request "test: Add submitblock test in interface_ipc":
(https://github.com/bitcoin/bitcoin/pull/33380#issuecomment-3299291247)
Tested ACK [0a26731](https://github.com/bitcoin/bitcoin/pull/33380/commits/0a26731c4cc182e887ce959cdd301227cdc752d7)

🤔 zaidmstrr reviewed a pull request: "test: Add submitblock test in interface_ipc"
(https://github.com/bitcoin/bitcoin/pull/33380#pullrequestreview-3230558328)
Tested ACK [0a26731](https://github.com/bitcoin/bitcoin/pull/33380/commits/0a26731c4cc182e887ce959cdd301227cdc752d7)

💬 plebhash commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3299309843)
> The current ambition is to have the Template Provider communicate this value (eventually, no rush).

this would require a change to the Sv2 spec so that `NewTemplate` message carries a new field called `coinbase_witness `.

but unfortunately, changing Sv2 spec is usually received with resistance from the current players.

if/when a softfork happens and we start committing stuff to the coinbases' `witness reserved value`, that could be stronger motivation.

but until that day comes, th
...

👍 darosior approved a pull request: "net: do not apply whitelist permissions to onion inbounds"
(https://github.com/bitcoin/bitcoin/pull/33395#pullrequestreview-3230622858)
utACK e46a7a547371317a4d116b9b1a314917508ea480

Happy to re-ACK if you take Vasil's suggestion. I don't think there is any functional difference, but the separation of concerns between `-whitelist` and `-whitebind` permissions is slightly neater.

💬 hebasto commented on issue "build: `test_bitcoin` link failure with `-flto=thin` & address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/33147#issuecomment-3299457712)
Using a [supported linker](https://clang.llvm.org/docs/ThinLTO.html#linkers), such as `lld` on Ubuntu, helps.

💬 musaHaruna 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-3299464744)
> Have you looked into making this an option of importdescriptors?
I have not looked into making it an option for `importdecriptors`, but I will look into it and check the performance, and get back to you on that.

Thanks for the suggestion. I really appreciate it.