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

💬 151henry151 commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3471351143)
Guix build for 1e6aa74b4b628e8acafe6a3d2aaf2a6aa0d9d290:

4f2f3d9c61fddfb4a9858375665dd72e3e4aa6a465e39f579bfea46793eec2d0 dist-archive/bitcoin-1e6aa74b4b62.tar.gz
7852ac3c67360a28522051129758ca4a9303641e3511f6714d0f738cb9a816b8 x86_64-linux-gnu/bitcoin-1e6aa74b4b62-x86_64-linux-gnu-debug.tar.gz
f1b1a102d01f486afd17a8c98437183f8a50bd72ea40800ad0ad4d5a4b9f0d47 x86_64-linux-gnu/bitcoin-1e6aa74b4b62-x86_64-linux-gnu.tar.gz

💬 maflcko commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2480316536)
> I don't think we should be caching the sizes,

Good point. I haven't looked at the other places where it is used. I guess one could consider renaming `GetTotalSize` to `ComputeTotalSize`, to hopefully avoid similar issues of this kind in the future?

💬 maflcko commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2480411370)
> What would be the benefits of using `self.MaybeArg<bool>`? I'm not really familiar with it tbh.

The function is explained in the docstring above the function. It will read the default value by itself, so it doesn't need to be hardcoded several times.



> I think with the rebase it's easier to change defaults without touching code logic.

I don't think so. I can still see ` FeeEstimateMode feerate_units = (!request.params[2].isNull() && request.params[2].get_bool())
...

💬 maflcko commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2480415279)
> You are right, although I think it would not be a problem because we don't print that many decimals.

They are printed. You can try it locally by printing the constructed univalue or by checking it in a test.



> Anyway, I think the approach in [afeeac1](https://github.com/bitcoin/bitcoin/commit/afeeac1596da47b0e431d468a665d344b39e1a87) is cleaner and follows what we already do for btc/kvB. What do you think?

I don't think it handles negative feerates correctly. Generally, it is best
...

👍 TheCharlatan approved a pull request: "test: ipc: resolve symlinks in `which capnp`"
(https://github.com/bitcoin/bitcoin/pull/33749#pullrequestreview-3402951597)
utACK 51093d6ae1d415b8dfa47f11cb634a87bd97e8a9

👍 TheCharlatan approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3402991102)
ACK c30647c4d34c2941696729704854467b30657c43