👋 optout21's pull request is ready for review: "test: Add test on skip heights in CBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/33661)

👍 ryanofsky approved a pull request: "mining: check witness commitment in submitBlock"
(https://github.com/bitcoin/bitcoin/pull/33745#pullrequestreview-3401822551)
Code review ACK 7d57c4e44451ce1167ba76a73c9cc6f6fd6cfca4. Code, test, documentation updates all seem like improvements and it is good to make submitSolution provide better feedback. I do think more improvements could be made here: documentation could be updated to say what requirements on submitSolution inputs are, and I think BlockTemplate accessors could be added so miners do not need to make hardcoded assumptions about the coinbase transaction? (https://github.com/bitcoin/bitcoin/pull/33745#d
...

💬 ryanofsky commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479687669)
re: https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479638680

> I previously hinted at resetting this flag too, but does that really make sense, if we've just re-calculated the merkle root?

To me, it would seem safest to go other direction and reset all the cached flags including `fChecked`. Or if any flags are intentionally not reset it would be nice to a comment making it clear that it was intentional and should be safe.

In the case of CheckMerkleRoot, it is not just check
...

💬 ryanofsky commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479641553)
In commit "mining: ensure witness commitment check in submitBlock" (504acfcd5d1ed4d77f7fd4f5dc57e66d6718d3d4)

Is there a submitSolution call missing here? I don't understand what this is testing

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479711613)
Done in c00612c15592faa094d60c7bba8eb9966f5910d6

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479713548)
Agreed -- I was thinking I'd tackle this in #33591, if that seems reasonable?

📝 john-moffett opened a pull request: "qt: Remove HD seed reference from blank wallet tooltip"
(https://github.com/bitcoin-core/gui/pull/908)
Blank descriptor wallets currently do not have HD seeds and none can be added (or 'set') by the user, so remove the reference in the tooltip.

As I understand it, descriptor wallets don't have a global HD seed and don't store the HD seeds for keys they generate. Currently, no new HD seeds can be added by the user (even for old wallets since `sethdseed` was removed), though it may be possible in the future, eg - https://github.com/bitcoin/bitcoin/pull/33043

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479721696)
I think I managed to squash this into 9d0c8d4d5ec2a50ba4756b9882d8801f24542a2e; hopefully the changes all look sane...

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479724535)
Gone now in 110e62ce76f029e32eeed44238db8d3c09ddb2f7.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479726030)
Done in a6649dffe92319a17790bf2add5206a84726f34d.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479727735)
This was squashed.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479729062)
Done in b0c8081258fc37816e4675e17fa9ea8d308921f3

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479731017)
Done in 0097138011b6e502bfbae528304a6a7e6b6462ad.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479732658)
Fixed in f784ae88327c61b08684e1e073b010a244e2b52e.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479735309)
There was an off-by-one here (feerate diagrams start with a [0, 0]), but otherwise I incorporated this into ed23b4537ae8a8a814738909fe9a84c548f2855d.

💬 davidgumberg commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479771522)
(Aside from the last line checking that the second node's chaintip has advanced), I don't think this is testing anything changed by this PR, this test succeeds on master, as I understand it, this is checking that after the `submitSolution` calls above, that `m_block_template->block` has been modified, but I'm not sure why that would be an interesting test.

💬 davidgumberg commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479793393)
I want to suggest adding a check here:

```suggestion
assert_equal(res.result, True)
# self.nodes[1]'s chaintip did not advance.
assert_equal(self.nodes[1].getchaintips()[0]["height"], current_block_height)
```

But I'm a bit confused, because testing this without the changes to `src/node/miner.cpp`, self.nodes[1]'s chaintip advances after submitSolution is called with the witnessless coinbase, and this check I'm suggesting fails.

To be specific, I'm using t
...

💬 davidgumberg commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2479859800)
> wonder if there are other places that could benefit from CTransaction caching. Two full hashes are cached, so caching the size should be cheap in both time and space?

I don't think we should be caching the sizes, since there aren't really any performance critical users of `GetTotalSize()`, it's invoked by these cmpctblock logs, many RPC's when they do pretty-printing of transactions, and the qt interface's transaction view, but there are critical paths where `CTransaction()`'s are constru
...

💬 davidgumberg commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#issuecomment-3470816225)
crACK https://github.com/bitcoin/bitcoin/pull/33738/commits/17856343f80fcfe307121679621ade6d1e0ec32b

👍 polespinasa approved a pull request: "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer"
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-3402148347)
code review ACK 32bfd6136134e7194ea0b56ec08498f66829fc40

I think it's worth resolving the odd case for `-blocksonly` in this pull request. But I would be happy too with it being done in a follow-up PR.