💬 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"}
```

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2515703456)
in ca18549ee72: Maybe creeped in during a rebase? This string is already on another line.

💬 ajtowns commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3518709899)
> Why doesn't `tx.get_vsize()` return the correct value in the code above. Did I make a stupid mistake somewhere?

The vsize that `check_mempool_result()` (aka `testmempoolaccept`) returns is adjusted for the sigop count (which is treated as 20 due to `fAccurate=false` via `GetTransactionSigOpCost()`/`GetLegacySigOpCount()`), while `get_vsize` is not. See also #32800.

💬 TheCharlatan commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2515827417)
This just checks pointer equality. Do we really need a separate function for that? Maybe we can just handle this on the C++ wrapper layer?

💬 tobtoht commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3518761170)
Guix Build:

```
x86_64
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
9e44c7635597430ee32fb26dff8fcd3261fe7
...

💬 ajtowns commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3518785282)
utACK 40dcbf580d8eb31a067b62bf9676099919b9841e

🤔 alvroble reviewed a pull request: "test: add case where `TOTAL_TRIES` is exceeded yet solution remains"
(https://github.com/bitcoin/bitcoin/pull/33701#pullrequestreview-3450241034)
re-ACK

🤔 ajtowns reviewed a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3446637980)
Have run this live for a little while now without encountering problems which is a good sign. Some comments on the interface follow.