💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237124698)
We need `kernel_BlockPointer` because the validation interface gives us a non-owning reference. It would imo be a lot nicer if we could generalize this into `kernel_Block`, so I have opened #33078 to improve ownership semantics in the validation interface, after which `kernel_BlockPointer` can then be removed, e.g. as in https://github.com/stickies-v/bitcoin/commits/kernel/remove-blockpointer/
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237124698)
We need `kernel_BlockPointer` because the validation interface gives us a non-owning reference. It would imo be a lot nicer if we could generalize this into `kernel_Block`, so I have opened #33078 to improve ownership semantics in the validation interface, after which `kernel_BlockPointer` can then be removed, e.g. as in https://github.com/stickies-v/bitcoin/commits/kernel/remove-blockpointer/
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2216251677)
Since we use reference counting here, would it be useful to document that in the documentation?
```suggestion
* Destroy the block. Handle is invalidated immediately, block is destroyed as soon as no references remain.
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2216251677)
Since we use reference counting here, would it be useful to document that in the documentation?
```suggestion
* Destroy the block. Handle is invalidated immediately, block is destroyed as soon as no references remain.
```
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237111430)
I would prefer to expose `kernel_TransactionUndo` and `kernel_Coin` handles so we can generalize iterating over these nested containers.
Using shared_ptr and aliasing constructors, we can do so without incurring any copies (but at the cost of allocating shared_ptr and incrementing the reference counter). In my view this is both more ergonomic (by exposing dedicated types) and performant (by avoiding the need for any `_copy` operations (except for the `kernel_ByteArray` ones).
If necessary,
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237111430)
I would prefer to expose `kernel_TransactionUndo` and `kernel_Coin` handles so we can generalize iterating over these nested containers.
Using shared_ptr and aliasing constructors, we can do so without incurring any copies (but at the cost of allocating shared_ptr and incrementing the reference counter). In my view this is both more ergonomic (by exposing dedicated types) and performant (by avoiding the need for any `_copy` operations (except for the `kernel_ByteArray` ones).
If necessary,
...
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2219798945)
We should probably use `cast_transaction_output` here to make sure we're not miscasting anything?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2219798945)
We should probably use `cast_transaction_output` here to make sure we're not miscasting anything?
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237140589)
I think the implementation would be a lot cleaner and safer if we didn't use `reinterpret_cast` (or only in rare, targeted cases) but instead just implement the `kernel_` structs in the .cpp file (keeping it hidden to the user). It does add minimal overhead by allocating a (usually trivial) struct, but in most cases that is infrequent and the cost negligible. If necessary, we could still re-introduce `reinterpret_cast` in places where we observe that it does affect performance, but I would stron
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237140589)
I think the implementation would be a lot cleaner and safer if we didn't use `reinterpret_cast` (or only in rare, targeted cases) but instead just implement the `kernel_` structs in the .cpp file (keeping it hidden to the user). It does add minimal overhead by allocating a (usually trivial) struct, but in most cases that is infrequent and the cost negligible. If necessary, we could still re-introduce `reinterpret_cast` in places where we observe that it does affect performance, but I would stron
...
💬 fanquake commented on pull request "guix: warn SOURCE_DATE_EPOCH set in guix-codesign":
(https://github.com/bitcoin/bitcoin/pull/33073#issuecomment-3128081665)
Guix Build (aarch64):
```bash
b8961023686b6aa3cef95f9f938c1c4b9c1e87945ea60c994dbe56c430faf43e guix-build-1bed0f734b3f/output/aarch64-linux-gnu/SHA256SUMS.part
90f9c040b2640cc7840cdb9341170c754b107ef7c681c762e8d69e3641378322 guix-build-1bed0f734b3f/output/aarch64-linux-gnu/bitcoin-1bed0f734b3f-aarch64-linux-gnu-debug.tar.gz
7ead728efa409a754eea8cd9a41471fc65395e3d8020fd78ba8ad5b8a4dce5ba guix-build-1bed0f734b3f/output/aarch64-linux-gnu/bitcoin-1bed0f734b3f-aarch64-linux-gnu.tar.gz
8c849e
...
(https://github.com/bitcoin/bitcoin/pull/33073#issuecomment-3128081665)
Guix Build (aarch64):
```bash
b8961023686b6aa3cef95f9f938c1c4b9c1e87945ea60c994dbe56c430faf43e guix-build-1bed0f734b3f/output/aarch64-linux-gnu/SHA256SUMS.part
90f9c040b2640cc7840cdb9341170c754b107ef7c681c762e8d69e3641378322 guix-build-1bed0f734b3f/output/aarch64-linux-gnu/bitcoin-1bed0f734b3f-aarch64-linux-gnu-debug.tar.gz
7ead728efa409a754eea8cd9a41471fc65395e3d8020fd78ba8ad5b8a4dce5ba guix-build-1bed0f734b3f/output/aarch64-linux-gnu/bitcoin-1bed0f734b3f-aarch64-linux-gnu.tar.gz
8c849e
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237230747)
This should get de-allocated when the user calls `kernel_block_destroy`, no?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237230747)
This should get de-allocated when the user calls `kernel_block_destroy`, no?
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237234545)
This was not relevant so far, since nothing could increment the reference count permanently. Will add once we have something that would exercise that case.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237234545)
This was not relevant so far, since nothing could increment the reference count permanently. Will add once we have something that would exercise that case.
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237265558)
They can't, we return `nullptr`. The failure branch needs to include `delete block` (or alternatively, as suggested, just instantiate `block` as a `std::unique_ptr` and then promote it to `shared_ptr` if reading succeeds.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237265558)
They can't, we return `nullptr`. The failure branch needs to include `delete block` (or alternatively, as suggested, just instantiate `block` as a `std::unique_ptr` and then promote it to `shared_ptr` if reading succeeds.
💬 furszy commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3128153057)
> and validation for RSA and secp256r1, I don't think an internal implementation would actually be a major blocker. These implementations can even be relatively slow and naive as they're just validation only, as you've done in dnssec-prover. I'm planning to dig into that next week.
Just off the top of my head, a minimalistic approach would be to implement an ASN.1 DER parser. For RSA, we’d extend our arith_uint to support 4096-bit numbers with a mod operator (doubling the modulus size as need
...
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3128153057)
> and validation for RSA and secp256r1, I don't think an internal implementation would actually be a major blocker. These implementations can even be relatively slow and naive as they're just validation only, as you've done in dnssec-prover. I'm planning to dig into that next week.
Just off the top of my head, a minimalistic approach would be to implement an ASN.1 DER parser. For RSA, we’d extend our arith_uint to support 4096-bit numbers with a mod operator (doubling the modulus size as need
...
👍 andrewtoth approved a pull request: "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline"
(https://github.com/bitcoin/bitcoin/pull/32279#pullrequestreview-3063819192)
ACK d5104cfbaeb82081e4b00a5084516555e446dcdc
(https://github.com/bitcoin/bitcoin/pull/32279#pullrequestreview-3063819192)
ACK d5104cfbaeb82081e4b00a5084516555e446dcdc
🤔 stratospher reviewed a pull request: "test: refactor mempool_accept_wtxid"
(https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3063408783)
Concept ACK. I like how you've used `MiniWallet` to remove the signing bits (`signrawtransactionwithkey`) which needed the private key.
You can simplify this further. `ValidWitnessMalleatedTx` returned parent transaction and children transaction in 2 different steps because the children transactions needed the parent's txid which required signing with private key. Now since we don't do `signrawtransactionwithkey`, you can replace class `ValidWitnessMalleatedTx` with a function `valid_witnes
...
(https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3063408783)
Concept ACK. I like how you've used `MiniWallet` to remove the signing bits (`signrawtransactionwithkey`) which needed the private key.
You can simplify this further. `ValidWitnessMalleatedTx` returned parent transaction and children transaction in 2 different steps because the children transactions needed the parent's txid which required signing with private key. Now since we don't do `signrawtransactionwithkey`, you can replace class `ValidWitnessMalleatedTx` with a function `valid_witnes
...
💬 stratospher commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2236986215)
```suggestion
parent = wallet.send_to(from_node=node, scriptPubKey=txgen.get_script_pubkey(), amount=int(9.99998 * COIN), fee=1000)
```
style nit: space after , (here and in other places too)
(https://github.com/bitcoin/bitcoin/pull/33067#discussion_r2236986215)
```suggestion
parent = wallet.send_to(from_node=node, scriptPubKey=txgen.get_script_pubkey(), amount=int(9.99998 * COIN), fee=1000)
```
style nit: space after , (here and in other places too)
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237313429)
Right, there is also no test for this :(. Will fix and add a test.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237313429)
Right, there is also no test for this :(. Will fix and add a test.
💬 stickies-v commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2237359282)
Sorry, need to read up on this thread properly in the next few days, but quickly wanted to add: moving `info_for_relay` to `net_processing.cpp` absolutely makes sense to me, that logic should not be in mempool interface. However, leaking mempool internals into p2p (through `mempool.GetIter(id)`) I really don't like.
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2237359282)
Sorry, need to read up on this thread properly in the next few days, but quickly wanted to add: moving `info_for_relay` to `net_processing.cpp` absolutely makes sense to me, that logic should not be in mempool interface. However, leaking mempool internals into p2p (through `mempool.GetIter(id)`) I really don't like.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3128247787)
Thank you so much for the review @stickies-v! Picked off the smaller items here, while we are still experimenting with the rest:
Updated 938767d957b7669accfb554a7cbb25141f7e8632 -> 4eb1c66dbdaf35cbc480cb201f6019bc0f5fde95 ([kernelApi_46](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_46) -> [kernelApi_47](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_47), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_46..kernelApi_47)).
* Addressed @stickies-v's [comm
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3128247787)
Thank you so much for the review @stickies-v! Picked off the smaller items here, while we are still experimenting with the rest:
Updated 938767d957b7669accfb554a7cbb25141f7e8632 -> 4eb1c66dbdaf35cbc480cb201f6019bc0f5fde95 ([kernelApi_46](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_46) -> [kernelApi_47](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_47), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_46..kernelApi_47)).
* Addressed @stickies-v's [comm
...
💬 marcofleon commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#issuecomment-3128251488)
Most recent push updates the `m_tx_inventory_to_send` comment and adds a clarifying comment as to why we create the inv before checking `m_tx_inventory_known_filter`.
I'm still considering https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2236858410.
(https://github.com/bitcoin/bitcoin/pull/33005#issuecomment-3128251488)
Most recent push updates the `m_tx_inventory_to_send` comment and adds a clarifying comment as to why we create the inv before checking `m_tx_inventory_known_filter`.
I'm still considering https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2236858410.
💬 marcofleon commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2237365239)
Done.
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2237365239)
Done.
💬 marcofleon commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2237365568)
Good looks, fixed.
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2237365568)
Good looks, fixed.
🤔 l0rinc reviewed a pull request: "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store"
(https://github.com/bitcoin/bitcoin/pull/32427#pullrequestreview-2822815289)
Concept ACK, not yet sure about the approach
### LevelDB migration
Moving away from unmaintained LevelDB makes sense and aligns with other modularization and optimization efforts.
Whether that means switching to SQLite or a custom solution like this one is debatable, but removing LevelDB already paves the way for further migrations - which still seem somewhat taboo at this point.
Before merging something like this, I'd be interested in seeing a full migration story (including other ind
...
(https://github.com/bitcoin/bitcoin/pull/32427#pullrequestreview-2822815289)
Concept ACK, not yet sure about the approach
### LevelDB migration
Moving away from unmaintained LevelDB makes sense and aligns with other modularization and optimization efforts.
Whether that means switching to SQLite or a custom solution like this one is debatable, but removing LevelDB already paves the way for further migrations - which still seem somewhat taboo at this point.
Before merging something like this, I'd be interested in seeing a full migration story (including other ind
...