💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3232177376)
Yep. But been on holiday this week and last.

🤔 BrandonOdiwuor reviewed a pull request: "Update `minisketch` subtree and switch to its build script"
(https://github.com/bitcoin/bitcoin/pull/32856#pullrequestreview-3163505949)
ACK a6b48f009f7bf88bc1f15ed9490276baa898884e

Verified migration to `minisketch` upstream CMake build script. Successfully built `minisketch` and `Bitcoin Core` on macOS 15.6 (Clang 17, CMake 4.0.2) and Ubuntu 24.04 (GCC 13, CMake 3.28.3)

**macOS**
```
$ cmake --build build -j9
```
<img width="1488" height="224" alt="Screenshot 2025-08-28 at 09 58 53" src="https://github.com/user-attachments/assets/538d6bc0-d55c-444d-9a81-4194678379dc" />

**Ubuntu**
```
$ cmake --build build -j9
`
...

💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2306467573)
[17:54:00.577] = help: Remove unused import: `decimal.Decimal`

💬 maflcko commented on pull request "test: Use extra_port() helper in feature_bind_extra.py":
(https://github.com/bitcoin/bitcoin/pull/33260#discussion_r2306531752)
> iterator

That doesn't work, because it is just what the current code does: Take a port and `+=1` it. The goal of this pull request is to properly derive (and check) each port by its own.

💬 maflcko commented on pull request "test: Use extra_port() helper in feature_bind_extra.py":
(https://github.com/bitcoin/bitcoin/pull/33260#discussion_r2306533787)
thx, but I want to keep the changes here minimal, not rewrite the whole test

💬 purpleKarrot commented on pull request "Update `minisketch` subtree and switch to its build script":
(https://github.com/bitcoin/bitcoin/pull/32856#discussion_r2306512554)
This option is useless and can be removed from minisketch because passing [`EXCLUDE_FROM_ALL`](https://cmake.org/cmake/help/latest/prop_dir/EXCLUDE_FROM_ALL.html) to `add_subdirectory` has the effect that

> Any install rules defined in the subdirectory or below will be ignored when installing the parent directory.

💬 purpleKarrot commented on pull request "Update `minisketch` subtree and switch to its build script":
(https://github.com/bitcoin/bitcoin/pull/32856#discussion_r2306490269)
Why?

We should not interfere with `CMAKE_EXPORT_COMPILE_COMMANDS`. If developers set this option globally in their dotfiles because it will give them code completion in their IDE, setting it to `OFF` here will break their setup.

💬 purpleKarrot commented on pull request "Update `minisketch` subtree and switch to its build script":
(https://github.com/bitcoin/bitcoin/pull/32856#discussion_r2306553108)
I find it a bit awkward to define this as a function in a module, when it is not really providing reusable functionality. The function may only called in a single place and there is only one valid argument.

A better approach would be to get rid of the `../cmake/minisketch.cmake` module and define the function inline without any arguments. Then, add a comment that makes clear that this is a workaround and also notes when it will be replaced:

```cmake
// TODO: use `block()` once the minimum cmak
...

💬 maflcko commented on pull request "rpc: require integer verbosity; remove boolean 'verbose'":
(https://github.com/bitcoin/bitcoin/pull/33214#issuecomment-3232540549)
I expect those to be widely used (not only in the internal tests, as you noticed in the repeated force pushes), so this may or may not be a widely breaking change (depending on how much they are used in active projects).

There is no auto-generated schema or RPC interface, so those projects would have to fix them manually.

No strong opinion, but maybe this can be closed for now, and revisited when there is a schema generator?

💬 maflcko commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2306701610)

“for a wallet transactions” → “for wallet transactions” [“a” makes the noun plural ‘transactions’ ungrammatical]

💬 maflcko commented on pull request "wallet, rpc: remove settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/32138#issuecomment-3232559201)
What is the point of keeping `m_pay_tx_fee`. It is just dead code now, no?

💬 maflcko commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2306835060)
this is the wrong commit. You'll have to apply the patch to the correct commit.

🤔 janb84 reviewed a pull request: "test: Use extra_port() helper in feature_bind_extra.py"
(https://github.com/bitcoin/bitcoin/pull/33260#pullrequestreview-3164184069)
crACK fabc2615af26c61a503f23ae4fd0353f90602bbe

PR adds extra function that returns a port. In this new function the utility function p2p_port(n) is called, which does a basic check and returns a (pseudo) random port number. This to reduce errors of using port numbers that are unavailable.

This looks like a simple solution without to much churn to solve #33250

💬 janb84 commented on pull request "test: Use extra_port() helper in feature_bind_extra.py":
(https://github.com/bitcoin/bitcoin/pull/33260#discussion_r2306925720)
```suggestion
def new_port():
```
NIT; wouldn't `new_port()` be more suitable ? line 50 is just the use of a new port not an extra port. "extra to what?"

💬 maflcko commented on pull request "test: Use extra_port() helper in feature_bind_extra.py":
(https://github.com/bitcoin/bitcoin/pull/33260#discussion_r2306984636)
It is "extra" on top of the normal node p2p ports, so I'll leave this as-is for now.

⚠️ jsarenik opened an issue: "Zero output not cleared"
(https://github.com/bitcoin/bitcoin/issues/33265)
I think this relates to https://github.com/bitcoin/bitcoin/issues/27463, fyi @instagibbs

Working on a clear reproduction script for regtest, but here are some preliminary notes and signet examples:

The script (network-less) is `51024e73`. It `decode`s to different addresses for mainnet, testnets (including signets) and regtest:

* `bc1pfeessrawgf` - mainnet
* `tb1pfees9rn5nz` - testnets (incl. signets)
* `bcrt1pfeesnyr2tx` - regtest

This LN Anchor address is important because such transac
...

💬 frankomosh commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2307004087)
okey. Will sort it out

💬 maflcko commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#issuecomment-3232995418)
This picks up https://github.com/bitcoin/bitcoin/pull/13990? If yes, it may be good to explain the differences.

💬 maflcko commented on pull request "wallet: Remove isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2307083091)
Ref also https://github.com/bitcoin/bitcoin/issues/22518

👍 stickies-v approved a pull request: "threading: reduce the scope of lock in getblocktemplate"
(https://github.com/bitcoin/bitcoin/pull/33264#pullrequestreview-3164397816)
ACK 06ec4809045f5cbe51d54e8eda08db03ca4d5ca1