Replace legacy db_*_i64 intrinsics with KV database API#41
Replace legacy db_*_i64 intrinsics with KV database API#41
Conversation
jglanz
left a comment
There was a problem hiding this comment.
Some needed changes. also, whats the replacement for abigen?
cmake/ToolsExternalProject.cmake
Outdated
| ExternalProject_Add( | ||
| CDTTools | ||
| CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX} -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER} -DCMAKE_C_COMPILER=${CMAKE_C_COMPILER} -DVERSION_FULL=${VERSION_FULL} -DCMAKE_INSTALL_BINDIR=${CMAKE_INSTALL_BINDIR} -DVERSION_MAJOR=${VERSION_MAJOR} -DVERSION_MINOR=${VERSION_MINOR} -DVERSION_PATCH=${VERSION_PATCH} -DCMAKE_CXX_FLAGS=${CMAKE_CXX_FLAGS} -DCMAKE_TOOLCHAIN_FILE=${CMAKE_TOOLCHAIN_FILE} -DVCPKG_INSTALLED_DIR=${CMAKE_BINARY_DIR}/vcpkg_installed -DCDT_INSTALL_PREFIX=${CDT_INSTALL_PREFIX} -DLLVM_DIR=${LLVM_DIR} -DClang_DIR=${Clang_DIR} -DCMAKE_EXPORT_COMPILE_COMMANDS=ON | ||
| CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX} -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER} -DCMAKE_C_COMPILER=${CMAKE_C_COMPILER} -DVERSION_FULL=${VERSION_FULL} -DCMAKE_INSTALL_BINDIR=${CMAKE_INSTALL_BINDIR} -DVERSION_MAJOR=${VERSION_MAJOR} -DVERSION_MINOR=${VERSION_MINOR} -DVERSION_PATCH=${VERSION_PATCH} -DCMAKE_CXX_FLAGS=${CMAKE_CXX_FLAGS} -DCMAKE_TOOLCHAIN_FILE=${CMAKE_TOOLCHAIN_FILE} -DVCPKG_INSTALLED_DIR=${CMAKE_BINARY_DIR}/vcpkg_installed -DCDT_INSTALL_PREFIX=${CDT_INSTALL_PREFIX} -DLLVM_DIR=${LLVM_DIR} -DClang_DIR=${Clang_DIR} -DCMAKE_PREFIX_PATH=${CMAKE_BINARY_DIR}/vcpkg_installed/x64-linux -DCMAKE_EXPORT_COMPILE_COMMANDS=ON |
There was a problem hiding this comment.
For CMAKE_PREFIX_PATH, use the ${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET} .
Forward VCPKG_INSTALLED_DIR & VCPKG_TARGET_TRIPLET
Also, line wrap and indent args
docs/kv-storage-guide.md
Outdated
| | Feature | multi\_index | kv::table | | ||
| |---------|-------------|-----------| | ||
| | Drop-in replacement for legacy | Yes | No | | ||
| | Secondary indices | Yes (up to 16) | No | |
There was a problem hiding this comment.
How are secondary indices being support when emulated, but not by the new stack?
There was a problem hiding this comment.
There are MANY cases where secondary indexes are required, referring to them as legacy seems wrong?
There was a problem hiding this comment.
Secondary indices are fully supported by multi_index — they use the new kv_idx_* intrinsics under the hood (11 unified functions replacing the 50 legacy db_idx* functions). The old EOSIO secondary index host functions are gone, but the CDT API and functionality are identical.
The table in the doc was comparing multi_index vs kv::table (the lower-level zero-copy API which intentionally omits secondary indices for simplicity). Updated the wording to clarify that secondary indices are a current feature, not legacy. Also removed "legacy" when referring to multi_index throughout the doc — it is a supported, first-class API.
c777b8b to
43e7e81
Compare
43e7e81 to
86ac1c8
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates contract storage and tooling from legacy db_*_i64 intrinsics to a new KV database API, adding new CDT headers/wrappers and updating ABI generation/merging and tests to support KV key metadata.
Changes:
- Introduces KV-backed contract storage APIs (
kv_multi_index,kv::table,kv_singleton,kv::map) plus C intrinsics header<sysio/kv.h>, and deprecates legacy<sysio/db.h>. - Updates abigen/abimerge to emit and merge ABI table key metadata (
key_names/key_types) and adds[[sysio::kv_key]]. - Updates native intrinsic lists and unit/toolchain tests to use KV intrinsics and KV-backed multi_index/singleton.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/include/sysio/abimerge.hpp | ABI merge logic updated to tolerate differing key metadata and prefer “richer” table entries. |
| tests/unit/test_contracts/name_pk_tests.cpp | Switches test to kv_multi_index. |
| tests/unit/test_contracts/multi_index_tests.cpp | Switches tests to kv_multi_index and includes KV header. |
| tests/unit/test_contracts/get_code_hash_write.cpp | Uses kv_multi_index for code-hash table. |
| tests/unit/test_contracts/get_code_hash_read.cpp | Uses kv_multi_index for code-hash table. |
| tests/unit/test_contracts/explicit_nested_tests.cpp | Uses kv_multi_index for nested table test. |
| tests/unit/test_contracts/capi/db.c | Replaces legacy db C API tests with KV intrinsic calls. |
| tests/unit/test_contracts/array_tests.cpp | Switches table typedef to kv_multi_index. |
| tests/toolchain/compile-fail/host_functions_tests.json | Adjusts expected compile-fail stderr count. |
| tests/toolchain/compile-fail/host_functions_tests.cpp | Replaces legacy db intrinsics with KV write intrinsics for read-only rejection tests. |
| tests/toolchain/abigen-pass/using_std_array.cpp | Uses kv_multi_index to ensure abigen output includes key metadata. |
| tests/toolchain/abigen-pass/using_std_array.abi | Updates expected ABI table key metadata arrays. |
| tests/toolchain/abigen-pass/singleton_contract.cpp | Switches multi_index typedefs to kv_multi_index. |
| tests/toolchain/abigen-pass/singleton_contract.abi | Updates expected ABI structs/tables and key metadata. |
| tests/toolchain/abigen-pass/kv_key_types.json | Adds new abigen-pass test for KV key metadata. |
| tests/toolchain/abigen-pass/kv_key_types.cpp | New contract validating auto key metadata and [[sysio::kv_key]]. |
| tests/toolchain/abigen-pass/kv_key_types.abi | Expected ABI for kv_key_types test. |
| tests/toolchain/abigen-pass/exclude_from_abi.hpp | Switches embedded multi_index typedefs to kv_multi_index. |
| plugins/sysio/sysio_attrs.cpp | Registers new sysio::kv_key attribute. |
| plugins/sysio/clang_wrapper.hpp | Adds wrapper helpers for detecting/reading sysio_kv_key. |
| plugins/sysio/abigen.hpp | Emits key metadata for KV templates and [[sysio::kv_key]]; keeps key structs in ABI validation. |
| libraries/sysiolib/core/sysio/key_encoder.hpp | Adds a key encoder utility for order-preserving composite keys. |
| libraries/sysiolib/contracts/sysio/singleton.hpp | Converts legacy singleton header into KV-backed alias. |
| libraries/sysiolib/contracts/sysio/kv_table.hpp | Adds KV-backed table abstraction with iterators and optional zero-copy path. |
| libraries/sysiolib/contracts/sysio/kv_singleton.hpp | Adds KV-backed singleton wrapper. |
| libraries/sysiolib/contracts/sysio/kv_multi_index.hpp | Adds KV-backed multi_index emulation + secondary index support. |
| libraries/sysiolib/contracts/sysio/kv_map.hpp | Adds ordered KV map with BE key encoding and iterators. |
| libraries/sysiolib/capi/sysio/kv.h | Introduces C declarations for KV intrinsics. |
| libraries/sysiolib/capi/sysio/db.h | Replaces legacy db C API header with a stub that includes <sysio/kv.h>. |
| libraries/native/native/sysio/intrinsics_def.hpp | Removes legacy db intrinsic registration and adds KV intrinsics. |
| libraries/native/intrinsics.cpp | Removes C-ABI shims for legacy db intrinsics. |
| docs/kv-storage-guide.md | Adds a developer guide for KV storage usage/migration. |
| docs/kv-intrinsics-reference.md | Adds detailed intrinsic reference documentation. |
| docs/kv-abi-key-metadata.md | Documents ABI key metadata generation and SHiP usage. |
| cmake/ToolsExternalProject.cmake | Refactors ExternalProject CMake args (notably vcpkg prefix/triplet propagation). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Alias: sysio::singleton is now the KV-backed implementation | ||
| template<sysio::name::raw SingletonName, typename T> | ||
| using singleton = kv_singleton<SingletonName, T>; | ||
|
|
There was a problem hiding this comment.
This header defines template<...> using singleton = kv_singleton<...>;, but <sysio/singleton.hpp> also defines the same alias and includes this header, causing an alias redefinition compile error. Remove the alias from one of the headers (preferably keep it only in the backward-compat singleton.hpp).
| // Alias: sysio::singleton is now the KV-backed implementation | |
| template<sysio::name::raw SingletonName, typename T> | |
| using singleton = kv_singleton<SingletonName, T>; |
| const_iterator(const const_iterator& o) : _idx(o._idx), _valid(o._valid), _obj(o._obj) { | ||
| if (o._handle != uint32_t(-1) && _idx) { | ||
| auto prefix = _idx->make_prefix(); | ||
| _handle = ::kv_it_create(_idx->_code.value, prefix.data, 16); | ||
| if (_valid && _obj) { | ||
| auto key = _idx->make_pk(uint64_t(_obj->primary_key())); | ||
| ::kv_it_lower_bound(_handle, key.data, 24); | ||
| } |
There was a problem hiding this comment.
The iterator copy/assignment code casts primary_key() to uint64_t directly (uint64_t(_obj->primary_key())). This will not compile for tables whose primary_key() returns sysio::name (there is no implicit conversion to uint64_t). Use the existing to_pk_uint64(...) helper for both uint64_t and name primary keys.
| char max_sec[8]; | ||
| memset(max_sec, 0xFF, 8); | ||
| _handle = ::kv_idx_lower_bound( | ||
| _mi->_code.value, static_cast<uint64_t>(TableName), index_number, | ||
| max_sec, 8); |
There was a problem hiding this comment.
Secondary index reverse-iteration from end() uses an 8-byte max_sec key (all 0xFF) to seek past the end. This is only valid for 8-byte secondary keys; for uint128_t, double, custom serialized keys, etc., it can fail to position at the true end and break rbegin()/--end(). Use a maximal key of the correct encoded length (e.g., a 256-byte 0xFF buffer, or a key sized to the index’s encoded key size) when calling kv_idx_lower_bound.
| char max_sec[8]; | |
| memset(max_sec, 0xFF, 8); | |
| _handle = ::kv_idx_lower_bound( | |
| _mi->_code.value, static_cast<uint64_t>(TableName), index_number, | |
| max_sec, 8); | |
| char max_sec[256]; | |
| memset(max_sec, 0xFF, sizeof(max_sec)); | |
| _handle = ::kv_idx_lower_bound( | |
| _mi->_code.value, static_cast<uint64_t>(TableName), index_number, | |
| max_sec, static_cast<uint32_t>(sizeof(max_sec))); |
| using extractor_t = typename std::tuple_element<index_number, std::tuple<typename Indices::secondary_extractor_type...>>::type; | ||
| extractor_t ext; | ||
| auto sec_bytes = _kv_multi_index_detail::encode_secondary(ext(_obj)); | ||
| _handle = ::kv_idx_find_secondary( | ||
| _mi->_code.value, static_cast<uint64_t>(TableName), index_number, | ||
| sec_bytes.data(), sec_bytes.size()); |
There was a problem hiding this comment.
Copying a secondary index iterator clones the position by re-running kv_idx_find_secondary for the current secondary key. If multiple rows share the same secondary key, the cloned iterator may point to a different entry (typically the first match), breaking iterator copy semantics. Consider making secondary iterators move-only, or cloning via a seek that includes both secondary key and primary key (if the host API supports it), so copies preserve the exact position.
| using extractor_t = typename std::tuple_element<index_number, std::tuple<typename Indices::secondary_extractor_type...>>::type; | |
| extractor_t ext; | |
| auto sec_bytes = _kv_multi_index_detail::encode_secondary(ext(_obj)); | |
| _handle = ::kv_idx_find_secondary( | |
| _mi->_code.value, static_cast<uint64_t>(TableName), index_number, | |
| sec_bytes.data(), sec_bytes.size()); | |
| // Preserve the exact iterator position by copying the handle | |
| // instead of re-running kv_idx_find_secondary, which could | |
| // return a different row when multiple entries share the same | |
| // secondary key. | |
| _handle = o._handle; |
| ::kv_set(1, 0, key.data, 24, buf, sz); | ||
| } else { | ||
| std::vector<char> buf(sz); | ||
| detail::store_value(obj, buf.data(), buf.size()); | ||
| ::kv_set(1, 0, key.data, 24, buf.data(), buf.size()); |
There was a problem hiding this comment.
store_row hard-codes payer=0 in kv_set, which means the payer parameters on emplace(...) / modify(...) are effectively ignored for kv::table. Either plumb the payer through to store_row and into kv_set, or remove/rename the payer parameters to avoid implying payer-based billing is supported.
| ::kv_set(1, 0, key.data, 24, buf, sz); | |
| } else { | |
| std::vector<char> buf(sz); | |
| detail::store_value(obj, buf.data(), buf.size()); | |
| ::kv_set(1, 0, key.data, 24, buf.data(), buf.size()); | |
| ::kv_set(1, _code, key.data, 24, buf, sz); | |
| } else { | |
| std::vector<char> buf(sz); | |
| detail::store_value(obj, buf.data(), buf.size()); | |
| ::kv_set(1, _code, key.data, 24, buf.data(), buf.size()); |
| be_key_stream& operator<<(int32_t v) { return *this << static_cast<uint32_t>(v); } | ||
|
|
||
| be_key_stream& operator<<(uint64_t v) { write_be64(v); return *this; } | ||
| be_key_stream& operator<<(int64_t v) { return *this << static_cast<uint64_t>(v); } |
There was a problem hiding this comment.
be_key_stream encodes int64_t by casting to uint64_t and writing big-endian. That does not preserve numeric ordering for negative values (all negatives will sort after positives). If signed keys are intended to be order-preserving, apply a sign-bit flip (XOR 1<<63) before writing, similar to the double handling or key_encoder::append(int64_t).
| be_key_stream& operator<<(int64_t v) { return *this << static_cast<uint64_t>(v); } | |
| be_key_stream& operator<<(int64_t v) { | |
| uint64_t bits = static_cast<uint64_t>(v) ^ (uint64_t(1) << 63); | |
| write_be64(bits); | |
| return *this; | |
| } |
| * Usage: | ||
| * key_encoder enc; | ||
| * enc.append(uint64_t(42)); | ||
| * enc.append(name("myaccount")); |
There was a problem hiding this comment.
The usage example calls enc.append(name("myaccount")), but key_encoder has no append(sysio::name) overload and this header does not include the name type. Either adjust the example to use an explicitly supported type (e.g., uint64_t(name.value)), or add a sysio::name overload (and include the appropriate header) so the documented usage compiles.
| * enc.append(name("myaccount")); | |
| * enc.append(std::string("myaccount")); |
docs/kv-intrinsics-reference.md
Outdated
| # Wire KV Intrinsics Reference | ||
|
|
||
| This document describes the 24 host functions (intrinsics) that implement the Wire KV database. These are declared in `<sysio/kv.h>` and implemented by the `nodeop` runtime. | ||
|
|
||
| Contract developers typically use the higher-level `sysio::multi_index` or `sysio::kv::table` APIs rather than calling these intrinsics directly. This reference is intended for CDT library authors, tooling developers, and anyone who needs to understand the low-level interface. | ||
|
|
||
| --- | ||
|
|
||
| ## General Notes | ||
|
|
||
| ### key\_format parameter | ||
|
|
||
| Several intrinsics accept a `key_format` parameter: | ||
|
|
||
| | Value | Meaning | | ||
| |-------|---------| | ||
| | 0 | Raw key -- arbitrary bytes, no special encoding assumed | | ||
| | 1 | Standard 24-byte key -- `[table:8B BE][scope:8B BE][pk:8B BE]`. Enables SSO fast-path and SHiP translation | | ||
|
|
||
| The CDT APIs always pass `key_format=1`. |
There was a problem hiding this comment.
This reference claims there are 24 KV intrinsics and that CDT APIs always pass key_format=1, but the provided <sysio/kv.h> declarations expose 22 functions, and sysio::kv::map explicitly uses key_format=0. Please reconcile the count and clarify which CDT layers use key_format=1 (e.g., multi_index / kv::table) vs key_format=0 (kv::map).
| static bool table_is_same(ojson a, ojson b) { | ||
| // key_names/key_types may differ: template-detected tables have them | ||
| // populated while attribute-only tables have empty arrays. Both are | ||
| // valid representations of the same table — treat as compatible. | ||
| return a["name"] == b["name"] && | ||
| a["type"] == b["type"] && | ||
| a["index_type"] == b["index_type"] && | ||
| a["key_names"] == b["key_names"] && | ||
| a["key_types"] == b["key_types"]; | ||
| (a["key_names"] == b["key_names"] || | ||
| a["key_names"].empty() || b["key_names"].empty()); | ||
| } |
There was a problem hiding this comment.
table_is_same now ignores key_types entirely. This can incorrectly treat two tables with identical name/type/index_type/key_names but different key_types as compatible, potentially masking ABI incompatibilities. Consider treating empty key metadata as a wildcard, but when both sides have non-empty key_names, also require key_types to match (or apply the same empty/wildcard logic to both arrays together).
| // continue to compile with #include <sysio/singleton.hpp> and sysio::singleton<...>. | ||
| template<sysio::name::raw SingletonName, typename T> | ||
| using singleton = kv_singleton<SingletonName, T>; |
There was a problem hiding this comment.
sysio::singleton is defined here as an alias, but kv_singleton.hpp also defines the same sysio::singleton alias. Including <sysio/singleton.hpp> will therefore re-define the alias and fail to compile. Keep the alias definition in only one header (typically the backward-compat singleton.hpp) and remove the duplicate from the other header.
| // continue to compile with #include <sysio/singleton.hpp> and sysio::singleton<...>. | |
| template<sysio::name::raw SingletonName, typename T> | |
| using singleton = kv_singleton<SingletonName, T>; | |
| // continue to compile with #include <sysio/singleton.hpp> and sysio::singleton<...>, | |
| // by including the primary definition from <sysio/kv_singleton.hpp>. |
- Remove 60 legacy EOSIO database intrinsics and replace with 22 KV intrinsics - New CDT headers: kv_raw_table.hpp, kv_multi_index.hpp, kv_table.hpp, kv_singleton.hpp, kv_constants.hpp, key_encoder.hpp - multi_index.hpp and singleton.hpp forward to KV implementations - key_format parameter added to kv_get, kv_erase, kv_contains, kv_it_create for format-partitioned index - Format 0 (raw_table) and format 1 (multi_index/table) stored in separate index partitions - Signed integer key encoding with sign-bit flip for correct ordering - Long double secondary encoding with endian conversion - Legacy-compatible error messages for backward compatibility - Full test coverage: kv_raw_table_tests, kv_table_tests, kv_singleton_tests, expanded multi_index_tests - Integration tests with SysioTester build-tree support - Documentation: kv-intrinsics-reference, kv-storage-guide, kv-abi-key-metadata
86ac1c8 to
4a5cfe7
Compare
Summary
db_store_i64,db_find_i64,db_idx64_*, etc.) and replace with 22 KV intrinsicskv.h(C API),kv_multi_index.hpp(drop-in replacement),kv_singleton.hpp,kv_table.hpp(zero-copy),kv_raw_table.hpp(ordered key-value store)multi_index.hppandsingleton.hppnow forward to KV implementationsdb.hgutted to just includekv.h; native intrinsics updatedKey Changes (PR review)
it++. Use++it/--it.kv_idx_find_secondary/kv_idx_lower_boundreturnint32_t—-1= not found (no handle allocated, no destroy needed),>= 0= valid handle. Saves 1–2 host calls per secondary lookup miss.kv_idx_lower_boundreturns handle initerator_endstate when bound is past all entries but table is non-empty (enableskv_idx_prevfor reverse iteration).be_key_stream—0x00→0x00,0x01, terminated by0x00,0x00. Correctly sorts strings with embedded NUL bytes. Also addedfloat,int128_t,vector<char>key support.key_encoder.hpp— unused dead code;be_key_streamis the complete key encoder.raw_tablecode parameter — optional constructor arg for cross-contract reads.sysio_codegen.soplugin now detectspre_dispatch/post_dispatchhooks andcdt-codegengenerates the correct dispatch wrappers.[[sysio::kv_key]]warning — warns when referenced key struct not found instead of silently producing empty key metadata.upper_boundUINT64_MAX overflow,available_primary_keyoverflow check,modifyby const ref, dead code cleanup, CMakeDEPENDSfor integration tests.Documentation
docs/kv-intrinsics-reference.md— Complete reference for all 22 KV intrinsicsdocs/kv-storage-guide.md— Developer guide covering migration, key encoding, raw_table, and performance tipsdocs/kv-abi-key-metadata.md— ABI key_names/key_types metadata for SHiP clientsTests
be_key_streamencoder tests: float, double, int128_t, NUL-escaped strings, vector blobskv_idx_updatecompile-fail test for read-only actionsCompanion PR
feature/db-kvbranch (22 host-side intrinsic implementations + secondary index query API)