💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2331300634)
64de3bb675e2ff3b8f0bb1de3d7680cbb2263807

take or leave suggestion: `addChunks` could take care of starting and stopping block building; block builder doesn't have to live outside of that function?

💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2331366938)
266500e01e996c6c08af31880545efb5568ded8b

MAX_REPLACEMENT_CANDIDATES documentation in rbf.h should get updated, including mentions at GetEntriesForConflicts

💬 instagibbs commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#issuecomment-3268061859)
reACK https://github.com/bitcoin/bitcoin/pull/33296/commits/8b6264768030db1840041abeeaeefd6c227a2644

💬 sipa commented on issue "[IBD] Raspberry Pi: 90% CPU time for 1.5% of block processing":
(https://github.com/bitcoin/bitcoin/issues/32832#issuecomment-3268062397)
Or run the libsecp256k1 benchmarks on both to see how fast signature verification is on both systems.

💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2331417443)
still prefer `main_only` to be explicitly called

💬 Crypt-iQ commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#issuecomment-3268086810)
> Maybe a simple bool flag failed_reconstruction on PartiallyDownloadedBlock would work? Feel free to ignore.

This would work, but I think we should wait until there is an in-repo fuzz harness with good coverage before trying to refactor the net_processing state or the `PartiallyDownloadedBlock` state. I think a bool `attempted_reconstruction` could replace the calls to `SetNull()` / `IsNull()`. An ideal solution would be to get rid of the unusable `PartiallyDownloadedBlock` while making sure
...

🤔 l0rinc reviewed a pull request: "common: Make arith_uint256 trivially copyable"
(https://github.com/bitcoin/bitcoin/pull/33332#pullrequestreview-3197960566)
Concept ACK, it makes sense to let the compiler generate code.

I'm not sure I agree with making these explicit, there's a reason the compiler can also generate them now - and we should let it. We don't have the move ctor/assignments and destructor either, seems like the Rule of Zero applies here: https://en.cppreference.com/w/cpp/language/rule_of_three.html#Rule_of_zero

But we would need to add a test to make the intention obvious - and maybe a benchmark, but I'm also fine with just providing
...

💬 l0rinc commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#discussion_r2331292846)
we've change the code and no tests were updated, which signals to me that our coverage is lacking: the CI won't complain when the current change is invalidated in any way.

We have an arith_uint256_tests already, we could add a simple test there that proves why we need this change (which fails before the change and passes after):
```patch
diff --git a/src/test/arith_uint256_tests.cpp b/src/test/arith_uint256_tests.cpp
--- a/src/test/arith_uint256_tests.cpp (revision 77c120d6bf1c214b30992ee5723c0
...

💬 l0rinc commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#discussion_r2331241070)
this is implicitly `noexcept` already, right? But we've added these lines exactly to be explicit, so I'd say we either add them or remove them completely

💬 l0rinc commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#discussion_r2331363171)
note that the self-assignment check is removed now

💬 Raimo33 commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#discussion_r2331452907)
a simple assert() seems logical and enough for such change.

💬 l0rinc commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#discussion_r2331464683)
As opposed to a `static_assert` you mean?

💬 Raimo33 commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#discussion_r2331466902)
no. I meant to ack your suggestion.

💬 stickies-v commented on pull request "RFC: blocks: add `-reobfuscate-blocks` arg to xor existing blk/rev on startup":
(https://github.com/bitcoin/bitcoin/pull/33324#issuecomment-3268176314)
Concept ACK, this seems like useful functionality to expose.

> Should we split ObfuscateBlocks out of init? I have split it into many local lambdas, but we may want to find better home for those methods...

I don't like using startup options for one-time operations (I feel the same about e.g. `-reindex`). Without having thought it through too much yet, maybe we can bundle this e.g. as part of `bitcoin-util` or a separate `bitcoin-xor-blocks` utility?

> Should we repurpose the existing -b
...

👍 hebasto approved a pull request: "guix: strip binaries in libexec"
(https://github.com/bitcoin/bitcoin/pull/33342#pullrequestreview-3198350176)
My Guix build:
```
aarch64
2042885891c2fdb6d3220da39e6f19b1a9852e3cca865db10f29e94fb2edc35d guix-build-3cceda9f4855/output/aarch64-linux-gnu/SHA256SUMS.part
f0e603d2a39bab4f4f81160584d6e27c90601c38e8302b813969214e797f16f8 guix-build-3cceda9f4855/output/aarch64-linux-gnu/bitcoin-3cceda9f4855-aarch64-linux-gnu-debug.tar.gz
410a706671693f8770474c797e35bb878961ff810e5a9bfb2bd19b4f069bfd0c guix-build-3cceda9f4855/output/aarch64-linux-gnu/bitcoin-3cceda9f4855-aarch64-linux-gnu.tar.gz
acc23910
...

🤔 mzumsande reviewed a pull request: "net: check for empty header before calling FillBlock"
(https://github.com/bitcoin/bitcoin/pull/33296#pullrequestreview-3198359933)
Code Review ACK 8b6264768030db1840041abeeaeefd6c227a2644

👍 hebasto approved a pull request: "guix: strip binaries in libexec"
(https://github.com/bitcoin/bitcoin/pull/33342#pullrequestreview-3198383238)
ACK 3cceda9f4855ac1f5df349f42efdbf5058d08f48, I've checked Guix build outputs.

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3268253082)
Should we make all the `main_only` arguments in TxGraph mandatory? Or even replace them with an enum like

```c++
enum class Level {
MAIN, //<! Always refers to level 0
STAGING, //<! Always refers to level 1 (fail if it doesn't exist)
TOP, //<! Refers to level 1 if it exists, 0 otherwise
};
```

?

🤔 l0rinc reviewed a pull request: "help: enrich help text for `-loadblock`"
(https://github.com/bitcoin/bitcoin/pull/33343#pullrequestreview-3198286952)
I'm not sure it's a documentation issue, I think we should rather fix it by adding the obfuscation key to the AutoFile instead.
I will investigate if adding a fix and updating `feature_loadblock.py` makes more sense and will push an alternative PR to see what people think is more appropriate here.
Thanks for jumping on this so quickly.

💬 l0rinc commented on pull request "help: enrich help text for `-loadblock`":
(https://github.com/bitcoin/bitcoin/pull/33343#discussion_r2331448834)
For context we could mention the discussions in https://github.com/bitcoin/bitcoin/issues/33280 that lead here.

nit: I'd personally would prefer calling it by it's function (obfuscation) instead of the implementation detail (xor)
```suggestion
argsman.AddArg("-loadblock=<file>", "Imports blocks from external file on startup (does not support obfuscated blocks)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
```

💬 l0rinc commented on pull request "help: enrich help text for `-loadblock`":
(https://github.com/bitcoin/bitcoin/pull/33343#issuecomment-3268288721)
I wasn't aware of this, it seems we have a dedicated tool to https://github.com/bitcoin/bitcoin/tree/master/contrib/linearize.
It's explicitly written to deobfuscate blocks from your node’s blk*.dat (handling XOR if present) and writes them plain to output_file or to an output/blkNNNNN.dat.