💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569042469)
Done
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569042469)
Done
💬 maflcko commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2568775620)
pls use named args and snake_case:
```suggestion
str += " " + tx_in.ToString(/*include_witness=*/false) + "\n";
```
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2568775620)
pls use named args and snake_case:
```suggestion
str += " " + tx_in.ToString(/*include_witness=*/false) + "\n";
```
💬 fanquake commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-3586302030)
Guix Build (x86_64):
```bash
cc72adc0e567f33605050a98cd7cd08db2b0c7da1aa11dd5f73b9077653b5e00 guix-build-3e01b5d0e7be/output/aarch64-linux-gnu/SHA256SUMS.part
15d23cda22ba4afceddb733820893e4ef3a5307717002403cdc5e66d3561bafb guix-build-3e01b5d0e7be/output/aarch64-linux-gnu/bitcoin-3e01b5d0e7be-aarch64-linux-gnu-debug.tar.gz
2d0e852259c802ea3c2411a38b93148dcaa2415b5d5d34d1f6362df39108e718 guix-build-3e01b5d0e7be/output/aarch64-linux-gnu/bitcoin-3e01b5d0e7be-aarch64-linux-gnu.tar.gz
b9c4572
...
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-3586302030)
Guix Build (x86_64):
```bash
cc72adc0e567f33605050a98cd7cd08db2b0c7da1aa11dd5f73b9077653b5e00 guix-build-3e01b5d0e7be/output/aarch64-linux-gnu/SHA256SUMS.part
15d23cda22ba4afceddb733820893e4ef3a5307717002403cdc5e66d3561bafb guix-build-3e01b5d0e7be/output/aarch64-linux-gnu/bitcoin-3e01b5d0e7be-aarch64-linux-gnu-debug.tar.gz
2d0e852259c802ea3c2411a38b93148dcaa2415b5d5d34d1f6362df39108e718 guix-build-3e01b5d0e7be/output/aarch64-linux-gnu/bitcoin-3e01b5d0e7be-aarch64-linux-gnu.tar.gz
b9c4572
...
🤔 hodlinator reviewed a pull request: "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-3515742822)
Reviewed e3b02f1539e59a6a20a00b4161219b5aba6fefed
Thanks for reverting to a simpler version of this change!
2 inline questions.
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-3515742822)
Reviewed e3b02f1539e59a6a20a00b4161219b5aba6fefed
Thanks for reverting to a simpler version of this change!
2 inline questions.
💬 hodlinator commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2568982833)
in 45dc62f0edf71d52ae6b024dec6b11746e3c4076 "refactor: make `MerkleRoot` benchmark more representative": Sending in the `&mutation` parameter makes the function perform more work (somewhat quadratically?). Are we sure we want to drop that?
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2568982833)
in 45dc62f0edf71d52ae6b024dec6b11746e3c4076 "refactor: make `MerkleRoot` benchmark more representative": Sending in the `&mutation` parameter makes the function perform more work (somewhat quadratically?). Are we sure we want to drop that?
💬 hodlinator commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2569096555)
`(size + 1) & -2` eh? :)
I'm happy with `(size + 1) & ~1ULL`. It should narrow down to 4-byte int on 32-bit.
Diffed `ull_literal` vs `decltype_trick` for x86-64 and ARM32 and they are the same. :+1:
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2569096555)
`(size + 1) & -2` eh? :)
I'm happy with `(size + 1) & ~1ULL`. It should narrow down to 4-byte int on 32-bit.
Diffed `ull_literal` vs `decltype_trick` for x86-64 and ARM32 and they are the same. :+1:
💬 hodlinator commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2569010822)
nit in 6c2d181bdc09024c5de5e29d9e913fcc2dba5071 "test: adjust `ComputeMerkleRoot` tests": Why extract `vtx_count` when it's only used once?
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2569010822)
nit in 6c2d181bdc09024c5de5e29d9e913fcc2dba5071 "test: adjust `ComputeMerkleRoot` tests": Why extract `vtx_count` when it's only used once?
✅ fanquake closed an issue: "guix: Unable to reproduce macOS SDK tarball on Fedora 40"
(https://github.com/bitcoin/bitcoin/issues/31873)
(https://github.com/bitcoin/bitcoin/issues/31873)
🚀 fanquake merged a pull request: "contrib: turn off compression of macOS SDK to fix determinism (across distros)"
(https://github.com/bitcoin/bitcoin/pull/32009)
(https://github.com/bitcoin/bitcoin/pull/32009)
💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2569155777)
My reasoning was that I want the benchmark to reflect the actual usage, see: https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-2973633245
It wouldn't be fair to see the `ComputeMerkleRoot` performance without the leaf constructions (and get an unrealistic speedup). What do you think?
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2569155777)
My reasoning was that I want the benchmark to reflect the actual usage, see: https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-2973633245
It wouldn't be fair to see the `ComputeMerkleRoot` performance without the leaf constructions (and get an unrealistic speedup). What do you think?
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569185787)
> peer=%d%s", node.GetId(), node.LogIP(fLogIPs)
This was improved by https://github.com/bitcoin/bitcoin/pull/28521 (my comment on it: https://github.com/bitcoin/bitcoin/pull/28521#pullrequestreview-2354306033), but maybe it can be improved further. Anyway this pattern is being repeated in many places, so this issue is bigger than the current PR. Concept ACK on further dedups :)
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2569185787)
> peer=%d%s", node.GetId(), node.LogIP(fLogIPs)
This was improved by https://github.com/bitcoin/bitcoin/pull/28521 (my comment on it: https://github.com/bitcoin/bitcoin/pull/28521#pullrequestreview-2354306033), but maybe it can be improved further. Anyway this pattern is being repeated in many places, so this issue is bigger than the current PR. Concept ACK on further dedups :)
💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2569184143)
Thanks for checking, I think it's the best solution regarding C++ simplicity and CPU support.
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2569184143)
Thanks for checking, I think it's the best solution regarding C++ simplicity and CPU support.
💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2569177747)
Good point, I can use it in a [few more places](https://github.com/bitcoin/bitcoin/compare/e3b02f1539e59a6a20a00b4161219b5aba6fefed..14a4dca89adaab6f1ad42af6716f9065f1abe2e1), pushed.
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2569177747)
Good point, I can use it in a [few more places](https://github.com/bitcoin/bitcoin/compare/e3b02f1539e59a6a20a00b4161219b5aba6fefed..14a4dca89adaab6f1ad42af6716f9065f1abe2e1), pushed.
🤔 l0rinc reviewed a pull request: "validation: fetch block inputs on parallel threads >20% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-3515011880)
This new design achieves the best results I've seen so far across all platforms measured, excellent job!!
It evolved from an `InputFetcher` helper (where the parallelism happened before `ConnectBlock` was called) to `CoinsViewCacheAsync`, a multithreaded `CCoinsViewCache` subclass whose worker threads run in parallel with `ConnectBlock` itself.
Internal spends are still filtered using short txid prefixes stored in a sorted `std::vector` and checked via `std::binary_search`. In the rare case of
...
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-3515011880)
This new design achieves the best results I've seen so far across all platforms measured, excellent job!!
It evolved from an `InputFetcher` helper (where the parallelism happened before `ConnectBlock` was called) to `CoinsViewCacheAsync`, a multithreaded `CCoinsViewCache` subclass whose worker threads run in parallel with `ConnectBlock` itself.
Internal spends are still filtered using short txid prefixes stored in a sorted `std::vector` and checked via `std::binary_search`. In the rare case of
...
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568405428)
nit: other comments use the bare verb form
```suggestion
* Retrieve the coin from the cache even if it is spent, without calling
```
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568405428)
nit: other comments use the bare verb form
```suggestion
* Retrieve the coin from the cache even if it is spent, without calling
```
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568602665)
I wonder if we could change this benchmark to use an actual LevelDB-backed view instead of a synthetic delay – and maybe reuse the setup from https://github.com/bitcoin/bitcoin/pull/32554 for a more realistic measurement?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568602665)
I wonder if we could change this benchmark to use an actual LevelDB-backed view instead of a synthetic delay – and maybe reuse the setup from https://github.com/bitcoin/bitcoin/pull/32554 for a more realistic measurement?
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568436113)
This highlights that our dbcache layering is a bit messy – having to static_cast the base view to CCoinsViewCache here is quite ugly. It would be good to clean this up in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568436113)
This highlights that our dbcache layering is a bit messy – having to static_cast the base view to CCoinsViewCache here is quite ugly. It would be good to clean this up in a follow-up.
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568645746)
Why do we need this cache to be constructed with deterministic=true here? Is the deterministic ordering actually required for the fuzz harness, or could we drop that?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568645746)
Why do we need this cache to be constructed with deterministic=true here? Is the deterministic ordering actually required for the fuzz harness, or could we drop that?
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568636267)
nit: the formatter should enforce a trailing newline at the end of C++ source files; could you add one here?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568636267)
nit: the formatter should enforce a trailing newline at the end of C++ source files; could you add one here?
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568638933)
What if we added a second run inside the same benchmark that calls `StartFetching` again or a series of `AccessCoin` calls - to exercise the “everything is already in the cache” path (similar to a large -dbcache scenario)?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2568638933)
What if we added a second run inside the same benchmark that calls `StartFetching` again or a series of `AccessCoin` calls - to exercise the “everything is already in the cache” path (similar to a large -dbcache scenario)?