🤔 stickies-v reviewed a pull request: "kernel: Introduce initial C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-2297633388)
Strong concept ACK.
I've started building a python wrapper library to get familiar with and actually use the interface, so most of my comments for now will be based on that experience and reading the documentation.
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-2297633388)
Strong concept ACK.
I've started building a python wrapper library to get familiar with and actually use the interface, so most of my comments for now will be based on that experience and reading the documentation.
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1757015877)
nit: I like that we're using this guard. Do you see a downside to making it variadic?
(Should be a pretty trivial rebase with e.g. `for i in {1..3}; do sed -i -E "s/BITCOINKERNEL_ARG_NONNULL\(([^)]+)\) BITCOINKERNEL_ARG_NONNULL\(([0-9]+)\)/BITCOINKERNEL_ARG_NONNULL(\1, \2)/" ./src/kernel/bitcoinkernel.h; done`)
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1757015877)
nit: I like that we're using this guard. Do you see a downside to making it variadic?
(Should be a pretty trivial rebase with e.g. `for i in {1..3}; do sed -i -E "s/BITCOINKERNEL_ARG_NONNULL\(([^)]+)\) BITCOINKERNEL_ARG_NONNULL\(([0-9]+)\)/BITCOINKERNEL_ARG_NONNULL(\1, \2)/" ./src/kernel/bitcoinkernel.h; done`)
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1757065221)
Is there benefit to this stand-alone Context documentation, since we already have (and could expand on/merge with) the `kernel_Context` documentation? I think perhaps a more useful alternative would be to start the documentation with a minimal example on how to use the kernel (or a non-code "getting started" guide), which would inevitably include/reference the `kernel_Context`, providing users a good starting point on which documentation to read first?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1757065221)
Is there benefit to this stand-alone Context documentation, since we already have (and could expand on/merge with) the `kernel_Context` documentation? I think perhaps a more useful alternative would be to start the documentation with a minimal example on how to use the kernel (or a non-code "getting started" guide), which would inevitably include/reference the `kernel_Context`, providing users a good starting point on which documentation to read first?
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1848821020)
What's the point of the `context` parameter - it seems unused, and inconsistent with the other `_destroy` functions?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1848821020)
What's the point of the `context` parameter - it seems unused, and inconsistent with the other `_destroy` functions?
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1757105125)
I find this phrasing a bit confusing. Is this a correct replacement?
```suggestion
* A function that takes pointer arguments makes no assumptions on their lifetime. Once the function
* returns the user can safely de-allocate the memory owned by those pointers.
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1757105125)
I find this phrasing a bit confusing. Is this a correct replacement?
```suggestion
* A function that takes pointer arguments makes no assumptions on their lifetime. Once the function
* returns the user can safely de-allocate the memory owned by those pointers.
```
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1757108471)
1) What's "it"?
2) I think adopting and sticking to a clear definition of MUST, MAY, SHOULD, ... would be appropriate here? E.g. in this case, I think they "MUST" not be de-allocated by the user, rather than "SHOULD"?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1757108471)
1) What's "it"?
2) I think adopting and sticking to a clear definition of MUST, MAY, SHOULD, ... would be appropriate here? E.g. in this case, I think they "MUST" not be de-allocated by the user, rather than "SHOULD"?
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1848822043)
nit: this is the only place where `context` is not the first option, would be nice for consistency?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1848822043)
nit: this is the only place where `context` is not the first option, would be nice for consistency?
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1757049107)
Is there any benefit to documenting the built-in static constant kernel context in the header documentation? If I understand correctly, that's an implementation detail and not relevant to the user? If so, I think we should
- only talk about the non-static context in `bitcoinkernel.h`, so that its meaning is unambiguous to the user
- consistently refer to the static context as "static context" wherever it is documented, as to not make me question everything whenever I come across an unqualified
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1757049107)
Is there any benefit to documenting the built-in static constant kernel context in the header documentation? If I understand correctly, that's an implementation detail and not relevant to the user? If so, I think we should
- only talk about the non-static context in `bitcoinkernel.h`, so that its meaning is unambiguous to the user
- consistently refer to the static context as "static context" wherever it is documented, as to not make me question everything whenever I come across an unqualified
...
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1848935049)
There are a few places, like here, where we expose modifier functions that are (quasi) required to be ran before initializing another object. An alternative approach would be to extend the `kernel_context_options_create` to take a (nullable) `kernel_ChainParameters*`, and remove these ~unsafe modifiers altogether? I think that would have the benefit of:
- removing a whole category of bugs where users set options at the wrong time (i.e. too late), silently leading to buggy behaviour
- making it
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1848935049)
There are a few places, like here, where we expose modifier functions that are (quasi) required to be ran before initializing another object. An alternative approach would be to extend the `kernel_context_options_create` to take a (nullable) `kernel_ChainParameters*`, and remove these ~unsafe modifiers altogether? I think that would have the benefit of:
- removing a whole category of bugs where users set options at the wrong time (i.e. too late), silently leading to buggy behaviour
- making it
...
💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2486612510)
Rebased to avoid spurious CI failure.
I'll take a look later if for any of the tests I can drop their custom coinbase output.
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2486612510)
Rebased to avoid spurious CI failure.
I'll take a look later if for any of the tests I can drop their custom coinbase output.
💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1848986804)
I expanded the comment to explain things a bit more.
I think you should not set this option unless you care what the coinbase output is. For example the `block_assemble` benchmark uses `P2WSH_OP_TRUE`. I'm guessing that's because it wants to look at the performance related to spending from a segwit coinbase.
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1848986804)
I expanded the comment to explain things a bit more.
I think you should not set this option unless you care what the coinbase output is. For example the `block_assemble` benchmark uses `P2WSH_OP_TRUE`. I'm guessing that's because it wants to look at the performance related to spending from a segwit coinbase.
💬 jonatack commented on pull request "test: addrman: tried 3 times and never a success so `isTerrible=true`":
(https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1849010201)
Oh indeed (thanks!) I think the following would be clearer.
```diff
@@ -453,12 +453,13 @@ BOOST_AUTO_TEST_CASE(getaddr_unfiltered)
addr3.nTime = Now<NodeSeconds>();
addrman->Good(addr3, /*time=*/Now<NodeSeconds>());
BOOST_CHECK(addrman->Add({addr3}, source));
- // The time is set, but after 3 unsuccessful attempts this addr should be isTerrible = true
+ // The time is set, but after ADDRMAN_RETRIES unsuccessful attempts not
+ // retried in the last minute, this
...
(https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1849010201)
Oh indeed (thanks!) I think the following would be clearer.
```diff
@@ -453,12 +453,13 @@ BOOST_AUTO_TEST_CASE(getaddr_unfiltered)
addr3.nTime = Now<NodeSeconds>();
addrman->Good(addr3, /*time=*/Now<NodeSeconds>());
BOOST_CHECK(addrman->Add({addr3}, source));
- // The time is set, but after 3 unsuccessful attempts this addr should be isTerrible = true
+ // The time is set, but after ADDRMAN_RETRIES unsuccessful attempts not
+ // retried in the last minute, this
...
💬 brunoerg commented on pull request "test: addrman: tried 3 times and never a success so `isTerrible=true`":
(https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1849026393)
Done, thanks.
(https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1849026393)
Done, thanks.
💬 brunoerg commented on pull request "test: addrman: tried 3 times and never a success so `isTerrible=true`":
(https://github.com/bitcoin/bitcoin/pull/30445#issuecomment-2486702866)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1849010201
(https://github.com/bitcoin/bitcoin/pull/30445#issuecomment-2486702866)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1849010201
💬 jonatack commented on pull request "test: addrman: tried 3 times and never a success so `isTerrible=true`":
(https://github.com/bitcoin/bitcoin/pull/30445#issuecomment-2486751478)
re-ACK 1807df3d9fb0135057a33e01173080ea14b0403d
(https://github.com/bitcoin/bitcoin/pull/30445#issuecomment-2486751478)
re-ACK 1807df3d9fb0135057a33e01173080ea14b0403d
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2486794714)
Thank you for the review @stickies-v!
Updated 6c9121f7907262b2bf065a7ceeb8bca620060a7f -> 6c9121f7907262b2bf065a7ceeb8bca620060a7f ([kernelApi_0](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_0) -> [kernelApi_1](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_0..kernelApi_1))
* Added, cleaned up, and precised a bunch of documentation
* Slightly changed the order of a function's arguments, such that it
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2486794714)
Thank you for the review @stickies-v!
Updated 6c9121f7907262b2bf065a7ceeb8bca620060a7f -> 6c9121f7907262b2bf065a7ceeb8bca620060a7f ([kernelApi_0](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_0) -> [kernelApi_1](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_0..kernelApi_1))
* Added, cleaned up, and precised a bunch of documentation
* Slightly changed the order of a function's arguments, such that it
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849075583)
Good point, I added your suggestion.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849075583)
Good point, I added your suggestion.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849071009)
When I apply this to the first commit I get:
```
In file included from /home/drgrid/bitcoin/src/test/kernel/test_kernel.cpp:5:
/home/drgrid/bitcoin/src/kernel/bitcoinkernel.h:201:32: error: too many arguments provided to function-like macro invocation
201 | ) BITCOINKERNEL_ARG_NONNULL(1, 3);
| ^
/home/drgrid/bitcoin/src/kernel/bitcoinkernel.h:34:9: note: macro 'BITCOINKERNEL_ARG_NONNULL' defined here
34 | #define BITCOINKERNEL_ARG_NONNULL(_x) __at
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849071009)
When I apply this to the first commit I get:
```
In file included from /home/drgrid/bitcoin/src/test/kernel/test_kernel.cpp:5:
/home/drgrid/bitcoin/src/kernel/bitcoinkernel.h:201:32: error: too many arguments provided to function-like macro invocation
201 | ) BITCOINKERNEL_ARG_NONNULL(1, 3);
| ^
/home/drgrid/bitcoin/src/kernel/bitcoinkernel.h:34:9: note: macro 'BITCOINKERNEL_ARG_NONNULL' defined here
34 | #define BITCOINKERNEL_ARG_NONNULL(_x) __at
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849072214)
Thanks, taken.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849072214)
Thanks, taken.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849046061)
I did not think that much about order here, but I do think having this section on the context is a good idea. The key is that the user is not required to instantiate the context for using some parts of the library (and I think this is important enough to not just make it a footnote). The user-instantiated context is only really required when interacting with the "stateful" endpoints. Besides, it may be relevant to know what the library is instantiating internally in case there is some sort of co
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849046061)
I did not think that much about order here, but I do think having this section on the context is a good idea. The key is that the user is not required to instantiate the context for using some parts of the library (and I think this is important enough to not just make it a footnote). The user-instantiated context is only really required when interacting with the "stateful" endpoints. Besides, it may be relevant to know what the library is instantiating internally in case there is some sort of co
...