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

💬 Sjors commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3299548132)
@plebhash indeed this PR just introduces the extra method and clarifies in the documentation that `submitSolution` and `applySolution` are slightly different from the `submitsolution` RPC.

👋 willcl-ark's pull request is ready for review: "WIP: Backport Cirrus runners to 29.x"
(https://github.com/bitcoin/bitcoin/pull/33403)

🚀 glozow merged a pull request: "test: Add submitblock test in interface_ipc"
(https://github.com/bitcoin/bitcoin/pull/33380)

💬 ryanofsky commented on issue "Default ASmap file path is not used unless -asmap is set":
(https://github.com/bitcoin/bitcoin/issues/33386#issuecomment-3299667325)
re: fjahr https://github.com/bitcoin/bitcoin/issues/33386#issuecomment-3298756998

> Can you pinpoint which part of the docs gave you this impression? Was it the RPC help or maybe files.md (https://github.com/bitcoin/bitcoin/blob/master/doc/files.md)? Is there a different place where you think this behavior could be better documented?

Since I noted the same issue in https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2516002191, the part of the doc which gave me that impression was
...