💬 Raimo33 commented on issue "[IBD] Raspberry Pi: 90% CPU time for 1.5% of block processing":
(https://github.com/bitcoin/bitcoin/issues/32832#issuecomment-3268049230)
> the asm optimizations aren't even enabled by default in Bitcoin Core builds.

Is there a specific reason for this?

💬 sipa commented on issue "[IBD] Raspberry Pi: 90% CPU time for 1.5% of block processing":
(https://github.com/bitcoin/bitcoin/issues/32832#issuecomment-3268056766)
Historically, because they were new and unreviewed, and after that I guess we forgot about them (sorry, @laanwj ...).

💬 Raimo33 commented on issue "[IBD] Raspberry Pi: 90% CPU time for 1.5% of block processing":
(https://github.com/bitcoin/bitcoin/issues/32832#issuecomment-3268058223)
@l0rinc if I were you I would setup a simple benchmark to test how many cycles it takes to execute a single 64x64->128 multiplication. and compare that with your i7.

🤔 instagibbs reviewed a pull request: "Cluster mempool implementation"
(https://github.com/bitcoin/bitcoin/pull/28676#pullrequestreview-3197152141)
few comments through "initial implementation"

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

this block is already public

💬 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