fix(kv): reject duplicate primary keys in multi_index::emplace

.gitignore

@@ -41,7 +41,8 @@ compile_commands.json
 [Bb]uild*/
 .ccache/
 .vcpkg-binary-cache/
-cmake-build-debug/
+# CLion-style build dirs: cmake-build-debug/, cmake-build-debug-vcpkg/, cmake-build-release/, ...
+cmake-build-*/
 examples/multi_index_example/build
 examples/hello/build
 
@@ -67,3 +68,14 @@ tmp/
 
 # oh-my-claudecode runtime state (operational artifacts, never committed)
 .omc/
+
+# prequel local review state (operational artifacts, never committed)
+.prequel/
+
+# Core dumps. Restricted to the shapes the kernel actually writes here -- core_pattern is
+# core.%e.%p -- and root-anchored, so neither a tracked header such as
+# libraries/boost/include/boost/hana/core.hpp nor a future core.cpp/core.hpp is hidden.
+/core
+/core.[0-9]*
+/core.*.[0-9]*
+/vgcore.[0-9]*

libraries/sysiolib/contracts/sysio/kv_multi_index.hpp

@@ -2,8 +2,9 @@
 /**
  * KV-backed multi_index emulation layer.
  *
- * Drop-in replacement for sysio::multi_index that uses KV intrinsics instead
- * of legacy db_*_i64 intrinsics. Same template API, different backend.
+ * A shim for sysio::multi_index that uses KV intrinsics instead of the legacy db_*_i64
+ * ones. The template API is the same in almost every respect; the places it is not are
+ * listed above the class.
  *
  * Key encoding: [scope: 8B BE][primary_key: 8B BE] = 16 bytes.
  * Table name is encoded in table_id (DJB2 hash of raw template parameter),
@@ -22,12 +23,14 @@
 #include <sysio/datastream.hpp>
 #include <sysio/check.hpp>
 #include <sysio/action.hpp>
+#include <sysio/context.hpp>
 
 #include <vector>
 #include <memory>
 #include <map>
 #include <cstring>
 #include <type_traits>
+#include <utility>
 #include <limits>
 #include <iterator>
 
@@ -140,7 +143,38 @@ namespace _kv_multi_index_detail {
 } // namespace _kv_multi_index_detail
 
 // Uses sysio::indexed_by and sysio::const_mem_fun from the standard CDT headers.
-// This class is a drop-in replacement: just change multi_index -> kv_multi_index.
+//
+// A shim for the EOSIO multi_index over a different store. Nearly all contract code carries
+// over. Two things do not, and they are worth keeping apart.
+//
+// SOURCE BREAKS AGAINST UPSTREAM -- code that compiles there and not here:
+//
+//   - the postfix iterator operators are deleted, because copying a KV iterator duplicates a
+//     host-side handle. Note rbegin()/rend() hand back a std::reverse_iterator, whose postfix
+//     operators are the adaptor's and are NOT deleted, so reverse loops compile silently and
+//     the sweep does not find them;
+//   - the primary bounds are uint64_t/name overloads where upstream has a member template, so
+//     an explicit call -- `t.template lower_bound<uint64_t>(k)`, likewise upper_bound -- is
+//     rejected with "does not refer to a template". A wrapper convertible to BOTH uint64_t and
+//     name is also ambiguous here (see the note at the bounds).
+//
+// RESTRICTIONS SHARED WITH UPSTREAM, which are not breaks even though they bite:
+//
+//   - taking the bare address of a primary bound, `&table::lower_bound`, does not compile --
+//     here because the name is an overload set, upstream because a member template's parameter
+//     cannot be deduced. A named static_cast resolves one on Wire;
+//   - a secondary key must be trivially copyable. Upstream's supported secondary types are all
+//     trivially copyable too; what differs is only where it is diagnosed. The static_assert
+//     lives in secondary_index_view, so it fires at get_index<...>(), not at declaration.
+//
+// The mutators reject a duplicate primary key and a handle whose code is not the receiving
+// account, matching upstream. Each guard is documented where it stands.
+//
+// sysio::multi_index is a direct alias of this template. sysio::singleton is not: it aliases
+// kv_singleton, which holds a kv_multi_index as a PRIVATE member. Its surface is single-row
+// accessors and mutators -- not the table's iterators, bounds or secondary-index API -- so it
+// is bound by the mutators' guards, and a singleton handle on another account is read-only,
+// but none of the divergences above are reachable through it.
 
 template<name::raw TableName, typename T, typename... Indices>
 class kv_multi_index {
@@ -155,6 +189,20 @@ class kv_multi_index {
    static uint64_t to_pk_uint64(uint64_t pk) { return pk; }
    static uint64_t to_pk_uint64(name pk) { return pk.value; }
 
+   /// The receiving account, avoiding a host call where possible.
+   ///
+   /// The generated dispatcher stores the receiver in sysio_contract_name at the top of
+   /// apply() -- and apply() is re-entered per receiver, so it is correct under notification
+   /// too -- making this a plain global read. SYSIO_DISPATCH emits its own strong apply() and
+   /// the native dispatch sets nothing, leaving the global 0, which is not a valid account
+   /// name and so is a safe "unset" sentinel; there we pay the intrinsic, as upstream always
+   /// does. Deliberately not cached on the object: a contract may hold a `static` table, and
+   /// the receiver differs between the initial action and a notification handler.
+   static name receiving_account() {
+      const name ctx = current_context_contract();
+      return ctx.value ? ctx : current_receiver();
+   }
+
    name     _code;
    uint64_t _scope;
    mutable uint64_t _next_primary_key = 0;
@@ -312,7 +360,7 @@ class kv_multi_index {
          using extractor_t = typename Index::secondary_extractor_type;
          extractor_t ext;
          auto sec_key = idx.encode_scoped_secondary(ext(obj));
-         auto pri_key = idx.pk_to_bytes(obj.primary_key());
+         auto pri_key = idx.pk_to_bytes(kv_multi_index::to_pk_uint64(obj.primary_key()));
          ::kv_idx_store(payer, _sec_tid,
                         pri_key.data, _kv_multi_index_detail::u64_size,
                         sec_key.data(), sec_key.size());
@@ -325,7 +373,7 @@ class kv_multi_index {
          using extractor_t = typename Index::secondary_extractor_type;
          extractor_t ext;
          auto sec_key = idx.encode_scoped_secondary(ext(obj));
-         auto pri_key = idx.pk_to_bytes(obj.primary_key());
+         auto pri_key = idx.pk_to_bytes(kv_multi_index::to_pk_uint64(obj.primary_key()));
          ::kv_idx_remove(_sec_tid,
                          pri_key.data, _kv_multi_index_detail::u64_size,
                          sec_key.data(), sec_key.size());
@@ -339,7 +387,7 @@ class kv_multi_index {
          extractor_t ext;
          auto old_sec = idx.encode_scoped_secondary(ext(old_obj));
          auto new_sec = idx.encode_scoped_secondary(ext(new_obj));
-         auto pri_key = idx.pk_to_bytes(old_obj.primary_key());
+         auto pri_key = idx.pk_to_bytes(kv_multi_index::to_pk_uint64(old_obj.primary_key()));
          if (old_sec != new_sec) {
             ::kv_idx_update(payer, _sec_tid,
                             pri_key.data, _kv_multi_index_detail::u64_size,
@@ -592,6 +640,24 @@ class kv_multi_index {
       return *obj;
    }
 
+   /// Two concrete overloads, the same shape find/require_find/get use above: a one-line
+   /// `name` form delegating to the `uint64_t` one.
+   ///
+   /// Concrete overloads rather than a template or a converting-proxy parameter, because both
+   /// of those change what the argument means. A template cannot deduce `lower_bound({42})`;
+   /// a proxy accepts `lower_bound({w})` for a `w` converting to a narrower type, which a real
+   /// `uint64_t` parameter rejects as narrowing. The parameter here is a `uint64_t`, so every
+   /// conversion is the one a `uint64_t` parameter performs.
+   ///
+   /// Two consequences of the overload pair, both shared with the three siblings above:
+   ///
+   ///   - `&table::lower_bound` is an overload set, so the bare address cannot be taken. A
+   ///     named cast resolves either one:
+   ///     `static_cast<const_iterator (table::*)(uint64_t) const>(&table::lower_bound)`.
+   ///   - a wrapper convertible to BOTH `uint64_t` and `name` is ambiguous.
+   ///
+   /// Both are pinned by test.
+   const_iterator lower_bound(name primary) const { return lower_bound(primary.value); }
    const_iterator lower_bound(uint64_t primary) const {
       auto key = make_pk(primary);
       auto prefix = make_prefix();
@@ -600,6 +666,7 @@ class kv_multi_index {
       return const_iterator(this, handle, status == 0);
    }
 
+   const_iterator upper_bound(name primary) const { return upper_bound(primary.value); }
    const_iterator upper_bound(uint64_t primary) const {
       if (primary == std::numeric_limits<uint64_t>::max()) return end();
       return lower_bound(primary + 1);
@@ -620,13 +687,30 @@ class kv_multi_index {
 
    template<typename Lambda>
    const_iterator emplace(name payer, Lambda&& constructor) {
+      // Reads honour _code (kv_get/kv_contains take a code argument) but writes do not:
+      // kv_set and kv_idx_store have no code parameter and always land on the receiver. A
+      // foreign-code handle would therefore probe one account and write another -- upstream
+      // rejects that, and a ported contract relying on the abort would otherwise get a silent
+      // write to its own row. Checked before the constructor runs, so a lambda with side
+      // effects is not executed on the rejected path.
+      check(_code == receiving_account(), "cannot create objects in table of another contract");
+
       T obj;
       constructor(obj);
 
       uint64_t pk = to_pk_uint64(obj.primary_key());
       auto key = make_pk(pk);
       auto value = serialize_row(obj);
 
+      // Reject a duplicate primary key. Nothing below this point will: kv_set is an upsert,
+      // so the row would be silently overwritten, and store_secondaries is an unconditional
+      // kv_idx_store, so the old (sec_key -> pri_key) mapping would survive and point at a
+      // row whose secondary value has changed. On Antelope db_store_i64 rejected duplicates
+      // at the chain layer; the KV intrinsics do not, so the wrapper must.
+      // kv::table::emplace checks the same way.
+      check(!::kv_contains(_table_id, _code.value, key.data, key_size),
+            "object with the same primary key already exists");
+
       ::kv_set(_table_id, payer.value, key.data, key_size, value.data(), value.size());
       store_secondaries(payer.value, obj);
 
@@ -652,6 +736,8 @@ class kv_multi_index {
 
    template<typename Lambda>
    void modify(const T& obj, name payer, Lambda&& updater) {
+      check(_code == receiving_account(), "cannot modify objects in table of another contract");
+
       T old_obj = obj;
       // Cast away const for modification (same pattern as legacy multi_index)
       auto& mutable_obj = const_cast<T&>(obj);
@@ -679,6 +765,8 @@ class kv_multi_index {
    }
 
    void erase(const T& obj) {
+      check(_code == receiving_account(), "cannot erase objects in table of another contract");
+
       uint64_t pk = to_pk_uint64(obj.primary_key());
       auto key = make_pk(pk);
 

libraries/sysiolib/contracts/sysio/kv_table.hpp

@@ -435,7 +435,18 @@ class table_impl {
       sec_ops::update_all(*this, payer, pri.data(), pri.size(), old_val, new_val);
    }
 
-   // Internal insert (no duplicate check — caller must verify)
+private:
+   // Internal insert (no duplicate check — caller must verify).
+   //
+   // Private deliberately. Inserting over an existing key here silently overwrites the row
+   // and, because store_secondaries is an unconditional kv_idx_store, either strands the old
+   // (sec_key -> pri_key) mapping (when the secondary value changed) or trips the host's
+   // ordered_unique constraint on (code, table_id, sec_key, pri_key). emplace() pays one
+   // kv_contains so no PUBLIC path reaches an unguarded insert.
+   //
+   // Only do_insert is sealed. store_secondaries, remove_secondaries, update_secondaries and
+   // do_erase below are public, and calling store_secondaries directly strands a mapping the
+   // same way -- treat them as internal.
    void do_insert(uint64_t payer, const be_key_stream& k, const K& key, const V& value) {
       if constexpr (is_fixed_serializable_v<V>) {
          char vbuf[sizeof(V)];
@@ -448,6 +459,7 @@ class table_impl {
       store_secondaries(payer, key, value);
    }
 
+public:
    // Internal erase used by both primary and secondary erase paths
    void do_erase(const K& key, const V& value) {
       remove_secondaries(key, value);
@@ -695,6 +707,17 @@ class table_impl {
 
    /// Insert a new row. Asserts if the key already exists. Use upsert()/set()
    /// for insert-or-update semantics.
+   ///
+   /// WRITES IGNORE code(), as they do in kv::global. kv_get and kv_contains take a code
+   /// argument, so reads honour whatever account this handle was constructed with; kv_set,
+   /// kv_erase and kv_idx_store have no such parameter and always land on the current
+   /// receiver. A handle opened on a FOREIGN account is therefore read-only in practice --
+   /// mutating through one probes their table and writes your own, and because table_id is
+   /// derived from the table name alone, that write lands on your row of the same name.
+   /// Nothing detects it at compile time. Construct foreign-code handles for reading only.
+   ///
+   /// (sysio::multi_index does guard this, because upstream does and ported contracts rely
+   /// on the abort; these wrappers have no such compatibility obligation.)
    void emplace(name payer, const K& key, const V& value, const char* exists_msg = "key already exists") {
       auto k = make_key(key);
       sysio::check(!::kv_contains(_table_id, code(), k.data(), k.size()), exists_msg);

libraries/sysiolib/core/sysio/context.hpp

@@ -4,8 +4,11 @@
 
 namespace sysio {
    namespace internal_use_do_not_use {
-      extern "C" uint64_t sysio_contract_name;
+      /// volatile MUST match the definition in sysiolib.cpp -- differing cv-qualification on
+      /// the same entity is ill-formed (no diagnostic required). It links today only because
+      /// extern "C" names carry no type and no translation unit sees both spellings.
+      extern "C" volatile uint64_t sysio_contract_name;
    }
 
-   inline name current_context_contract() { return name{internal_use_do_not_use::sysio_contract_name}; }
+   inline name current_context_contract() { return name{uint64_t{internal_use_do_not_use::sysio_contract_name}}; }
 }

tests/CMakeLists.txt

@@ -24,6 +24,7 @@ add_unit_test( time_tests )
 add_unit_test( varint_tests )
 add_unit_test( kv_table_tests )
 add_unit_test( kv_cached_tests )
+add_unit_test( kv_multi_index_tests )
 
 add_test( NAME toolchain_tests COMMAND ${CMAKE_BINARY_DIR}/tools/toolchain-tester/toolchain-tester ${CMAKE_SOURCE_DIR}/tests/toolchain --cdt ${CMAKE_BINARY_DIR}/bin --verbose )
 set_property(TEST toolchain_tests PROPERTY LABELS toolchain_tests)
@@ -36,6 +37,10 @@ configure_file(${CMAKE_CURRENT_SOURCE_DIR}/unit/abi_version_tests.sh ${CMAKE_BIN
 add_test(NAME abi_version_tests COMMAND ${CMAKE_BINARY_DIR}/tests/unit/abi_version_tests.sh "${CMAKE_BINARY_DIR}" WORKING_DIRECTORY ${CMAKE_BINARY_DIR})
 set_property(TEST abi_version_tests PROPERTY LABELS unit_tests)
 
+configure_file(${CMAKE_CURRENT_SOURCE_DIR}/unit/dispatch_receiver_tests.sh ${CMAKE_BINARY_DIR}/tests/unit/dispatch_receiver_tests.sh COPYONLY)
+add_test(NAME dispatch_receiver_tests COMMAND ${CMAKE_BINARY_DIR}/tests/unit/dispatch_receiver_tests.sh "${CMAKE_BINARY_DIR}" WORKING_DIRECTORY ${CMAKE_BINARY_DIR})
+set_property(TEST dispatch_receiver_tests PROPERTY LABELS unit_tests)
+
 configure_file(${CMAKE_CURRENT_SOURCE_DIR}/unit/multidir_contract_tests.sh ${CMAKE_BINARY_DIR}/tests/unit/multidir_contract_tests.sh COPYONLY)
 add_test(NAME multidir_contract_tests COMMAND ${CMAKE_BINARY_DIR}/tests/unit/multidir_contract_tests.sh "${CMAKE_BINARY_DIR}" WORKING_DIRECTORY ${CMAKE_BINARY_DIR})
 set_property(TEST multidir_contract_tests PROPERTY LABELS unit_tests)

tests/integration/multi_index_tests.cpp

@@ -35,6 +35,15 @@ BOOST_FIXTURE_TEST_CASE(main_multi_index_tests, TESTER) { try {
    };
 
    push_action( "testapi"_n, "s1g"_n,  "testapi"_n, {} );        // idx64_general
+   push_action( "testapi"_n, "s1namepk"_n, "testapi"_n, {} );    // name_pk_secondaries
+
+   // A foreign-code handle cannot mutate: reads honour the handle's code but writes land on
+   // the receiver, so without the guard this silently wrote the receiver's own row.
+   check_failure( "s1foreign"_n, "cannot create objects in table of another contract" );
+
+   // Duplicate primary key aborts instead of upserting. Without the guard this action
+   // succeeded and left the previous secondary mapping stranded.
+   check_failure( "s1dupidx"_n, "object with the same primary key already exists" );
    push_action( "testapi"_n, "s1store"_n,  "testapi"_n, {} );    // idx64_store_only
    push_action( "testapi"_n, "s1check"_n,  "testapi"_n, {} );    // idx64_check_without_storing
    push_action( "testapi"_n, "s2g"_n,  "testapi"_n, {} );        // idx128_general

tests/unit/CMakeLists.txt

@@ -35,6 +35,7 @@ add_cdt_unit_test(time_tests)
 add_cdt_unit_test(varint_tests)
 add_cdt_unit_test(kv_table_tests)
 add_cdt_unit_test(kv_cached_tests)
+add_cdt_unit_test(kv_multi_index_tests)
 
 target_compile_options( rope_tests PUBLIC -g )
 add_subdirectory(test_contracts)