๐ Raimo33 opened a pull request: "crypto: optimize SipHash Write() method with chunked processing"
(https://github.com/bitcoin/bitcoin/pull/33325)
## Summary
The current [Write()](https://github.com/bitcoin/bitcoin/blob/master/src/crypto/siphash.cpp#L48) implementation of Siphash uses a byte-by-byte approach to iterate the span. This resulted in significant overhead
for large inputs due to repeated bounds checking and span manipulations, without any help from the compiler.
This PR aims at optimizing Siphash by replacing byte-by-byte processing in CSipHasher::Write() with an optimized
chunked approach that processes data in 8-byte a
...
(https://github.com/bitcoin/bitcoin/pull/33325)
## Summary
The current [Write()](https://github.com/bitcoin/bitcoin/blob/master/src/crypto/siphash.cpp#L48) implementation of Siphash uses a byte-by-byte approach to iterate the span. This resulted in significant overhead
for large inputs due to repeated bounds checking and span manipulations, without any help from the compiler.
This PR aims at optimizing Siphash by replacing byte-by-byte processing in CSipHasher::Write() with an optimized
chunked approach that processes data in 8-byte a
...
๐ฌ kevkevinpal commented on pull request "ci: reduce runner sizes on various jobs":
(https://github.com/bitcoin/bitcoin/pull/33319#issuecomment-3263065907)
ACK [5eeb2fa](https://github.com/bitcoin/bitcoin/pull/33319/commits/5eeb2facbbbbf68a2c30ef9e6747e39c85d7b116)
Looks good to me, makes sense to me to use smaller runners.
All the checks passed on this PR and the longest running one only too 44 minutes
is there any reason to keep `CentOS, depends, gui` and `MSan, depends` on a `lg` runner?
(https://github.com/bitcoin/bitcoin/pull/33319#issuecomment-3263065907)
ACK [5eeb2fa](https://github.com/bitcoin/bitcoin/pull/33319/commits/5eeb2facbbbbf68a2c30ef9e6747e39c85d7b116)
Looks good to me, makes sense to me to use smaller runners.
All the checks passed on this PR and the longest running one only too 44 minutes
is there any reason to keep `CentOS, depends, gui` and `MSan, depends` on a `lg` runner?
โ ๏ธ HTM31-cyber opened an issue: "htm.sheng@gmail.com"
(https://github.com/bitcoin/bitcoin/issues/33326)
(https://github.com/bitcoin/bitcoin/issues/33326)
๐ฌ l0rinc commented on pull request "crypto: optimize SipHash Write() method with chunked processing":
(https://github.com/bitcoin/bitcoin/pull/33325#issuecomment-3263170325)
This seems like a good idea to me, I will review this in more detail soon.
To set your expectations, these fundamental changes can take a long time to review - and we need very good reasons to risk introducing a potential error and making the code more complicated for a speedup that the users might never notice.
I like that you're explaining it in a lot of detail in the commit message, it's not a trivial change! We may be able to use more C++20 alignment-specific methods there (e.g. like t
...
(https://github.com/bitcoin/bitcoin/pull/33325#issuecomment-3263170325)
This seems like a good idea to me, I will review this in more detail soon.
To set your expectations, these fundamental changes can take a long time to review - and we need very good reasons to risk introducing a potential error and making the code more complicated for a speedup that the users might never notice.
I like that you're explaining it in a lot of detail in the commit message, it's not a trivial change! We may be able to use more C++20 alignment-specific methods there (e.g. like t
...
โ
willcl-ark closed an issue: "htm.sheng@gmail.com"
(https://github.com/bitcoin/bitcoin/issues/33326)
(https://github.com/bitcoin/bitcoin/issues/33326)
โ ๏ธ HTM31-cyber opened an issue: "I am sharing 'OKX_1753235667059' with you"
(https://github.com/bitcoin/bitcoin/issues/33327)
https://d.docs.live.net/e9047f02d0a49392/Pictures/ยงโฌNTFโฯยง by HTM/OKX_1753235667059.jpg
One app for all your Word, Excel, PowerPoint and PDF needs. Get the Microsoft 365 Copilot app: https://aka.ms/GetM365
(https://github.com/bitcoin/bitcoin/issues/33327)
https://d.docs.live.net/e9047f02d0a49392/Pictures/ยงโฌNTFโฯยง by HTM/OKX_1753235667059.jpg
One app for all your Word, Excel, PowerPoint and PDF needs. Get the Microsoft 365 Copilot app: https://aka.ms/GetM365
โ
pinheadmz closed an issue: "I am sharing 'OKX_1753235667059' with you"
(https://github.com/bitcoin/bitcoin/issues/33327)
(https://github.com/bitcoin/bitcoin/issues/33327)
๐ฌ hebasto commented on pull request "Fix compatibility with `-debuglogfile` command-line option":
(https://github.com/bitcoin/bitcoin/pull/33215#discussion_r2328354097)
Thanks! Fixed.
(https://github.com/bitcoin/bitcoin/pull/33215#discussion_r2328354097)
Thanks! Fixed.
๐ฌ hebasto commented on pull request "Fix compatibility with `-debuglogfile` command-line option":
(https://github.com/bitcoin/bitcoin/pull/33215#discussion_r2328354145)
Thanks! Fixed.
(https://github.com/bitcoin/bitcoin/pull/33215#discussion_r2328354145)
Thanks! Fixed.
๐ฌ Raimo33 commented on pull request "refactor: unify container presence checks (without PR conflicts)":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3263190690)
ACK f70d2c7faa8f7d724e146e4c409de9c6778b7299
This is a no brainer for me. It makes code easier to read and therefore mantain. Also, as already mentioned, for multisets and multimaps, `::contains` will avoid extra iterations with early exits.
The changes are relatively trivial so very little surface for subtle errors.
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3263190690)
ACK f70d2c7faa8f7d724e146e4c409de9c6778b7299
This is a no brainer for me. It makes code easier to read and therefore mantain. Also, as already mentioned, for multisets and multimaps, `::contains` will avoid extra iterations with early exits.
The changes are relatively trivial so very little surface for subtle errors.
๐ iajhff opened a pull request: "Mapping for Lockedpool"
(https://github.com/bitcoin/bitcoin/pull/33328)
support: Optimize LockedPool::free with O(log n) arena lookup
Replace O(n) linear search with O(log n) map-based lookup using
arena base addresses. Improves performance for frequent
allocation/deallocation patterns in wallet operations.
(https://github.com/bitcoin/bitcoin/pull/33328)
support: Optimize LockedPool::free with O(log n) arena lookup
Replace O(n) linear search with O(log n) map-based lookup using
arena base addresses. Improves performance for frequent
allocation/deallocation patterns in wallet operations.
๐ฌ l0rinc commented on issue "-loadblock doesn't work":
(https://github.com/bitcoin/bitcoin/issues/33280#issuecomment-3263193876)
I'm not sure I can follow what versions you're using in the examples and what you're trying to achieve high level - could you please update the title and issue description and start with the zoomed out problem definition for what you're trying to achieve and what you tried and what versions you have used for which operation etc?
(https://github.com/bitcoin/bitcoin/issues/33280#issuecomment-3263193876)
I'm not sure I can follow what versions you're using in the examples and what you're trying to achieve high level - could you please update the title and issue description and start with the zoomed out problem definition for what you're trying to achieve and what you tried and what versions you have used for which operation etc?
๐ฌ Raimo33 commented on pull request "crypto: optimize SipHash Write() method with chunked processing":
(https://github.com/bitcoin/bitcoin/pull/33325#issuecomment-3263344271)
I just realized somebody had my same idea on knots28. But in the latest version it got reset to match Core's implementation. ๐ค.
https://github.com/Raimo33/bitcoinknots/commit/8463aef09fe4c6e3806e3b8a4c967c297302b96f
(https://github.com/bitcoin/bitcoin/pull/33325#issuecomment-3263344271)
I just realized somebody had my same idea on knots28. But in the latest version it got reset to match Core's implementation. ๐ค.
https://github.com/Raimo33/bitcoinknots/commit/8463aef09fe4c6e3806e3b8a4c967c297302b96f
๐ฌ Raimo33 commented on pull request "crypto: optimize SipHash Write() method with chunked processing":
(https://github.com/bitcoin/bitcoin/pull/33325#issuecomment-3263377003)
> We may be able to use more C++20 alignment-specific methods there (e.g. like the `std::assume_aligned` we used in
I tried with
```diff
- uint64_t chunk;
- std::memcpy(&chunk, ptr, 8);
+ auto aligned_ptr = std::assume_aligned<8>(ptr);
+ uint64_t chunk = *reinterpret_cast<const uint64_t *>(aligned_ptr);
```
but no luck. performance is identical. `memcpy` is extremely optimized.
(https://github.com/bitcoin/bitcoin/pull/33325#issuecomment-3263377003)
> We may be able to use more C++20 alignment-specific methods there (e.g. like the `std::assume_aligned` we used in
I tried with
```diff
- uint64_t chunk;
- std::memcpy(&chunk, ptr, 8);
+ auto aligned_ptr = std::assume_aligned<8>(ptr);
+ uint64_t chunk = *reinterpret_cast<const uint64_t *>(aligned_ptr);
```
but no luck. performance is identical. `memcpy` is extremely optimized.
๐ฌ systemic-threat commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3263510323)
Concept NACK
I've educated myself on the issue.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3263510323)
Concept NACK
I've educated myself on the issue.
๐ฌ stwenhao commented on issue "Potential systemic risk: OP_RETURN outputs as unintended governance signaling":
(https://github.com/bitcoin/bitcoin/issues/33323#issuecomment-3263529896)
The same things can be done without OP_RETURN. Even if you have P2PK-only, old, plain, ECDSA signatures, then still: they can commit to arbitrary data, through their R-values.
> Each UTXO could embed a short marker in its OP_RETURN.
Not only in OP_RETURN. Since miners started accepting out-of-band transactions, then different transaction versions can be used to signal things, just like they are used during soft-forks, to check, how many miners are ready to support some BIP.
> These markers co
...
(https://github.com/bitcoin/bitcoin/issues/33323#issuecomment-3263529896)
The same things can be done without OP_RETURN. Even if you have P2PK-only, old, plain, ECDSA signatures, then still: they can commit to arbitrary data, through their R-values.
> Each UTXO could embed a short marker in its OP_RETURN.
Not only in OP_RETURN. Since miners started accepting out-of-band transactions, then different transaction versions can be used to signal things, just like they are used during soft-forks, to check, how many miners are ready to support some BIP.
> These markers co
...
๐ฌ ybaidiuk commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3263615631)
Gloria destroying bitcoin.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3263615631)
Gloria destroying bitcoin.
๐ฌ maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2328596111)
Not sure what your question is, can you link to logs that explain what you are observing and asking about?
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2328596111)
Not sure what your question is, can you link to logs that explain what you are observing and asking about?
๐ฌ maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2328596193)
ah, restored doc
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2328596193)
ah, restored doc
๐ฌ maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3263622408)
(doc-only force push to restore the explanation)
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3263622408)
(doc-only force push to restore the explanation)