kernel: Introduce C header API - #30595
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30595. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_5_m |
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
|
Very cool. Can't wait to dig in when I have some free time. |
|
This seems to offer a lot of nice features, but can you explain the tradeoffs of wrapping the C++ interface in C instead of using C++ from rust directly? It seems like having a C middle layer introduces a lot of boilerplate, and I'm wondering if it is really necessary. For example it seems like there is a rust cxx crate (https://docs.rs/cxx/latest/cxx/, https://chatgpt.com/share/dd4dde59-66d6-4486-88a6-2f42144be056) that lets you call C++ directly from Rust and avoid the need for C boilerplate. It looks like https://cppyy.readthedocs.io/en/latest/index.html is an even more full-featured way of calling c++ from python. Another drawback of going through a C API seems like not just increased boilerplate, but reduced safety. For example, the implementation is using reinterpret_cast everywhere and it seems like the exposed C functions use a |
|
Thank you for the questions and kicking this discussion off @ryanofsky! I'll update the PR description with a better motiviation re. C vs C++ header, but will also try to answer your questions here.
It is true that the interoperability between C++ and Rust has become very good. In fact there is someone working on wrapping the entirety of Bitcoin Core in Rust: https://github.com/klebs6/bitcoin-rs. During the last Core Dev meeting in Berlin I also asked if a C API were desirable in the first place (notes here) during the libbitcoinkernel session. I moved forward with this implementation, because the consensus at the time with many contributors in the room was that it was desirable. The reasons for this as discussed during the session at the meeting can be briefly summarised:
So if we want the broadest possible support, across as many languages as possible with both dynamic and statically compiled libraries, a C header is the go-to option. I'm speculating here, but a C++ header might also make future standard version bumps and adoption of new standard library features harder. If having some trade-offs with compatibility, library portability, and language support is acceptable, a C++ header might be acceptaple though. It would be nice to hear more reviewers give their opinions here. I'd also like to add that two libraries that we use and depend on in this project, minisketch and zeromq, use the same pattern. They are C++ codebases, that only expose a C API that in both instances can be used with a C++ RAII wrapper. So there is precedent in the free software ecosystem for doing things this way. The quality of C++ language interop seems to vary a lot between languages. Python and Rust seem to have decent support, ziglang on the other hand has no support for C++ bindings. JVM family languages are a bit hit and miss, and many of the common academic and industrial data analysis languages, like Julia, R, and Matlab have no support for direct C++ bindings. The latter category should not be disregarded as potential future users, since this library might be useful to access Bitcoin Core data for data analysis projects.
I feel like the reduced type safety due to casting is bit of a red herring. The type casting can be harder to abuse if you always use a dedicated helper function for interpreting passed in data types (as I believe is implemented here). Casting is also a pattern used in many other projects; both minisketch and libzmq use similar type casts extensively. It should definitely be possible to scrutinize the API in this PR to a point where it offers decent safety to its users as well as contributors to and maintainers of this code base. The concerns around boilerplate are more serious in my view, but at least with the current internal code and headers I feel like exposing a safe C++ API is not trivial either. The current headers do not lend themselves to it well, for example through tricky locking mechanics, exposing boost types, or confusing lifetimes. There also comes a point where we should probably stop extensively refactoring internal code for the kernel. I've heard some voices during the last two Core Dev meetings with concerns that the kernel project might turn the validation code into an extensive forever building site. Having some boilerplate and glue to abstract some the ugliness and make it safe seems like an acceptable solution for this dilemma. If this means boilerplate is required anyway, I would personally prefer a C API. Some of the boilerplate-y duplicate definitions in the header could be dropped again eventually if some of the It might be interesting to see how some of the RPC methods could be re-implemented using the kernel header. There have been some RPC implementation bugs over the years that were due to unsafe usage of our internal code within the method implementations. Using the kernel header instead might make this safer and reduce boilerplate. To be clear, I am not suggesting replacing the implementations, but separately re-implementing some of them to show where the kernel header might shine.
We have disagreed on the design of this before. If I understood you correctly, consolidating all error codes into a single enumeration was one of the reasons you opened your version for handling fatal errors in the kernel: #29700 as an alternative to my original: #29642. I am still a bit torn by the two approaches. I get that it may be useful to exactly see which errors may be encountered by invoking a certain routine, but at the same time I get the feeling this often ends up splintering the error handling to the point where you end up with a catch all approach after all. I also think that it is nice to have a single, central list for looking up all error codes and defining some routines for handling them in close proximity to their definition. It would be nice to finally hear some more voices besides the two of us discussing this. real-or-random has recently provided some good points on error handling in the libsecp silent payments pr (that I mostly did not adopt in this PR) and argues that most error codes are not useful to the user. As mentioned in the description, error handling is a weak spot of this pull request and I would like to improve it. |
|
I guess another thing I'd like to know is if this is the initial C API, and the implementation is around 3000 lines, and it doesn't handle "transactions, block headers, coins cache, utxo set, meta data, and the mempool", how much bigger do you think it will get if it does cover most of the things you would like it to cover? Like is this 20%, 30%, or 50% of the expected size? I like the idea of reviewing and merging this PR, and establishing a way to interoperate with rust libraries and external projects. I just think going forward we should not lock ourselves into an approach that requires everything to go through a C interface. As we build on this and add features, we should experiment with other approaches that use C++ directly, especially when it can reduce boilerplate and avoid bugs. Thanks for pointing to me to the other error handling discussion. I very much agree with the post that says having a single error handling path is highly desirable. I especially agree with this in cases where detailed error messages are still provided (keeping in mind that error handling != error reporting, you can return simple error states with detailed messages or logging). Of course there are places where callers do need to handle separate error cases, especially when there are temporary failures, timeouts, and interruptions, and in these cases functions should return 2 or 3 error states instead of 1. But I don't think there is a reason in modern application code for functions to be able to return 5, 10, 20, or 50 error states generally. In low-level or very general OS, networking or DBMS code it might make sense, but for application code it seems like a cargo cult programming practice that made IBM service manuals very impressive in the 1980s but does not have a present day rationale. There are special cases, but I don't think it should be a normal thing for functions to be returning 15 error codes if we are trying to provide a safe and easy to use API. Again though, if this approach is the easiest way to get cross-language interoperability working right now, I think we should try it. I just think we should be looking for ways to make things simpler and safer going forward. |
I think a fair comparison would be comparing the amount of code "glue" required, e.g. the size of the
Heh, well put. I think for most functions here it could be feasible to have more concise error codes without too much effort, but I feel like I have to detach from this a bit before being able to come up with an alternative. |
Thanks, I think I'd need to look at this more to give concrete suggestions, but I'd hope most functions would just return a simple success or failure status, with a descriptive error message in the case of failure. When functions need to return more complicated information or can fail in different ways that callers will want to distinguish, it should be easy to return the relevant information in custom struct or enum types. I think it's usually better for functions to return simpler custom types than more complicated shared types, because it lets callers know what values functions can return just by looking at their declarations. |
cdec740 to
1932d10
Compare
de298e6 to
8345e0e
Compare
Completely got rid of the |
|
Thanks for the update. It's good to drop the error codes so the C API can correspond 1:1 with the C++ API and not be tied to a more old fashioned and cumbersome error handling paradigm (for callers that want to know which errors are possible and not have to code defensively or fall back to failing generically). I am still -0 on the approach of introducing a C API to begin with, but happy to help review this and get merged and maintain it if other developers think this is the right approach to take (short term or long term). It would be great to have more concept and approach ACKs for this PR particularly from the @theuni who commented earlier and @josibake who seems to have some projects built on this and linked in the PR description. I think personally, if I wanted to use bitcoin core code from python or rust I would use tools like:
And interoperate with C++ directly, instead of wrapping the C++ interface in a C interface first. Tools like these do not support all C++ types and features, and can make it necessary to selectively wrap more complicated C++ interfaces with simpler C++ interfaces, or even C interfaces, but I don't think this would be a justification for preemptively requiring every C++ type and function to be wrapped in C before it can be exposed. I just think the resulting boilerplate code: kernel_Warning cast_kernel_warning(kernel::Warning warning)
{
switch (warning) {
case kernel::Warning::UNKNOWN_NEW_RULES_ACTIVATED:
return kernel_Warning::kernel_LARGE_WORK_INVALID_CHAIN;
case kernel::Warning::LARGE_WORK_INVALID_CHAIN:
return kernel_Warning::kernel_LARGE_WORK_INVALID_CHAIN;
} // no default case, so the compiler can warn about missing cases
assert(false);
}and duplicative type definitions and documentation: /**
* A struct for holding the kernel notification callbacks. The user data pointer
* may be used to point to user-defined structures to make processing the
* notifications easier.
*/
typedef struct {
void* user_data; //!< Holds a user-defined opaque structure that is passed to the notification callbacks.
kernel_NotifyBlockTip block_tip; //!< The chain's tip was updated to the provided block index.
kernel_NotifyHeaderTip header_tip; //!< A new best block header was added.
kernel_NotifyProgress progress; //!< Reports on current block synchronization progress.
kernel_NotifyWarningSet warning_set; //!< A warning issued by the kernel library during validation.
kernel_NotifyWarningUnset warning_unset; //!< A previous condition leading to the issuance of a warning is no longer given.
kernel_NotifyFlushError flush_error; //!< An error encountered when flushing data to disk.
kernel_NotifyFatalError fatal_error; //!< A un-recoverable system error encountered by the library.
} kernel_NotificationInterfaceCallbacks;are fundamentally unnecessary and not worth effort of writing and maintaining when C++ is not a new or unusual language and not meaningfully less accessible or interoperable than C is. There are legitimate reasons to wrap C++ in C. One reason would be to provide ABI compatibility. Another would be to make code accessible with dlopen/dlsym. But I think even in these cases you would want to wrap C++ in C selectively, or just define an intermediate C interface to pass pointers but use C++ on either side of the interface. I don't think you would want to drop down to C when not otherwise needed. This is just to explain my point of view though. Overall I think this is very nice work, and I want to help with it, not hold it up. |
|
Another idea worth mentioning is that a bitcoin kernel C API could be implemented as a separate C library depending on the C++ library. The new code here does not necessarily need to be part of the main bitcoin core git repository, and it could be in a separate project. A benefit of this approach is it could relieve bitcoin core developers from the responsibility of updating the C API and API documention when they change the C++ code. But a drawback is that C API might not always be up to date with latest version of bitcoin core code and could be broken between releases. Also it might not be as well reviewed or understood and might have more bugs. |
|
Thank you for the review @yuvicc, e95efc0 -> 6c7a34f (kernelApi_80 -> kernelApi_81, compare) |
|
re-ACK 6c7a34f |
| btck_NotifyWarningSet warning_set; //!< A warning issued by the kernel library during validation. | ||
| btck_NotifyWarningUnset warning_unset; //!< A previous condition leading to the issuance of a warning is no longer given. | ||
| btck_NotifyFlushError flush_error; //!< An error encountered when flushing data to disk. | ||
| btck_NotifyFatalError fatal_error; //!< A un-recoverable system error encountered by the library. |
There was a problem hiding this comment.
llm-nit: [Only if you re-touch]:
- A un-recoverable system error encountered by the library. -> An unrecoverable system error encountered by the library. [“A” before a vowel sound should be “An”; “un-recoverable” is more commonly written “unrecoverable” — the revised phrase is grammatical and clearer.]
|
re-ACK 6c7a34f nits if/when you have to retouchdiff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index 8bba3cf1c0..07b59d9543 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -602,11 +602,11 @@ void btck_transaction_output_destroy(btck_TransactionOutput* output)
}
int btck_script_pubkey_verify(const btck_ScriptPubkey* script_pubkey,
- const int64_t amount,
+ int64_t amount,
const btck_Transaction* tx_to,
const btck_TransactionOutput** spent_outputs_, size_t spent_outputs_len,
- const unsigned int input_index,
- const btck_ScriptVerificationFlags flags,
+ unsigned int input_index,
+ btck_ScriptVerificationFlags flags,
btck_ScriptVerifyStatus* status)
{
// Assert that all specified flags are part of the interface before continuing
@@ -1236,7 +1236,7 @@ const btck_BlockTreeEntry* btck_chain_get_tip(const btck_Chain* chain)
return btck_BlockTreeEntry::ref(btck_Chain::get(chain).Tip());
}
-int btck_chain_get_height(const btck_Chain* chain)
+int32_t btck_chain_get_height(const btck_Chain* chain)
{
LOCK(::cs_main);
return btck_Chain::get(chain).Height();
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index 99ae2bd67a..afe7f38781 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -608,7 +608,7 @@ BITCOINKERNEL_API int BITCOINKERNEL_WARN_UNUSED_RESULT btck_script_pubkey_verify
const btck_Transaction* tx_to,
const btck_TransactionOutput** spent_outputs, size_t spent_outputs_len,
unsigned int input_index,
- unsigned int flags,
+ btck_ScriptVerificationFlags flags,
btck_ScriptVerifyStatus* status) BITCOINKERNEL_ARG_NONNULL(1, 3);
/** |
|
Code review ACK 6c7a34f i think this is a good start for the C API, and ready for merge. |
|
reACK 6c7a34f 👾 There have been numerous changes to the header API since my last ACK. Changes
|
|
ACK 6c7a34f - soon we'll be running bitcoin (kernel) |
|
post-merge ACK 6c7a34f |
|
What a milestone. Congrats @TheCharlatan! |
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
lgtm. Just left a few nits and questions. I am happy to address them myself, if applicable
|
|
||
| LoggingConnection(btck_LogCallback callback, void* user_data, btck_DestroyCallback user_data_destroy_callback) | ||
| { | ||
| LOCK(cs_main); |
There was a problem hiding this comment.
nit in 28d679b: Could add a comment explaining why cs_main is used here? Maybe a new logging-specific global mutex could be added here?
There was a problem hiding this comment.
Ideally this all goes away soon with a kernel/common logging split. Didn't feel like introducing a new global in the interim, since this is typically only done once.
There was a problem hiding this comment.
Ah, nice. Looks like it will go away with https://github.com/bitcoin/bitcoin/pull/34374/files
| ":(exclude)src/ipc/libmultiprocess/", | ||
| ":(exclude)src/util/fs.h", | ||
| ":(exclude)src/test/kernel/test_kernel.cpp", | ||
| ":(exclude)src/bitcoin-chainstate.cpp", |
There was a problem hiding this comment.
Seems fine for now, but it would be nice to check that non-ascii datadirs work on Windows. Though, I can't help here and in CI, the following diff seems to fail:
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index f31a9a0277..0e151c12c0 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -4,6 +4,7 @@
#include <kernel/bitcoinkernel.h>
#include <kernel/bitcoinkernel_wrapper.h>
+#include <util/fs.h>
#define BOOST_TEST_MODULE Bitcoin Kernel Test Suite
#include <boost/test/included/unit_test.hpp>
@@ -100,7 +101,7 @@ public:
struct TestDirectory {
std::filesystem::path m_directory;
TestDirectory(std::string directory_name)
- : m_directory{std::filesystem::temp_directory_path() / (directory_name + random_string(16))}
+ : m_directory{std::filesystem::temp_directory_path() / fs::PathFromString(" 🔥").std_path() / (directory_name + random_string(16))}
{
std::filesystem::create_directories(m_directory);
}There was a problem hiding this comment.
Yes, and as you lay out, the real solution here would be including our existing filesystem and test utilities in lieu of std::filesystem. I tried doing that, but it would not work. I tried using strace to see whether the paths with unicode symbols get passed through the C API correctly, which indicated that they do. I wonder if the problem is in fsbridge's use of fopen when compiling for windows? Might this be a regression from 53e4951 ?
There was a problem hiding this comment.
Is there a tracking issue for this? I guess it is bitcoin_kernel_util from #28690?
There was a problem hiding this comment.
No, but I was preparing a patch for this, I'll pick it back up soon.
sedited
left a comment
sedited
left a comment
There was a problem hiding this comment.
Thank you so much for taking another look @maflcko! I think these all deserve a follow up. I would suggest tackling the unicode path issues in a separate PR.
I've been exploring ways the past week to integrate these tighter into our existing test framework, while still linking as a shared library and exercising the external kernel headers. The main problem with that seems to be linking in a re-definition of the translation function.
|
|
||
| LoggingConnection(btck_LogCallback callback, void* user_data, btck_DestroyCallback user_data_destroy_callback) | ||
| { | ||
| LOCK(cs_main); |
There was a problem hiding this comment.
Ideally this all goes away soon with a kernel/common logging split. Didn't feel like introducing a new global in the interim, since this is typically only done once.
| ":(exclude)src/ipc/libmultiprocess/", | ||
| ":(exclude)src/util/fs.h", | ||
| ":(exclude)src/test/kernel/test_kernel.cpp", | ||
| ":(exclude)src/bitcoin-chainstate.cpp", |
There was a problem hiding this comment.
Yes, and as you lay out, the real solution here would be including our existing filesystem and test utilities in lieu of std::filesystem. I tried doing that, but it would not work. I tried using strace to see whether the paths with unicode symbols get passed through the C API correctly, which indicated that they do. I wonder if the problem is in fsbridge's use of fopen when compiling for windows? Might this be a regression from 53e4951 ?
|
@maflcko do you mind me taking charge of implementing your suggestions, or do you already have the patches prepared? |
20 participants
This is a first attempt at introducing a C header for the libbitcoinkernel library that may be used by external applications for interfacing with Bitcoin Core's validation logic. It currently is limited to operations on blocks. This is a conscious choice, since it already offers a lot of powerful functionality, but sits just on the cusp of still being reviewable scope-wise while giving some pointers on how the rest of the API could look like.
The current design was informed by the development of some tools using the C header:
The library has also been used by other developers already:
Next to the C++ header also made available in this pull request, bindings for other languages are available here:
The rust bindings include unit and fuzz tests for the API.
The header currently exposes logic for enabling the following functionality:
The pull request introduces a new kernel-only test binary that purely relies on the kernel C header and the C++ standard library. This is intentionally done to show its capabilities without relying on other code inside the project. This may be relaxed to include some of the existing utilities, or even be merged into the existing test suite.
The complete docs for the API as well as some usage examples are hosted on thecharlatan.ch/kernel-docs. The docs are generated from the following repository (which also holds the examples): github.com/TheCharlatan/kernel-docs.
How can I review this PR?
Scrutinize the commit messages, run the tests, write your own little applications using the library, let your favorite code sanitizer loose on it, hook it up to your fuzzing infrastructure, profile the difference between the existing bitcoin-chainstate and the bitcoin-chainstate introduced here, be nitty on the documentation, police the C interface, opine on your own API design philosophy.
To get a feeling for the API, read through the tests, or one of the examples.
To configure this PR for making the shared library and the bitcoin-chainstate and test_kernel utilities available:
Once compiled the library is part of the build artifacts that can be installed with:
Why a C header (and not a C++ header)
Also see #30595 (comment).
What about versioning?
The header and library are still experimental and I would expect this to remain so for some time, so best not to worry about versioning yet.
Potential future additions
In future, the C header could be expanded to support (some of these have been roughly implemented):
Current drawbacks