📝 hebasto opened a pull request: "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it"
(https://github.com/bitcoin/bitcoin/pull/33857)
This PR is part of the ongoing effort to migrate to the modern UCRT runtime for cross-compiled Windows binaries, including release builds.

For more details about this migration, see:
- https://github.com/bitcoin/bitcoin/issues/30210
- https://github.com/bitcoin/bitcoin/pull/33593
- https://github.com/bitcoin/bitcoin/pull/33764

The changes in `depends/hosts/mingw32.mk` enable automatic detection of cross-compilers for Windows + UCRT, removing the need to specify them explicitly, as shown
...

🤔 mzumsande reviewed a pull request: "validation: fix assumevalid is ignored during reindex"
(https://github.com/bitcoin/bitcoin/pull/33854#pullrequestreview-3449234404)
Have you tried this out on signet or mainnet?
I haven't yet, but just from looking at the code I would suspect that we now

1. do the -reindex part without connecting any blocks (except genesis [here](https://github.com/bitcoin/bitcoin/blob/138726a6f8101e0fe7e9ae701ef17b37fcbdee73/src/validation.cpp#L5161))
2. omplete headers-sync with a peer until minchainwork
3. Download the first block we don't have on disk from a peer and only then call `ActivateBestChain()`
4. try to connect that bl
...

💬 hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3518140890)
My Guix build:
```
aarch64
451630ecff800ab320a9d5ad8062758df9d700310690110e764c916ae4c8121e guix-build-527acc5ee497/output/dist-archive/bitcoin-527acc5ee497.tar.gz
bf19a3b8e9e9cf609102c38cd6c00dca4d2645f66ae3af591f29fdeceef7b6cf guix-build-527acc5ee497/output/x86_64-w64-mingw32/SHA256SUMS.part
b69dc956f6fd6bb9b4e20e5f4d29eb4ef74e450d250346bf0450cc051b5cfa95 guix-build-527acc5ee497/output/x86_64-w64-mingw32/bitcoin-527acc5ee497-win64-codesigning.tar.gz
9e44c7635597430ee32fb26dff8fcd3261f
...

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3518142243)
It turns out that using the `getCoinbase()` result directly in the Python test led to memory management related errors. Parts of the coinbase were dropped by the time of the second `checkBlock()`.

Rather than debug (our use of) PyCapnp, I just introduced a `@dataclass` for `CoinbaseTemplateData`, so we fully own it.

💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2514720362)
missed that, yeah, you are right.

💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515093447)
I think this is better to do it in the diff below for two reasons

1. In [Internal interface guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines)
It's stated that

> Interface method definitions should wrap existing functionality instead of implementing new functionality. Any substantial new node or wallet functionality should be implemented in [src/node/](https://github.com/bitcoin/bitcoin/blob/master/src/node) or [src/wallet/](ht
...

💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515020247)
In 91a44988a14037e9c923a789f1b7a616ff902ee1 Stop enforcing descendant size/count limits

After this commit, the `limitdescendantcount` set to 64 in `feature_rbf.py` test is no longer necessary. The `limitancestorcount` set to 64 is also not necessary since we stopped enforcing that in some previous commit.

This can be removed in the followup along with other stale once in feature_dbcrash.py and `wallet_basic.py`.

💬 hebasto commented on pull request "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it":
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3518166527)
> Can be tested on the following systems:
>
> * Fedora 42 or 43 (requires the `ucrt64-gcc-c++` package).

For example: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/19274168370/job/55109785216.

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3518176660)
> > should the depends build instructions be updated to mention this?
>
> I'd prefer updating the docs once `depends/hosts/mingw32.mk` is adjusted so that compilers don't need to be specified explicitly.

Done in https://github.com/bitcoin/bitcoin/pull/33857.

💬 hebasto commented on issue "build: use UCRT runtime for Windows (release) binaries":
(https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-3518182486)
> - [ ] Any other documentation / Windows release build configuration updates.

See: https://github.com/bitcoin/bitcoin/pull/33857.

👍 theStack approved a pull request: "test: assumeutxo: add missing tests in wallet_assumeutxo.py"
(https://github.com/bitcoin/bitcoin/pull/30455#pullrequestreview-3449412250)
ACK 55c6a69f777ac08a14e61b98efea00e5c8f98a5f

With two non-blocking suggestions (or three, including https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-3486623443).

💬 theStack commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2515282136)
follow-up idea: could deduplicate with the `error_message` within `test_backup_during_background_sync_pruned_node`, so it doesn't have to be adapted twice if it ever changes (maybe that was already intended, since you already moved the message into a variable?)

💬 theStack commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2515271970)
nit, for smaller future diff if another node is added
```suggestion
["-fastprune", "-prune=1"],
```

💬 polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3518313943)
@glozow thanks for your comments :)

> Can you explain why it's not good practice?

We open the door to other software/users to make easy mistakes by forcing them to do a unit conversion that Core shouldn't have done in the first place. I understand that this has been like this since ¿always?, but giving users the option to have sat/vB (or even more units thanks to FeeFrac) can be useful and reduce the burden on other software.

> It seems extremely dangerous to ever change the default...
...

💬 plebhash commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3518350045)
> @plebhash can you try to see if it's easy for you to support both `getCoinbase()` and `getCoinbaseTx()` in [stratum-mining/sv2-apps#59](https://github.com/stratum-mining/sv2-apps/pull/59), preferring the first and falling back to the latter if it doesn't exist?

@Sjors we're not using `getCoinbaseTx` on that code

we do `getBlock` and that provides everything we need, including relevant coinbase info

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515354081)
It's not clear to me what the plan is, but if the approach isn't switched, the following patch compiles, looks more sensible, and seems to pass all tests:

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index ee46f99add8..37015ef69e6 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1613,11 +1613,11 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptSubPackage(const std::vector<CTr
AssertLockHeld(m_pool.cs);
auto result = [&]() EXCLUSIVE_LOCKS_REQUIRED
...

💬 glozow commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3518399410)
> forcing them to do a unit conversion that Core shouldn't have done in the first place.

I still don't see the argument for why a unit per kvB is so problematic. BTC and satoshis are easily convertible.

I do think it's worthwhile to unify the format for inputting and and outputting feerates. And I agree it should be configurable, given there are multiple preferred formats.

My concern is the idea that the default format might be changed in the future, causing somebody's reading of a feer
...

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515659830)
I think @glozow's approach is definitely the right direction to go, but let's defer making that change to a future PR so that it can get more focused review, since it's not trivial to reason about how all this currently works. I'm taking @sipa's suggestion for now and will re-push.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515672555)
I took this change in #33591.

💬 ismaelsadeeq commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3518690143)
I really appreciate the feedback and reviews so far from @ryanofsky, @Sjors, and @brunoerg.
Given that I’ve opened #33758 with the overall plan and we’ve had some recent discussions https://github.com/bitcoin/bitcoin/issues/33756#issuecomment-3502079264, I’ll work on fixing the C. I issue here hence marking as draft.

I will also work on the e2e implementation separately.