script/solver.{h,cpp} from kernel headers
#28434
This removes the non consensus-critical solver.h header from the kernel headers by stripping out the TxoutType enum (needed by policy.h) into its own header.
It’s only a small gain but it’s also pretty straightforward. Figured i’d PR it directly to gather whether people think that’s worth it.
They were indirectly relying on policy/policy.h including solver.h.
solver.h is included by policy.h only to get access to the TxoutType
enum. Separate it in its own module to let us strip solver.h from
the kernel headers.
It was indirectly included by policy.h, which only needs access to
TxoutType.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
ACK 84024b44e25507f5ef10e03057510383c47af332
Thank for your contribution!
I don’t think we have to hunt down all the non-consensus code in the kernel headers at this time though. As long as it is still unclear what the API is supposed to be in the end, the focus should be on removing platform-specific or external dependency headers. For easy wins such as here, the benefit is clear though: Less unrelated code in the kernel lib means one fewer thing to handle in the future of the kernel library.
9@@ -10,32 +10,15 @@
10
11 #include <attributes.h>
12 #include <script/script.h>
13+#include <script/txouttype.h>
solver.o is still linked into libbitcoinkernel_la
Re #28434#pullrequestreview-1627392653
I don’t understand this change. solver.o is still linked into libbitcoinkernel_la
Yeah, completely read over the part that this should also be removing the solver object from the kernel, which this PR is not doing. The description should be updated.
As for removing solver.h from the public headers: I think that’s a bit too early with no linter in place to check it, no?
We don’t have a linter in place for the other headers that were recently removed either though. @cfields has a commit for manually defining the list of includes for bitcoin-chainstate. But like I said, I don’t think we should be looking for more of these right now, but it doesn’t hurt to split this up either.
We don’t have a linter in place for the other headers that were recently removed either though.
At least for those it was pretty clear that they shouldn’t be part of the public headers. However, here I don’t see it. What if a user wants to use the solver and it has to be re-added to libbitcoinkernel later? Seems odd to rule that out now, no?
What if a user wants to use the solver and it has to be re-added to libbitcoinkernel later? Seems odd to rule that out now, no?
There is nothing here that precludes the solver not becoming part of the API later on.