💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237012262)
We did try this originally, but (very annoyingly) it does not seem to be possible without a primary workflow to call this workflow. The reason is that the `runs-on` field only has access to the following "contexts":
```
github, needs, strategy, matrix, vars, inputs
```
See https://docs.github.com/en/actions/reference/workflows-and-actions/contexts
Whilst `vars` sounds like it would do the job, the problem is that in workflows of the type `on: pull_request` that `vars` (which refer her
...
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237012262)
We did try this originally, but (very annoyingly) it does not seem to be possible without a primary workflow to call this workflow. The reason is that the `runs-on` field only has access to the following "contexts":
```
github, needs, strategy, matrix, vars, inputs
```
See https://docs.github.com/en/actions/reference/workflows-and-actions/contexts
Whilst `vars` sounds like it would do the job, the problem is that in workflows of the type `on: pull_request` that `vars` (which refer her
...
💬 murchandamus commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3127923208)
@RobinLinus, are you planning on working on this?
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3127923208)
@RobinLinus, are you planning on working on this?
💬 theStack commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3127965996)
Concept ACK
Makes sense to keep the maxorphantx option for a while to only show a warning. Changes look good at first glance, will do a deeper dive with focus on test and fuzzing changes (considering https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3120296633) during the week.
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3127965996)
Concept ACK
Makes sense to keep the maxorphantx option for a while to only show a warning. Changes look good at first glance, will do a deeper dive with focus on test and fuzzing changes (considering https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3120296633) during the week.
📝 fanquake opened a pull request: "[NO MERGE]: TSAN should fail"
(https://github.com/bitcoin/bitcoin/pull/33081)
Ref: https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3114706601.
(https://github.com/bitcoin/bitcoin/pull/33081)
Ref: https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3114706601.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3128011661)
Updated 1ffc1c9d94b16cdbfb92a26d0f0e75451efad4fe -> 938767d957b7669accfb554a7cbb25141f7e8632 ([kernelApi_45](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_45) -> [kernelApi_46](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_46), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_45..kernelApi_46))
* Fixed symbol visibility for windows static builds and simplified the macro checks in the header a bit.
* Removed the kernel library symbol visibility hack, this
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3128011661)
Updated 1ffc1c9d94b16cdbfb92a26d0f0e75451efad4fe -> 938767d957b7669accfb554a7cbb25141f7e8632 ([kernelApi_45](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_45) -> [kernelApi_46](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_46), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_45..kernelApi_46))
* Fixed symbol visibility for windows static builds and simplified the macro checks in the header a bit.
* Removed the kernel library symbol visibility hack, this
...
🤔 stickies-v reviewed a pull request: "kernel: Introduce initial C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3030139760)
Did another review round while updating py-bitcoinkernel. The main themes are:
- using reference counting internally to let us simplify the interface (e.g. expose `kernel_TransactionUndo` and `kernel_Coin` instead of letting the user manage indexes) as well as replace expensive `_copy` operations with `_get` ones
- note: in the WG we have discussed exposing reference counting through the public interface too, but personally I don't see the benefits for that complexity yet, even if I'm happy
...
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3030139760)
Did another review round while updating py-bitcoinkernel. The main themes are:
- using reference counting internally to let us simplify the interface (e.g. expose `kernel_TransactionUndo` and `kernel_Coin` instead of letting the user manage indexes) as well as replace expensive `_copy` operations with `_get` ones
- note: in the WG we have discussed exposing reference counting through the public interface too, but personally I don't see the benefits for that complexity yet, even if I'm happy
...
💬 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
...