👍 laanwj approved a pull request: "doc: Add deps install notes for multiprocess"
(https://github.com/bitcoin/bitcoin/pull/32293#pullrequestreview-2776074231)
ACK ff4ea070aa4b1ed1ebb6dcf62e01c9dd57319f69
This needs to be mentioned. i think adding paragraph after paragraph of dependencies to the build docs like this becomes a bit of a mess for someone without context "what do i actually have to install", but that's not something that needs to be resolved here.
(https://github.com/bitcoin/bitcoin/pull/32293#pullrequestreview-2776074231)
ACK ff4ea070aa4b1ed1ebb6dcf62e01c9dd57319f69
This needs to be mentioned. i think adding paragraph after paragraph of dependencies to the build docs like this becomes a bit of a mess for someone without context "what do i actually have to install", but that's not something that needs to be resolved here.
💬 sipa commented on issue "Wallet should be able to store multiple transactions with same txid":
(https://github.com/bitcoin/bitcoin/issues/11240#issuecomment-2813238080)
I think this really calls for storing up to 2 variants of each transactions: the one we expect to confirm, and optionally the one we've seen accepted in a block.
Super hacky way of avoiding database upgrade issues: store inside the `wtx` record a concatenation of both serialized transactions. Old wallets (I think) will just read the first one, ignoring the second version if any.
(https://github.com/bitcoin/bitcoin/issues/11240#issuecomment-2813238080)
I think this really calls for storing up to 2 variants of each transactions: the one we expect to confirm, and optionally the one we've seen accepted in a block.
Super hacky way of avoiding database upgrade issues: store inside the `wtx` record a concatenation of both serialized transactions. Old wallets (I think) will just read the first one, ignoring the second version if any.
💬 Sjors commented on issue "Stratum v2 via IPC Mining Interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-2813323000)
All interface changes that I need for v30 have been merged!
Some progress has been made on enabling IPC for the release as well, but please review #31375 and https://github.com/bitcoin/bitcoin/pull/31802.
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-2813323000)
All interface changes that I need for v30 have been merged!
Some progress has been made on enabling IPC for the release as well, but please review #31375 and https://github.com/bitcoin/bitcoin/pull/31802.
💬 maflcko commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2813333039)
CI fails. Did this pass locally?
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2813333039)
CI fails. Did this pass locally?
💬 fanquake commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2813337803)
For reference (https://github.com/bitcoin/bitcoin/actions/runs/14517522442/job/40730094523?pr=32296#step:6:4644):
```bash
/home/runner/work/_temp/src/secp256k1/src/modinv64_impl.h:360:17: runtime error: implicit conversion from type 'uint64_t' (aka 'unsigned long') of value 7287561039273463316 (64-bit, unsigned) to type 'int' changed the value to 1893666324 (32-bit, signed)
#0 0x555bd4509ce3 in secp256k1_modinv64_posdivsteps_62_var /home/runner/work/_temp/src/secp256k1/src/modinv64_impl.h
...
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2813337803)
For reference (https://github.com/bitcoin/bitcoin/actions/runs/14517522442/job/40730094523?pr=32296#step:6:4644):
```bash
/home/runner/work/_temp/src/secp256k1/src/modinv64_impl.h:360:17: runtime error: implicit conversion from type 'uint64_t' (aka 'unsigned long') of value 7287561039273463316 (64-bit, unsigned) to type 'int' changed the value to 1893666324 (32-bit, signed)
#0 0x555bd4509ce3 in secp256k1_modinv64_posdivsteps_62_var /home/runner/work/_temp/src/secp256k1/src/modinv64_impl.h
...
👍 stickies-v approved a pull request: "qt, wallet: Convert uint256 to Txid"
(https://github.com/bitcoin/bitcoin/pull/32238#pullrequestreview-2776194854)
ACK afbb1a1bfd729dc5e92cc5a83be681999a2d3b43
I checked all the callsites of functions with changed signatures and verified that indeed `Txid` is the correct type.
(https://github.com/bitcoin/bitcoin/pull/32238#pullrequestreview-2776194854)
ACK afbb1a1bfd729dc5e92cc5a83be681999a2d3b43
I checked all the callsites of functions with changed signatures and verified that indeed `Txid` is the correct type.
💬 maflcko commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2813376299)
> While this wasn't the main motivation for the change, IBD on Ubuntu/GCC on SSD with i9 indicates a 2% speedup as well:
If IBD speedup is not the main motivation, what else is the motivation, given that the title mentions `[IBD]`?
Also, what code part is hitting this during IBD, given that reads and writes are now buffered?
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2813376299)
> While this wasn't the main motivation for the change, IBD on Ubuntu/GCC on SSD with i9 indicates a 2% speedup as well:
If IBD speedup is not the main motivation, what else is the motivation, given that the title mentions `[IBD]`?
Also, what code part is hitting this during IBD, given that reads and writes are now buffered?
📝 l0rinc converted_to_draft a pull request: "[IBD] specialize block serialization"
(https://github.com/bitcoin/bitcoin/pull/31868)
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)
### Summary
This PR contain a few different optimization I found by IBD profiling, and via the newly added block seralization benchmarks. It also takes advantage of the recently merged [`std::span` changes](https://github.com/bitcoin/bitcoin/pull/31519) enabling propagating static extents.
The commits merge similar (de)serialization methods, and separates th
...
(https://github.com/bitcoin/bitcoin/pull/31868)
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)
### Summary
This PR contain a few different optimization I found by IBD profiling, and via the newly added block seralization benchmarks. It also takes advantage of the recently merged [`std::span` changes](https://github.com/bitcoin/bitcoin/pull/31519) enabling propagating static extents.
The commits merge similar (de)serialization methods, and separates th
...
💬 l0rinc commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#discussion_r2049233469)
Yes:
> typedef unsigned char uint8_t;
(https://github.com/bitcoin/bitcoin/pull/31868#discussion_r2049233469)
Yes:
> typedef unsigned char uint8_t;
💬 l0rinc commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2813392140)
> what code part is hitting this during IBD
During serialization we have very many 1 byte writes, this change avoids the heavy allocations.
But drafting until I have time to properly remeasure everything after the recent merge.
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2813392140)
> what code part is hitting this during IBD
During serialization we have very many 1 byte writes, this change avoids the heavy allocations.
But drafting until I have time to properly remeasure everything after the recent merge.
💬 maflcko commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2813398761)
I understand that there are many 1-byte writes, but they go into the BufferedReader/Writer, which is not modified here. The Buffered* then passes them to AutoFile, which is modified here, but never receives 1-byte writes anymore
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2813398761)
I understand that there are many 1-byte writes, but they go into the BufferedReader/Writer, which is not modified here. The Buffered* then passes them to AutoFile, which is modified here, but never receives 1-byte writes anymore
💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049250853)
Good call, done. Made me realize I should update `vMatch` in the parent PR, so thanks.
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049250853)
Good call, done. Made me realize I should update `vMatch` in the parent PR, so thanks.
💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049251124)
Done.
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049251124)
Done.
💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049251391)
Done.
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049251391)
Done.
💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049251829)
Done.
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049251829)
Done.
💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049252754)
Done.
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049252754)
Done.
💬 ryanofsky commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2813429363)
Code review ff4ea070aa4b1ed1ebb6dcf62e01c9dd57319f69
Thanks for adding this. Somehow I thought the capnproto dependencies were already listed, but that change is sitting in a commit in #10102 (bbcbe85cef21049b5f321265264e2c4fe394d209).
Few suggestions:
- Would suggest changing "Multiprocess Dependencies" to "IPC Dependencies" since the only thing they are use for on master is -ipcbind feature, and multiprocess functionality is a superset of IPC functionality.
- Might be good to state e
...
(https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2813429363)
Code review ff4ea070aa4b1ed1ebb6dcf62e01c9dd57319f69
Thanks for adding this. Somehow I thought the capnproto dependencies were already listed, but that change is sitting in a commit in #10102 (bbcbe85cef21049b5f321265264e2c4fe394d209).
Few suggestions:
- Would suggest changing "Multiprocess Dependencies" to "IPC Dependencies" since the only thing they are use for on master is -ipcbind feature, and multiprocess functionality is a superset of IPC functionality.
- Might be good to state e
...
💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049254166)
I'll leave this as is for now. A bit out of scope for this PR.
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049254166)
I'll leave this as is for now. A bit out of scope for this PR.
💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049254712)
Agreed, done.
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049254712)
Agreed, done.
💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049256701)
Done. I love structured bindings now.
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049256701)
Done. I love structured bindings now.