💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2330732155)
7ddce57e96aff26fb2a5abc1009e6baef24d5bab

look for some trivial coverage of this, or directly use this in GetVirtualTransactionSize?

💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2331041173)
Can `Assume(!m_dependencies_processed);`

💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2330691874)
56999a7085f21c8d462fd5afdfbb41dd93df423f

now is a great time to document what this thing is

💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2331064925)
34251141eca785578a0b2ca6d1c0932d34b2418d

I didn't try implementing but have you considered simply processing dependencies as you go in PreChecks->StageAddition? Then we don't have to track whether we've applied deps.

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

this TODO is addressed in this commit

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

do we need `dynamic_cast` here since we know the underlying type vs static_cast?

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