💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2214073393)
clang-format sorts this alphabetically, but `windows.h` needs to be included before `shellapi.h` otherwise we get [build errors](https://github.com/stickies-v/bitcoin/actions/runs/16352515776/job/46202676758#step:10:953) like:
```
C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\um\shellapi.h(68,1): error C2146: syntax error: missing ';' before identifier 'DECLSPEC_IMPORT' [D:\a\bitcoin\bitcoin\build\src\bitcoin-chainstate.vcxproj]
(compiling source file '../../src/bitcoin-chai
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2214073393)
clang-format sorts this alphabetically, but `windows.h` needs to be included before `shellapi.h` otherwise we get [build errors](https://github.com/stickies-v/bitcoin/actions/runs/16352515776/job/46202676758#step:10:953) like:
```
C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\um\shellapi.h(68,1): error C2146: syntax error: missing ';' before identifier 'DECLSPEC_IMPORT' [D:\a\bitcoin\bitcoin\build\src\bitcoin-chainstate.vcxproj]
(compiling source file '../../src/bitcoin-chai
...
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2213968769)
Should this be const? It's not moveable as-is. If intentional, brief docstring would be good?
```suggestion
std::unique_ptr<kernel_BlockUndo, Deleter> m_block_undo;
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2213968769)
Should this be const? It's not moveable as-is. If intentional, brief docstring would be good?
```suggestion
std::unique_ptr<kernel_BlockUndo, Deleter> m_block_undo;
```
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2213740015)
nit: couple of naming mismatches after latest force pushes:
<details>
<summary>git diff on 1ffc1c9d94</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index b72c001d1b..ec4db4e7c7 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -130,7 +130,7 @@ typedef struct kernel_TransactionOutput kernel_TransactionOutput;
*
* Messages that were logged before a connection is created are buffered in a
* 1MB buffer. Logging c
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2213740015)
nit: couple of naming mismatches after latest force pushes:
<details>
<summary>git diff on 1ffc1c9d94</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index b72c001d1b..ec4db4e7c7 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -130,7 +130,7 @@ typedef struct kernel_TransactionOutput kernel_TransactionOutput;
*
* Messages that were logged before a connection is created are buffered in a
* 1MB buffer. Logging c
...
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2216084200)
This looks like it leaks memory since we never deallocate `block`. Perhaps using `std::unique_ptr` here is a better approach? This (+`kernel_block_undo_read`) seems like the most dangerous one, but perhaps good practice to do this in other places we allocate memory, e.g. [here](https://github.com/bitcoin/bitcoin/pull/30595/files#diff-cc28221ef8d0c7294dda4e3df9f70bb6c062006b387468380c2c2cc02b6762c3R421).
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2216084200)
This looks like it leaks memory since we never deallocate `block`. Perhaps using `std::unique_ptr` here is a better approach? This (+`kernel_block_undo_read`) seems like the most dangerous one, but perhaps good practice to do this in other places we allocate memory, e.g. [here](https://github.com/bitcoin/bitcoin/pull/30595/files#diff-cc28221ef8d0c7294dda4e3df9f70bb6c062006b387468380c2c2cc02b6762c3R421).
💬 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
...