kernel: Introduce initial 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. |
|
🚧 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
1932d10 to
9246f36
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. |
src/kernel/bitcoinkernel.h
Outdated
| /** | ||
| * Function signatures for the kernel notifications. | ||
| */ | ||
| typedef void (*btck_NotifyBlockTip)(void* user_data, btck_SynchronizationState state, btck_BlockTreeEntry* entry, double verification_progress); |
There was a problem hiding this comment.
I think this should be a const btck_BlockTreeEntry* entry, so I've opened #33321 for the upstream change
| typedef void (*btck_NotifyBlockTip)(void* user_data, btck_SynchronizationState state, btck_BlockTreeEntry* entry, double verification_progress); | |
| typedef void (*btck_NotifyBlockTip)(void* user_data, btck_SynchronizationState state, const btck_BlockTreeEntry* entry, double verification_progress); |
| namespace { | ||
|
|
||
| template <typename C, typename CPP> | ||
| struct Handle { |
There was a problem hiding this comment.
I think ::create and ::copy methods could further improve the implementation, e.g.:
git diff on 82c5036
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index fc927ea3d7..51d914eaa1 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -87,6 +87,19 @@ struct Handle {
{
delete reinterpret_cast<CPP*>(ptr);
}
+
+ template <typename... Args>
+ static C* create(Args&&... args)
+ {
+ auto cpp_obj{std::make_unique<CPP>(std::forward<Args>(args)...)};
+ return reinterpret_cast<C*>(cpp_obj.release());
+ }
+
+ static C* copy(const C* ptr)
+ {
+ auto cpp_obj{std::make_unique<CPP>(get(ptr))};
+ return reinterpret_cast<C*>(cpp_obj.release());
+ }
};
} // namespace
@@ -284,7 +297,7 @@ protected:
{
if (m_cbs.block_checked) {
m_cbs.block_checked((void*)m_cbs.user_data,
- btck_Block::ref(new std::shared_ptr<const CBlock>{block}),
+ btck_Block::create(block),
btck_BlockValidationState::ref(&stateIn));
}
}
@@ -415,7 +428,7 @@ btck_Transaction* btck_transaction_create(const void* raw_transaction, size_t ra
{
try {
DataStream stream{std::span{reinterpret_cast<const std::byte*>(raw_transaction), raw_transaction_len}};
- return btck_Transaction::ref(new std::shared_ptr<const CTransaction>{new CTransaction{deserialize, TX_WITH_WITNESS, stream}});
+ return btck_Transaction::create(std::make_shared<CTransaction>(deserialize, TX_WITH_WITNESS, stream));
} catch (...) {
return nullptr;
}
@@ -439,8 +452,7 @@ size_t btck_transaction_count_inputs(const btck_Transaction* transaction)
btck_Transaction* btck_transaction_copy(const btck_Transaction* transaction)
{
- auto tx{new std::shared_ptr<const CTransaction>(btck_Transaction::get(transaction))};
- return btck_Transaction::ref(tx);
+ return btck_Transaction::copy(transaction);
}
int btck_transaction_to_bytes(const btck_Transaction* transaction, btck_WriteBytes writer, void* user_data)
@@ -464,7 +476,7 @@ void btck_transaction_destroy(btck_Transaction* transaction)
btck_ScriptPubkey* btck_script_pubkey_create(const void* script_pubkey, size_t script_pubkey_len)
{
auto data = std::span{reinterpret_cast<const uint8_t*>(script_pubkey), script_pubkey_len};
- return btck_ScriptPubkey::ref(new CScript(data.begin(), data.end()));
+ return btck_ScriptPubkey::create(data.begin(), data.end());
}
int btck_script_pubkey_to_bytes(const btck_ScriptPubkey* script_pubkey_, btck_WriteBytes writer, void* user_data)
@@ -475,7 +487,7 @@ int btck_script_pubkey_to_bytes(const btck_ScriptPubkey* script_pubkey_, btck_Wr
btck_ScriptPubkey* btck_script_pubkey_copy(const btck_ScriptPubkey* script_pubkey)
{
- return btck_ScriptPubkey::ref(new CScript(btck_ScriptPubkey::get(script_pubkey)));
+ return btck_ScriptPubkey::copy(script_pubkey);
}
void btck_script_pubkey_destroy(btck_ScriptPubkey* script_pubkey)
@@ -487,12 +499,12 @@ void btck_script_pubkey_destroy(btck_ScriptPubkey* script_pubkey)
btck_TransactionOutput* btck_transaction_output_create(const btck_ScriptPubkey* script_pubkey, int64_t amount)
{
const CAmount& value{amount};
- return btck_TransactionOutput::ref(new CTxOut(value, btck_ScriptPubkey::get(script_pubkey)));
+ return btck_TransactionOutput::create(value, btck_ScriptPubkey::get(script_pubkey));
}
btck_TransactionOutput* btck_transaction_output_copy(const btck_TransactionOutput* output)
{
- return btck_TransactionOutput::ref(new CTxOut{btck_TransactionOutput::get(output)});
+ return btck_TransactionOutput::copy(output);
}
const btck_ScriptPubkey* btck_transaction_output_get_script_pubkey(const btck_TransactionOutput* output)
@@ -619,8 +631,8 @@ btck_LoggingConnection* btck_logging_connection_create(btck_LogCallback callback
LogDebug(BCLog::KERNEL, "Logger connected.");
- return btck_LoggingConnection::ref(
- new LoggingConnection{std::make_unique<std::list<std::function<void(const std::string&)>>::iterator>(connection), user_data, user_data_destroy_callback});
+ return btck_LoggingConnection::create(
+ std::make_unique<std::list<std::function<void(const std::string&)>>::iterator>(connection), user_data, user_data_destroy_callback);
}
void btck_logging_connection_destroy(btck_LoggingConnection* connection)
@@ -646,19 +658,19 @@ btck_ChainParameters* btck_chain_parameters_create(const btck_ChainType chain_ty
{
switch (chain_type) {
case btck_ChainType_MAINNET: {
- return btck_ChainParameters::ref(new std::unique_ptr<const CChainParams>{CChainParams::Main()});
+ return btck_ChainParameters::create(CChainParams::Main());
}
case btck_ChainType_TESTNET: {
- return btck_ChainParameters::ref(new std::unique_ptr<const CChainParams>{CChainParams::TestNet()});
+ return btck_ChainParameters::create(CChainParams::TestNet());
}
case btck_ChainType_TESTNET_4: {
- return btck_ChainParameters::ref(new std::unique_ptr<const CChainParams>{CChainParams::TestNet4()});
+ return btck_ChainParameters::create(CChainParams::TestNet4());
}
case btck_ChainType_SIGNET: {
- return btck_ChainParameters::ref(new std::unique_ptr<const CChainParams>{CChainParams::SigNet({})});
+ return btck_ChainParameters::create(CChainParams::SigNet({}));
}
case btck_ChainType_REGTEST: {
- return btck_ChainParameters::ref(new std::unique_ptr<const CChainParams>{CChainParams::RegTest({})});
+ return btck_ChainParameters::create(CChainParams::RegTest({}));
}
}
assert(false);
@@ -667,7 +679,7 @@ btck_ChainParameters* btck_chain_parameters_create(const btck_ChainType chain_ty
btck_ChainParameters* btck_chain_parameters_copy(const btck_ChainParameters* chain_parameters)
{
const auto& original = btck_ChainParameters::get(chain_parameters);
- return btck_ChainParameters::ref(new std::unique_ptr<const CChainParams>{std::make_unique<const CChainParams>(*original)});
+ return btck_ChainParameters::create(std::make_unique<const CChainParams>(*original));
}
void btck_chain_parameters_destroy(btck_ChainParameters* chain_parameters)
@@ -678,7 +690,7 @@ void btck_chain_parameters_destroy(btck_ChainParameters* chain_parameters)
btck_ContextOptions* btck_context_options_create()
{
- return btck_ContextOptions::ref(new ContextOptions{});
+ return btck_ContextOptions::create();
}
void btck_context_options_set_chainparams(btck_ContextOptions* options, const btck_ChainParameters* chain_parameters)
@@ -716,12 +728,12 @@ btck_Context* btck_context_create(const btck_ContextOptions* options)
LogError("Kernel context sanity check failed.");
return nullptr;
}
- return btck_Context::ref(new std::shared_ptr<const Context>(context));
+ return btck_Context::create(context);
}
btck_Context* btck_context_copy(const btck_Context* context)
{
- return btck_Context::ref(new std::shared_ptr<const Context>(btck_Context::get(context)));
+ return btck_Context::copy(context);
}
int btck_context_interrupt(btck_Context* context)
@@ -792,8 +804,7 @@ btck_ChainstateManagerOptions* btck_chainstate_manager_options_create(const btck
fs::create_directories(abs_data_dir);
fs::path abs_blocks_dir{fs::absolute(fs::PathFromString({blocks_dir, blocks_dir_len}))};
fs::create_directories(abs_blocks_dir);
- auto chainman_opts{std::make_unique<ChainstateManagerOptions>(btck_Context::get(context), abs_data_dir, abs_blocks_dir)};
- return btck_ChainstateManagerOptions::ref(chainman_opts.release());
+ return btck_ChainstateManagerOptions::create(btck_Context::get(context), abs_data_dir, abs_blocks_dir);
} catch (const std::exception& e) {
LogError("Failed to create chainstate manager options: %s", e.what());
return nullptr;
@@ -884,7 +895,7 @@ btck_ChainstateManager* btck_chainstate_manager_create(
return nullptr;
}
- return btck_ChainstateManager::ref(new ChainMan{std::move(chainman), opts.m_context});
+ return btck_ChainstateManager::create(std::move(chainman), opts.m_context);
}
btck_BlockTreeEntry* btck_chainstate_manager_get_block_tree_entry_by_hash(const btck_ChainstateManager* chainman, const btck_BlockHash* block_hash)
@@ -948,12 +959,12 @@ btck_Block* btck_block_create(const void* raw_block, size_t raw_block_length)
return nullptr;
}
- return btck_Block::ref(new std::shared_ptr<const CBlock>{block});
+ return btck_Block::create(block);
}
btck_Block* btck_block_copy(const btck_Block* block)
{
- return btck_Block::ref(new std::shared_ptr<const CBlock>(btck_Block::get(block)));
+ return btck_Block::copy(block);
}
size_t btck_block_count_transactions(const btck_Block* block)
@@ -999,7 +1010,7 @@ btck_Block* btck_block_read(const btck_ChainstateManager* chainman, const btck_B
LogError("Failed to read block.");
return nullptr;
}
- return btck_Block::ref(new std::shared_ptr<const CBlock>{block});
+ return btck_Block::create(block);
}
int32_t btck_block_tree_entry_get_height(const btck_BlockTreeEntry* entry)
@@ -1033,12 +1044,12 @@ btck_BlockSpentOutputs* btck_block_spent_outputs_read(const btck_ChainstateManag
LogError("Failed to read block spent outputs data.");
return nullptr;
}
- return btck_BlockSpentOutputs::ref(new std::shared_ptr<CBlockUndo>(block_undo));
+ return btck_BlockSpentOutputs::create(block_undo);
}
btck_BlockSpentOutputs* btck_block_spent_outputs_copy(const btck_BlockSpentOutputs* block_spent_outputs)
{
- return btck_BlockSpentOutputs::ref(new std::shared_ptr<CBlockUndo>{btck_BlockSpentOutputs::get(block_spent_outputs)});
+ return btck_BlockSpentOutputs::copy(block_spent_outputs);
}
size_t btck_block_spent_outputs_count(const btck_BlockSpentOutputs* block_spent_outputs)
@@ -1061,7 +1072,7 @@ void btck_block_spent_outputs_destroy(btck_BlockSpentOutputs* block_spent_output
btck_TransactionSpentOutputs* btck_transaction_spent_outputs_copy(const btck_TransactionSpentOutputs* transaction_spent_outputs)
{
- return btck_TransactionSpentOutputs::ref(new CTxUndo{btck_TransactionSpentOutputs::get(transaction_spent_outputs)});
+ return btck_TransactionSpentOutputs::copy(transaction_spent_outputs);
}
size_t btck_transaction_spent_outputs_count(const btck_TransactionSpentOutputs* transaction_spent_outputs)
@@ -1084,7 +1095,7 @@ const btck_Coin* btck_transaction_spent_outputs_get_coin_at(const btck_Transacti
btck_Coin* btck_coin_copy(const btck_Coin* coin)
{
- return btck_Coin::ref(new Coin{btck_Coin::get(coin)});
+ return btck_Coin::copy(coin);
}
uint32_t btck_coin_confirmation_height(const btck_Coin* coin)
Optionally, could specialize these further to improve copy for std::unique_ptr CPP types, and create for std::shared_ptr CPP types.
src/kernel/bitcoinkernel.cpp
Outdated
|
|
||
| void btck_chain_destroy(btck_Chain* chain) | ||
| { | ||
| // The chain is always unowned, so only delete the wrapper struct, not the data it is pointing to. |
There was a problem hiding this comment.
I think this distinction is irrelevant: if chain is always unowned, this function can never be called, because the caller would never have a non-const btck_Chain*, in which case we can probably just remove the function, or keep it for completeness because it's harmless. Alternatively, and preferably, let's just add a Handler::destroy function, so we can easily change the logic for all destroy functions later (e.g. add logging if nullptr was passed, which may be helpful for debugging):
git diff on 82c5036
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index fc927ea3d7..e6fc6aec1c 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -87,6 +87,12 @@ struct Handle {
{
delete reinterpret_cast<CPP*>(ptr);
}
+
+ static void destroy(C* ptr)
+ {
+ if (!ptr) return;
+ delete ptr;
+ }
};
} // namespace
@@ -456,9 +462,8 @@ int btck_transaction_to_bytes(const btck_Transaction* transaction, btck_WriteByt
void btck_transaction_destroy(btck_Transaction* transaction)
{
- if (!transaction) return;
Assert(btck_Transaction::get(transaction)->vout.size() > 0);
- delete transaction;
+ btck_Transaction::destroy(transaction);
}
btck_ScriptPubkey* btck_script_pubkey_create(const void* script_pubkey, size_t script_pubkey_len)
@@ -480,8 +485,7 @@ btck_ScriptPubkey* btck_script_pubkey_copy(const btck_ScriptPubkey* script_pubke
void btck_script_pubkey_destroy(btck_ScriptPubkey* script_pubkey)
{
- if (!script_pubkey) return;
- delete script_pubkey;
+ btck_ScriptPubkey::destroy(script_pubkey);
}
btck_TransactionOutput* btck_transaction_output_create(const btck_ScriptPubkey* script_pubkey, int64_t amount)
@@ -507,8 +511,7 @@ int64_t btck_transaction_output_get_amount(const btck_TransactionOutput* output)
void btck_transaction_output_destroy(btck_TransactionOutput* output)
{
- if (!output) return;
- delete output;
+ btck_TransactionOutput::destroy(output);
}
int btck_script_pubkey_verify(const btck_ScriptPubkey* script_pubkey,
@@ -633,7 +636,7 @@ void btck_logging_connection_destroy(btck_LoggingConnection* connection)
LogDebug(BCLog::KERNEL, "Logger disconnected.");
LogInstance().DeleteCallback(*btck_LoggingConnection::get(connection).m_connection);
- delete connection;
+ btck_LoggingConnection::destroy(connection);
// Switch back to buffering by calling DisconnectTestLogger if the
// connection that was just removed was the last one.
@@ -672,8 +675,7 @@ btck_ChainParameters* btck_chain_parameters_copy(const btck_ChainParameters* cha
void btck_chain_parameters_destroy(btck_ChainParameters* chain_parameters)
{
- if (!chain_parameters) return;
- delete chain_parameters;
+ btck_ChainParameters::destroy(chain_parameters);
}
btck_ContextOptions* btck_context_options_create()
@@ -703,8 +705,7 @@ void btck_context_options_set_validation_interface(btck_ContextOptions* options,
void btck_context_options_destroy(btck_ContextOptions* options)
{
- if (!options) return;
- delete options;
+ btck_ContextOptions::destroy(options);
}
btck_Context* btck_context_create(const btck_ContextOptions* options)
@@ -731,8 +732,7 @@ int btck_context_interrupt(btck_Context* context)
void btck_context_destroy(btck_Context* context)
{
- if (!context) return;
- delete context;
+ btck_Context::destroy(context);
}
btck_BlockTreeEntry* btck_block_tree_entry_get_previous(const btck_BlockTreeEntry* entry)
@@ -747,8 +747,7 @@ btck_BlockTreeEntry* btck_block_tree_entry_get_previous(const btck_BlockTreeEntr
void btck_block_tree_entry_destroy(btck_BlockTreeEntry* block_tree_entry)
{
- if (!block_tree_entry) return;
- delete block_tree_entry;
+ btck_BlockTreeEntry::destroy(block_tree_entry);
}
btck_ValidationMode btck_block_validation_state_get_validation_mode(const btck_BlockValidationState* block_validation_state_)
@@ -808,8 +807,7 @@ void btck_chainstate_manager_options_set_worker_threads_num(btck_ChainstateManag
void btck_chainstate_manager_options_destroy(btck_ChainstateManagerOptions* options)
{
- if (!options) return;
- delete options;
+ btck_ChainstateManagerOptions::destroy(options);
}
int btck_chainstate_manager_options_set_wipe_dbs(btck_ChainstateManagerOptions* chainman_opts, int wipe_block_tree_db, int wipe_chainstate_db)
@@ -913,7 +911,7 @@ void btck_chainstate_manager_destroy(btck_ChainstateManager* chainman)
}
}
- delete chainman;
+ btck_ChainstateManager::destroy(chainman);
}
int btck_chainstate_manager_import_blocks(btck_ChainstateManager* chainman, const char** block_file_paths, size_t* block_file_paths_lens, size_t block_file_paths_len)
@@ -988,8 +986,7 @@ btck_BlockHash* btck_block_get_hash(const btck_Block* block)
void btck_block_destroy(btck_Block* block)
{
- if (!block) return;
- delete block;
+ btck_Block::destroy(block);
}
btck_Block* btck_block_read(const btck_ChainstateManager* chainman, const btck_BlockTreeEntry* entry)
@@ -1055,8 +1052,7 @@ const btck_TransactionSpentOutputs* btck_block_spent_outputs_get_transaction_spe
void btck_block_spent_outputs_destroy(btck_BlockSpentOutputs* block_spent_outputs)
{
- if (!block_spent_outputs) return;
- delete block_spent_outputs;
+ btck_BlockSpentOutputs::destroy(block_spent_outputs);
}
btck_TransactionSpentOutputs* btck_transaction_spent_outputs_copy(const btck_TransactionSpentOutputs* transaction_spent_outputs)
@@ -1071,8 +1067,7 @@ size_t btck_transaction_spent_outputs_count(const btck_TransactionSpentOutputs*
void btck_transaction_spent_outputs_destroy(btck_TransactionSpentOutputs* transaction_spent_outputs)
{
- if (!transaction_spent_outputs) return;
- delete transaction_spent_outputs;
+ btck_TransactionSpentOutputs::destroy(transaction_spent_outputs);
}
const btck_Coin* btck_transaction_spent_outputs_get_coin_at(const btck_TransactionSpentOutputs* transaction_spent_outputs, size_t coin_index)
@@ -1104,8 +1099,7 @@ const btck_TransactionOutput* btck_coin_get_output(const btck_Coin* coin)
void btck_coin_destroy(btck_Coin* coin)
{
- if (!coin) return;
- delete coin;
+ btck_Coin::destroy(coin);
}
int btck_chainstate_manager_process_block(
@@ -1151,6 +1145,5 @@ int btck_chain_contains(const btck_Chain* chain, const btck_BlockTreeEntry* entr
void btck_chain_destroy(btck_Chain* chain)
{
- // The chain is always unowned, so only delete the wrapper struct, not the data it is pointing to.
- delete chain;
+ btck_Chain::destroy(chain);
}
There was a problem hiding this comment.
I removed the destroy functions, like you said, there is no good reason to keep them, if we only have const pointers anyway. I also removed the nullptr checks altogether. I don't think there is really a good reason to keep them.
| }; | ||
|
|
||
| template <typename CType, void (*DestroyFunc)(CType*)> | ||
| class UniqueHandle |
There was a problem hiding this comment.
This is just a handle for objects for which we haven't defined a copy function, right? I'm not entirely convinced exposing a separate type for this is a good idea, I don't think there's an inherent reason we wouldn't ever allow copying a ContextOptions, for example, so this might change in the future? An alternative approach would be to unify everything under Handle making CopyFunc optional? That should make the API more intuitive?
There was a problem hiding this comment.
The handle type is not exposed to the user (other than being defined in the header), so I don't think it matters that much. I think I prefer the current approach though, since the difference between the two is pretty clear. I think there is a decent reason why would not want to copy a ContextOptions: We transfer potential ownership of a user-space object to it, which could be a double free if we just copy it to another object. We could make it a shared pointer internally, but then it might also be confusing to be mutating it.
82c5036 to
89f9518
Compare
|
Thank you for the review @stickies-v! Took most of your suggestions and added a few minor additional cleanups. 82c5036 -> 1857296 (kernelApi_62 -> kernelApi_63, compare)
|
As a first step, implement the equivalent of what was implemented in the now deprecated libbitcoinconsensus header. Also add a test binary to exercise the header and library. Unlike the deprecated libbitcoinconsensus the kernel library can now use the hardware-accelerated sha256 implementations thanks for its statically-initialzed context. The functions kept around for backwards-compatibility in the libbitcoinconsensus header are not ported over. As a new header, it should not be burdened by previous implementations. Also add a new error code for handling invalid flag combinations, which would otherwise cause a crash. The macros used in the new C header were adapted from the libsecp256k1 header. To make use of the C header from C++ code, a C++ header is also introduced for wrapping the C header. This makes it safer and easier to use from C++ code. Co-authored-by: stickies-v <stickies-v@protonmail.com>
Exposing logging in the kernel library allows users to follow what is going on when using it. Users of the C header can use `kernel_logging_connection_create(...)` to pass a callback function to Bitcoin Core's internal logger. Additionally the level and severity can be globally configured. By default, the logger buffers messages until `kernel_loggin_connection_create(...)` is called. If the user does not want any logging messages, it is recommended that `kernel_disable_logging()` is called, which permanently disables the logging and any buffering of messages.
The context introduced here holds the objects that will be required for running validation tasks, such as the chosen chain parameters, callbacks for validation events, and an interrupt utility. These will be used in a few commits, once the chainstate manager is introduced. This commit also introduces conventions for defining option objects. A common pattern throughout the C header will be: ``` options = object_option_create(); object = object_create(options); ``` This allows for more consistent usage of a "builder pattern" for objects where options can be configured independently from instantiation.
As a first option, add the chainparams. For now these can only be instantiated with default values. In future they may be expanded to take their own options for regtest and signet configurations. This commit also introduces a unique pattern for setting the option values when calling the `*_set(...)` function.
The notifications are used for notifying on connected blocks and on warning and fatal error conditions. The user of the C header may define callbacks that gets passed to the internal notification object in the `kernel_NotificationInterfaceCallbacks` struct. Each of the callbacks take a `user_data` argument that gets populated from the `user_data` value in the struct. It can be used to recreate the structure containing the callbacks on the user's side, or to give the callbacks additional contextual information.
This is the main driver class for anything validation related, so expose it here. Creating the chainstate manager options will currently also trigger the creation of their respectively configured directories. The chainstate manager and block manager options are consolidated into a single object. The kernel might eventually introduce a separate block manager object for the purposes of being a light-weight block store reader. The chainstate manager will associate with the context with which it was created for the duration of its lifetime. It is only valid if that context remains in memory too. The tests now also create dedicated temporary directories. This is similar to the behaviour in the existing unit test framework. Co-authored-by: stickies-v <stickies-v@protonmail.com>
Re-use the same pattern used for the context options. This allows users to set the number of threads used in the validation thread pool.
The library will now internally load the chainstate when a new ChainstateManager is instantiated. Options for controlling details of loading the chainstate will be added over the next few commits.
The added function allows the user process and validate a given block with the chainstate manager. The *_process_block(...) function does some preliminary checks on the block before passing it to `ProcessNewBlock(...)`. These are similar to the checks in the `submitblock()` rpc. Richer processing of the block validation result will be made available in the following commits through the validation interface. The commits also adds a utility for deserializing a `CBlock` (`kernel_block_create()`) that may then be passed to the library for processing. The tests exercise the function for both mainnet and regtest. The commit also adds the data of 206 regtest blocks (some blocks also contain transactions).
Adds options for wiping the chainstate and block tree indexes to the chainstate load options. In combination and once the `*_import_blocks(...)` function is added in a later commit, this triggers a reindex. For now, it just wipes the existing data.
This allows a user to run the kernel without creating on-disk files for the block tree and chainstate indexes. This is potentially useful in scenarios where the user needs to do some ephemeral validation operations. One specific use case is when linearizing the blocks on disk. The block files store blocks out of order, so a program may utilize the library and its header to read the blocks with one chainstate manager, and then write them back in order, and without orphans, with another chainstate maanger. To save disk resources and if the indexes are not required once done, it may be beneficial to keep the indexes in memory for the chainstate manager that writes the blocks back again.
The `kernel_import_blocks` function is used to both trigger a reindex, if the indexes were previously wiped through the chainstate load options, or import the block data of a single block file. The behaviour of the import can be verified through the test logs.
Calling interrupt can halt long-running functions associated with objects that were created through the passed-in context.
This adds the infrastructure required to process validation events. For now the external validation interface only has support for the `BlockChecked` callback, but support for the other internal validation interface methods can be added in the future. The validation interface follows an architecture for defining its callbacks and ownership that is similar to the notifications. The task runner is created internally with a context, which itself internally creates a unique ValidationSignals object. When the user creates a new chainstate manager the validation signals are internally passed to the chainstate manager through the context. The callbacks block any further validation execution when they are called. It is up to the user to either multiplex them, or use them otherwise in a multithreaded mechanism to make processing the validation events non-blocking. A validation interface can register for validation events with a context. Internally the passed in validation interface is registerd with the validation signals of a context. The BlockChecked callback introduces a seperate type for a non-owned block. Since a library-internal object owns this data, the user needs to be explicitly prevented from deleting it. In a later commit a utility will be added to copy its data.
These allow for the interpretation of the data in a `BlockChecked` validation interface callback. This is useful to get richer information in case a block failed to validate.
This adds functions for copying serialized block data into a user-owned variable-sized byte array. Use it in the tests for verifying the implementation of the validation interface's `BlockChecked` method.
This adds functions for reading a block from disk with a retrieved block tree entry. External services that wish to build their own index, or analyze blocks can use this to retrieve block data. The block tree can now be traversed from the tip backwards. This is guaranteed to work, since the chainstate maintains an internal block tree index in memory and every block (besides the genesis) has an ancestor. The user can use this function to iterate through all blocks in the chain (starting from the tip). The tip is retrieved from a separate `Chain` object, which allows distinguishing whether entries are currently in the best chain. Once the block tree entry for the genesis block is reached a nullptr is returned if the user attempts to get the previous entry.
This adds functions for reading the undo data from disk with a retrieved block tree entry. The undo data of a block contains all the spent script pubkeys of all the transactions in a block. For ease of understanding the undo data is renamed to spent outputs with seperate data structures exposed for a block's and a transaction's spent outputs. In normal operations undo data is used during re-orgs. This data might also be useful for building external indexes, or to scan for silent payment transactions. Internally the block undo data contains a vector of transaction undo data which contains a vector of the coins consumed. The coins are all int the order of the transaction inputs of the consuming transactions. Each coin can be used to retrieve a transaction output and in turn a script pubkey and amount.
Adds further functions useful for traversing the block index and retrieving block information. This includes getting the block height and hash.
This is useful for a host block processing feature where having an identifier for the block is needed. Without this, external users need to serialize the block and calculate the hash externally, which is less efficient.
This showcases a re-implementation of bitcoin-chainstate only using the kernel C++ API header.
And turn it on in the CI.
89f9518 to
1857296
Compare
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