💬 Sjors commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2922826515)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1

Just a rebase and minor test tweak since my previous review.

💬 polespinasa commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2922833884)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1

Just a rebase due to silent merge conflict since my prev review

📝 hebasto opened a pull request: "cmake, qt: Process `*.qrc` files manually"
(https://github.com/bitcoin/bitcoin/pull/32648)
The first commit enables modification of the properties of the source files produced by Qt Resource Compiler, which helps, for example, to silence compiler warnings.

The second commit makes use of this new capability to silence `-Wtrailing-whitespace` warnings and resolves https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-2876950405.

👍 theStack approved a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2881862929)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1

(as per `$ git range-diff 35bcd8ee...a189d636`)

💬 hebasto commented on pull request "build: add `-W[leading|trailing]-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-2922854588)
> > GCC 15 now has options to turn these into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable these options.
>
> Concept ACK on that.
>
> > Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
> > ```shell
> > [ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
> > /root/bitcoin/build/src/qt/bitcoinqt_autoge
...

💬 TheBlueMatt commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2922883838)
> I'm not super familiar with FIBRE -- does it have a chance of being used again? Would dropping unsolicited compact blocks prevent FIBRE from working again?

I believe as of a week of two ago there's work to revive it!

> This is a good point, you would need to be the fastest peer to exploit it in the non-blocksonly case.

This was only half my point - my larger point was that we can't really fix this issue because if you're the fastest peer once you're gonna be the fastest peer again. I'm not
...

⚠️ Jezmaul opened an issue: "Wallet"
(https://github.com/bitcoin/bitcoin/issues/32649)
https://github.com/bitcoin/bitcoin/blob/3E6RsQwEWiCLvZ3Lq33X1dR43VLTzYK9gg/src/wallet/sqlite.cpp#L719

✅ fanquake closed an issue: "Wallet"
(https://github.com/bitcoin/bitcoin/issues/32649)

💬 Crypt-iQ commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2922992916)
> This was only half my point - my larger point was that we can't really fix this issue because if you're the fastest peer once you're gonna be the fastest peer again. I'm not sure it's worth trying to fix it broadly given the impact it might have (even if small) and the fact that it isn't really a great fix.

Hmm, I agree with this. I guess the solution is a better protocol to avoid fingerprinting? Adding more complexity here sounds a bit scary.

I looked at the WIP fibre patchset here: https:/
...

💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-2923023983)
> I.e. be the first to supply the latest valid block at the tip and you'll be selected as high bandwidth (we process unsolicited block messages, so this should be trivial).

Yup I think this means the defense is ineffective.

> Even if you're low-bandwidth can't you just send a headers announcement immediately followed by the compact block? As long as your headers message is the first announcement of the block, the compact block will be requested and since these messages are processed sequen
...

💬 Crypt-iQ commented on pull request "p2p: Add mutation check inside compact block's FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#issuecomment-2923073843)
Concept ACK

🤔 1440000bytes reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2882335536)
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1

<details>
<summary>Signature</summary>

<pre>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
-----BEGIN PGP SIGNATURE-----

iHUEARYKAB0WIQSSUwYqT5LWNFkIXKYtIwUgISpZAQUCaDoHIwAKCRAtIwUgISpZ
AXfEAQCLt/Tzit+ZxCmsP0BkCi4zG+4OyFHcvzytQ6SJFQuUjwD/TsjUydeA06Ft
RcJvG/+brpVM8NV3Ga/trp74dg67NQE=
=xtxJ
-----END PGP SIGNATURE-----
</pre>


</details>

Previous review: https://github.com/b
...

💬 mzumsande commented on pull request "p2p: Add mutation check inside compact block's FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2116539524)
Could this be an opportunity to resolve the TODO comment above after `check_block` by not calling this function anymore? If I understand the comment correctly it only calls `CheckBlock()` to allow the caching of the check result and avoid repeated Merkle checks, which `IsBlockMutated()` also would do today?

💬 theuni commented on pull request "thread-safety: fix annotations with REVERSE_LOCK":
(https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2923398293)
> I think those arguments are pretty compelling, and think maybe the ideal thing would be to close this PR (to avoid confusing different approaches) and open a new PR dropping `REVERSE_LOCK` entirely and replacing with it lock/unlock calls, or perhaps with a `WITH_UNLOCK()` macro that runs a fragment of code with `unlock()` before and `lock()` after, and does not relock when an exception is thrown, and does not make an unsafe `lock()` call inside a destructor.

Hmm.

For context, my primary
...

💬 fjahr commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-2923413036)
> I'm interested in seeing benchmarks of this on various systems

<details>
<summary>Macbook Pro M4</summary>

$ build/bin/bench_bitcoin --filter="Linearize.*Example.*" -min-time=10000

| ns/cost | cost/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1.12 | 893,382,447.85 | 0.5% | 11.06 | `LinearizeBoundedExample0`
| 1.34 | 743,601,759.92 | 0.7
...

🤔 hodlinator reviewed a pull request: "headerssync: Keep tests ahead of increasing params"
(https://github.com/bitcoin/bitcoin/pull/32579#pullrequestreview-2881152192)
Thanks for your feedback so far @l0rinc! Took most of your suggestions (https://github.com/bitcoin/bitcoin/pull/32579#pullrequestreview-2857912770).

💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115848183)
I like the idea of including the generation date, and was including changes to the Python generator file in an unpublished version of this PR, but decided to keep that separate. Added to that separate WIP branch.

The magic numbers are not updated manually, they are included in the calculated output:
https://github.com/bitcoin/bitcoin/blob/11a2d3a63e90cdc1920ede3c67d52a9c72860e6b/contrib/devtools/headerssync-params.py#L347-L348

💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115856000)
If we didn't succeed, then `request_more` shouldn't matter, but agree it's best to have it be false (just like `pow_validated_headers` should be empty). It would be better if `ProcessNextHeaders` returned something like `std::optional<ProcessingResult>` instead of having a `bool` field to indicate success, but this PR already feels large enough.

"foiled" means the attack attempt was detected and therefore failed. Elaborated the comment a bit in next push.

💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115831187)
One could argue at least the `uint64_t`-taking ctor here and in `arith_uint256` should be explicit, but I'd rather not have to change src/pow.cpp in this PR.

💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2116586810)
Took the scoping idea, still not sure about the structured bindings.