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

💬 polespinasa commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2479853555)
What if in the case of being in `-blocksonly` mode and receive a `CMCTBLOCK` message we instantly respond with a `GETBLOCKTXN` asking for all transactions (skipping all `InitData` logic)?

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index dad2c4ce6d..d46c9b003b 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -4413,6 +4413,15 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
}

if (!requeste
...

💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2479877695)
It doesn't really ever make sense to process a `CMPCTBLOCK` message as blocks-only, the peer should just send the block, if we want to do anything with the message as a blocks-only, we should just process the header of the `CMPCTBLOCK`:

```
fRevertToHeaderProcessing = true;
```

This will call `ProcessHeadersMessage()` and that can request the block.

💬 polespinasa commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2480072019)
> If an inbound peer (which doesn't get disconnected for sending `BLOCK_CACHED_INVALID` headers) knows of (or spends the PoW to construct) an invalid block with a valid header, they can resend the same header and spam the log.

Just curiosity, why we don't disconnect it?

👍 polespinasa approved a pull request: "validation: Improve warnings in case of chain corruption"
(https://github.com/bitcoin/bitcoin/pull/33553#pullrequestreview-3402460705)
code reviewed and tested ACK 6d2c8ea9dbd77c71051935b5ab59224487509559

This PR introduces an informative warning about an invalid block header, which may fix issues like #26391 by giving more information to the user.

Tried to force the warning (f8955eaee19d592cc20c497d145f5eae25277014) following https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-1291765534 and it behaves as expected.

Non-blocking nit: I agree with @stickies-v that the block hash may be useful in the message.