💬 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

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2480474170)
What your patch demonstrates is that without the fix in this PR (clearing the cache), the invalid block is accepted by the node.

If you then also drop the `assert_equal ... current_block_height)` light, you'll see that the fails at "Block should propagate".

The "Submit again, with the witness" step doesn't replace the invalid block, and `ProcessNewBlock` returns `true` for duplicate blocks and `submitSolution` passes it on.

I think is good, because in Stratum v2 (depending on configurat
...

👍 TheCharlatan approved a pull request: "mining: check witness commitment in submitBlock"
(https://github.com/bitcoin/bitcoin/pull/33745#pullrequestreview-3403009700)
ACK 7d57c4e44451ce1167ba76a73c9cc6f6fd6cfca4

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2480523381)
I have already done that, which parts do you think I'm missing?

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2480567300)
Taken

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2480568701)
`remote_block_before` is set before the two `submitBlock` calls above. I improved the wording a bit and I moved the `getchaintips()[0]["height"]` down to the next test, because that was probably confusing.

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3471963619)
Pushed [7d57c4e](https://github.com/bitcoin/bitcoin/commit/7d57c4e44451ce1167ba76a73c9cc6f6fd6cfca4) to [2fc95ca](https://github.com/bitcoin/bitcoin/commit/2fc95ca731b52ca419215109c568e89bf683a94b): [compare](https://github.com/bitcoin/bitcoin/compare/7d57c4e44451ce1167ba76a73c9cc6f6fd6cfca4..2fc95ca731b52ca419215109c568e89bf683a94b). No behavior change.

I added a patch the to PR description that reproduces the original issue.

Expanded the `submitSolution` documentation to indicate that a
...

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2480606043)
Added a patch to the PR description which reproduces the issue.

📝 A-Manning opened a pull request: "rest: Query predecessor headers using negative count param"
(https://github.com/bitcoin/bitcoin/pull/33752)
There is currently no mechanism by which to batch-query headers from tip, backwards towards genesis.
This PR extends the REST Blockheaders API to support batch-querying predecessor headers, by providing a negative count parameter.

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3472031005)
@ryanofsky wrote:

> I do think more improvements could be made here:

These could be followups, so will comment here:

> - documentation could be updated to say what requirements on submitSolution inputs are

Not sure what to write there, basically whatever consensus requires. Previously mining software would the whole coinbase from scratch and we never really documented that.

The Stratum v2 spec does specify what their `SubmitSolution` should look like: https://stratumprotocol.org/
...

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2480649764)
I kept all three, but as I explained in https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3471963619 two of them are already `false`.