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

📝 ismaelsadeeq converted_to_draft a pull request: "node: add `BlockTemplateCache`"
(https://github.com/bitcoin/bitcoin/pull/33421)
This PR implements the first step of #33758.

This PR Implement a cache layer. All newly created block templates should be stored along with their respective configuration options. Client requests for block templates will specify the maximum age of the template in seconds; that is, how fresh they want the template to be:

If a template with matching options exists in the cache and the interval has not elapsed, a cached template is returned.
If no template exists or the interval has elapsed,
...

🤔 glozow reviewed a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3449740145)
Small batch of RPC comments

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515530268)
in ca18549ee72: Perhaps set the expectation that there could be interface changes.
```suggestion
{"hidden", &getmempoolfeeratediagram},
```

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515774000)
in ca18549ee7288f16f7c93ef9f9cac932d29a0eba

(Happy to make this a followup, because there are other RPCs that need this change too)

```suggestion
RPCResult{RPCResult::Type::NUM, "vsize", "Sigops-adjusted virtual transaction size: the maximum of BIP 141 virtual size and the transaction's sigops multiplied by -bytespersigop."
```

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515779737)
nit, also happy to put in followup

```suggestion
{RPCResult::Type::NUM, "vsize", "cumulative sigops-adjusted vsize"},
{RPCResult::Type::NUM, "fee", "cumulative modified fee"}
```