[1.0 -> main] fix table id mix up during snapshot creation

libraries/chain/controller.cpp

@@ -2081,21 +2081,29 @@ struct controller_impl {
          using by_table_id = object_to_table_id_tag_t<value_t>;
 
          snapshot->write_section<value_t>([this, &row_counter]( auto& section ) {
-            table_id last_seen_tid = -1;
+            table_id flattened_table_id = -1; //first table id will be assigned 0 by chainbase
 
-            decltype(utils)::template walk_by<by_table_id>(db, [this, &section, &row_counter, &last_seen_tid]( const auto &row ) {
-               if(last_seen_tid != row.t_id) {
-                  auto tid_key = boost::make_tuple(row.t_id);
-                  auto next_tid_key = boost::make_tuple(table_id_object::id_type(row.t_id._id + 1));
-                  unsigned_int size = utils_t::template size_range<by_table_id>(db, tid_key, next_tid_key);
+            index_utils<table_id_multi_index>::walk(db, [this, &section, &flattened_table_id, &row_counter](const table_id_object& table_row) {
+               auto tid_key = boost::make_tuple(table_row.id);
+               auto next_tid_key = boost::make_tuple(table_id_object::id_type(table_row.id._id + 1));
 
-                  section.add_row(row.t_id, db); //indicate the new table id for next...
-                  section.add_row(size, db);     //...number of rows
+               //Tables are stored in the snapshot by their sorted by-id walked order, but without record of their table id. On snapshot
+               // load, the table index will be reloaded in order, but all table ids flattened by chainbase to their insert order.
+               // e.g. if walking table ids 4,5,10,11,12 on creation, these will be reloaded as table ids 0,1,2,3,4. Track this
+               // flattened order here to know the "new" (upon snapshot load) table id a row belongs to
+               ++flattened_table_id;
 
-                  last_seen_tid = row.t_id;
-               }
-               section.add_row(row, db);
-               row_counter.progress();
+               unsigned_int size = utils_t::template size_range<by_table_id>(db, tid_key, next_tid_key);
+               if(size == 0u)
+                  return;
+
+               section.add_row(flattened_table_id, db); //indicate the new (flattened for load) table id for next...
+               section.add_row(size, db);               //...number of rows
+
+               utils_t::template walk_range<by_table_id>(db, tid_key, next_tid_key, [this, &section, &row_counter]( const auto &row ) {
+                  section.add_row(row, db);
+                  row_counter.progress();
+               });
             });
          });
       });

unittests/snapshot_tests.cpp

@@ -760,4 +760,49 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(jumbo_row, SNAPSHOT_SUITE, snapshot_suites)
    jumbo_row_test<savanna_tester, SNAPSHOT_SUITE>();
 }
 
+template<typename TESTER, typename SNAPSHOT_SUITE>
+void removed_table_snapshot_test() {
+   typename SNAPSHOT_SUITE::snapshot_t snap;
+   controller::config cfg;
+   {
+      TESTER chain;
+
+      chain.create_account("snapshot"_n);
+      chain.set_code("snapshot"_n, test_contracts::snapshot_test_wasm());
+      chain.set_abi("snapshot"_n, test_contracts::snapshot_test_abi());
+      chain.produce_block();
+
+      chain.push_action("snapshot"_n, "add"_n, "snapshot"_n, mutable_variant_object()("scope", "apple"_n) ("id", 1u)("payload",sha256::hash("1")));
+      chain.push_action("snapshot"_n, "add"_n, "snapshot"_n, mutable_variant_object()("scope", "apple"_n) ("id", 2u)("payload",sha256::hash("2")));
+      chain.push_action("snapshot"_n, "add"_n, "snapshot"_n, mutable_variant_object()("scope", "banana"_n)("id", 3u)("payload",sha256::hash("3")));
+      chain.push_action("snapshot"_n, "add"_n, "snapshot"_n, mutable_variant_object()("scope", "banana"_n)("id", 4u)("payload",sha256::hash("4")));
+      chain.push_action("snapshot"_n, "add"_n, "snapshot"_n, mutable_variant_object()("scope", "carrot"_n)("id", 5u)("payload",sha256::hash("5")));
+      chain.push_action("snapshot"_n, "add"_n, "snapshot"_n, mutable_variant_object()("scope", "carrot"_n)("id", 6u)("payload",sha256::hash("6")));
+
+      chain.produce_block();
+
+      chain.push_action("snapshot"_n, "remove"_n, "snapshot"_n, mutable_variant_object()("scope", "banana"_n)("id", 3u));
+      chain.push_action("snapshot"_n, "remove"_n, "snapshot"_n, mutable_variant_object()("scope", "banana"_n)("id", 4u));
+
+      chain.produce_block();
+
+      chain.control->abort_block();
+      auto writer = SNAPSHOT_SUITE::get_writer();
+      chain.control->write_snapshot(writer);
+      snap = SNAPSHOT_SUITE::finalize(writer);
+      cfg = chain.get_config();
+   }
+   {
+      snapshotted_tester chain2(cfg, SNAPSHOT_SUITE::get_reader(snap), 0);
+
+      chain2.push_action("snapshot"_n, "verify"_n, "snapshot"_n, mutable_variant_object()("scope", "carrot"_n)("id", 5u)("payload",sha256::hash("5")));
+      chain2.push_action("snapshot"_n, "verify"_n, "snapshot"_n, mutable_variant_object()("scope", "carrot"_n)("id", 6u)("payload",sha256::hash("6")));
+   }
+}
+
+BOOST_AUTO_TEST_CASE_TEMPLATE(removed_table, SNAPSHOT_SUITE, snapshot_suites) {
+   removed_table_snapshot_test<legacy_tester, SNAPSHOT_SUITE>();
+   removed_table_snapshot_test<savanna_tester, SNAPSHOT_SUITE>();
+}
+
 BOOST_AUTO_TEST_SUITE_END()

unittests/test-contracts/snapshot_test/snapshot_test.abi

@@ -3,6 +3,24 @@
     "version": "eosio::abi/1.2",
     "types": [],
     "structs": [
+        {
+            "name": "add",
+            "base": "",
+            "fields": [
+                {
+                    "name": "scope",
+                    "type": "name"
+                },
+                {
+                    "name": "id",
+                    "type": "uint64"
+                },
+                {
+                    "name": "payload",
+                    "type": "checksum256"
+                }
+            ]
+        },
         {
             "name": "increment",
             "base": "",
@@ -42,13 +60,74 @@
                     "type": "checksum256"
                 }
             ]
+        },
+        {
+            "name": "remove",
+            "base": "",
+            "fields": [
+                {
+                    "name": "scope",
+                    "type": "name"
+                },
+                {
+                    "name": "id",
+                    "type": "uint64"
+                }
+            ]
+        },
+        {
+            "name": "test_record",
+            "base": "",
+            "fields": [
+                {
+                    "name": "id",
+                    "type": "uint64"
+                },
+                {
+                    "name": "payload",
+                    "type": "checksum256"
+                }
+            ]
+        },
+        {
+            "name": "verify",
+            "base": "",
+            "fields": [
+                {
+                    "name": "scope",
+                    "type": "name"
+                },
+                {
+                    "name": "id",
+                    "type": "uint64"
+                },
+                {
+                    "name": "payload",
+                    "type": "checksum256"
+                }
+            ]
         }
     ],
     "actions": [
+        {
+            "name": "add",
+            "type": "add",
+            "ricardian_contract": ""
+        },
         {
             "name": "increment",
             "type": "increment",
             "ricardian_contract": ""
+        },
+        {
+            "name": "remove",
+            "type": "remove",
+            "ricardian_contract": ""
+        },
+        {
+            "name": "verify",
+            "type": "verify",
+            "ricardian_contract": ""
         }
     ],
     "tables": [
@@ -58,6 +137,13 @@
             "index_type": "i64",
             "key_names": [],
             "key_types": []
+        },
+        {
+            "name": "test",
+            "type": "test_record",
+            "index_type": "i64",
+            "key_names": [],
+            "key_types": []
         }
     ],
     "ricardian_clauses": [],

unittests/test-contracts/snapshot_test/snapshot_test.cpp

@@ -27,3 +27,28 @@ void snapshot_test::increment( uint32_t value ) {
       } );
    }
 }
+
+void snapshot_test::add(eosio::name scope, uint64_t id, eosio::checksum256 payload) {
+   require_auth(get_self());
+
+   test_table(get_self(), scope.value).emplace(get_self(), [&](test_record& record) {
+      record.id = id;
+      record.payload = payload;
+   });
+}
+
+void snapshot_test::remove(eosio::name scope, uint64_t id) {
+   require_auth(get_self());
+
+   test_table table(get_self(), scope.value);
+   test_table::const_iterator it = table.require_find(id);
+   table.erase(it);
+}
+
+void snapshot_test::verify(eosio::name scope, uint64_t id, eosio::checksum256 payload) {
+   require_auth(get_self());
+
+   test_table table(get_self(), scope.value);
+   test_table::const_iterator it = table.require_find(id);
+   check(it->payload == payload, "that's not right");
+}

unittests/test-contracts/snapshot_test/snapshot_test.hpp

@@ -39,4 +39,14 @@ class [[eosio::contract]] snapshot_test : public eosio::contract {
       eosio::indexed_by< "byiiii"_n, eosio::const_mem_fun< main_record, const eosio::checksum256&,
                                                            &main_record::get_index_i256 > >
    >;
+
+   struct [[eosio::table("test")]] test_record {
+      uint64_t id = 0;
+      eosio::checksum256 payload;
+      uint64_t primary_key() const {return id;}
+   };
+   using test_table = eosio::multi_index<"test"_n, test_record>;
+   [[eosio::action]] void add(eosio::name scope, uint64_t id, eosio::checksum256 payload);
+   [[eosio::action]] void remove(eosio::name scope, uint64_t id);
+   [[eosio::action]] void verify(eosio::name scope, uint64_t id, eosio::checksum256 payload);
 };