💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1848870979)
I thought I did, but give what the documentation says, it indeed shouldn't be needed. Will test again.

📝 Sjors converted_to_draft a pull request: "mining: add early return to waitTipChanged()"
(https://github.com/bitcoin/bitcoin/pull/31297)
It's useful to be able to wait for an active chaintip to appear, by calling ` waitTipChanged(uint256::ZERO);`.

Unfortunately this doesn't work out of the box, because `notifications().m_tip_block` is `ZERO` until a new block arrives.

Additionally we need an early return before calling `wait_for`.

👍 ryanofsky approved a pull request: "Drop script_pub_key arg from createNewBlock"
(https://github.com/bitcoin/bitcoin/pull/31318#pullrequestreview-2446313068)
Concept ACK . It seems like a nice improvement to not require specifying an option that won't typically be used. But I'm a little hazy on the details of this. I think better documentation could help. Left a suggestion and some questions below.

💬 ryanofsky commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1848878221)
re: https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1846813317

Would it make sense to declare the field as `CScript coinbase_output_script{CScript{} << OP_TRUE}` instead of `std::optional<CScript> coinbase_output_script{};` just to be clearer about what the default value is? Or maybe just add documentation to say what happens if this is not set?

I think the drawback of using std::optional here is that it's not really clear what happens if the option is not set. It's also not clea
...

🚀 achow101 merged a pull request: "test: Introduce ensure_for helper"
(https://github.com/bitcoin/bitcoin/pull/30893)

📝 Sjors opened a pull request: "kernel: make m_tip_block std::optional"
(https://github.com/bitcoin/bitcoin/pull/31325)
Suggested in https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1844244309

💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2486525961)
It's a bit tricky at the moment to test the interaction of multiple pull requests on my stratum v2 branches. I'll test this again later, and most likely close the PR then.

Meanwhile I extracted the make `m_tip_block std::optional` commit into #31325 for separate consideration.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2486536906)
#31297 is on hold, as it's probably unnecessary.

Please review #31325 first, so I can decide whether to build this PR on top of it.

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1754989437)
nit / meta discussion: even though it'll make things more verbose, I think it might be worth referring to flags with their full name to make it easier for users to find them? I.e. "If the witness flag is set" would become "if kernel_SCRIPT_FLAGS_VERIFY_WITNESS is set in `flags`".

🤔 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.

💬 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`)

💬 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?

💬 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?

💬 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.
```

💬 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"?

💬 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?

💬 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
...

💬 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
...

💬 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.

💬 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.