Fix scan height (db) bug on account check-ins (#120)

src/db/storage.cpp

@@ -859,6 +859,41 @@ namespace db
     return out;
   }
 
+  expect<std::vector<account_id>> storage_reader::accounts_at_height(block_id height)
+  {
+    MONERO_PRECOND(txn != nullptr);
+    assert(db != nullptr);
+
+    MONERO_CHECK(check_cursor(*txn, db->tables.accounts_bh, curs.accounts_bh_cur));
+
+    std::vector<account_id> out{};
+
+    MDB_val key = lmdb::to_val(height);
+    MDB_val value{};
+    int err = mdb_cursor_get(curs.accounts_bh_cur.get(), &key, &value, MDB_SET_KEY);
+    {
+      std::size_t count = 0;
+      MONERO_LMDB_CHECK(mdb_cursor_count(curs.accounts_bh_cur.get(), &count));
+      out.reserve(count);
+    }
+    for (;;)
+    {
+      if (err)
+      {
+        if (err == MDB_NOTFOUND)
+          break;
+        return {lmdb::error(err)};
+      }
+
+      out.push_back(
+        MONERO_UNWRAP(accounts_by_height.get_value<MONERO_FIELD(account_lookup, id)>(value))
+      );
+      err = mdb_cursor_get(curs.accounts_bh_cur.get(), &key, &value, MDB_NEXT_DUP);
+    }
+
+    return out;
+  }
+
   expect<lmdb::key_stream<account_status, account, cursor::close_accounts>>
   storage_reader::get_accounts(cursor::accounts cur) noexcept
   {
@@ -2779,9 +2814,14 @@ namespace db
         if (!existing || existing->scan_height != user->scan_height())
           continue; // to next account
 
+        // Don't re-store data if already scanned
+        ++out.accounts_updated;
+        if (block_id(last_update) <= existing->scan_height)
+          continue; // to next account
+
         const block_id existing_height = existing->scan_height;
 
-        existing->scan_height = std::max(existing_height, block_id(last_update));
+        existing->scan_height = block_id(last_update);
         value = lmdb::to_val(*existing);
         MONERO_LMDB_CHECK(mdb_cursor_put(accounts_cur.get(), &key, &value, MDB_CURRENT));
 
@@ -2802,8 +2842,6 @@ namespace db
           )
         );
         MONERO_CHECK(check_spends(out.spend_pubs, *webhooks_cur, *outputs_cur, *user));
-
-        ++out.accounts_updated;
       } // ... for every account being updated ...
       return {std::move(out)};
     });

src/db/storage.h

@@ -118,6 +118,9 @@ namespace db
     //! \return Objects for use with cryptonote::next_difficulty and median timestamp check
     expect<pow_window> get_pow_window(block_id last);
 
+    //! \return Accounts at height (possible empty).
+    expect<std::vector<account_id>> accounts_at_height(block_id height);
+
     //! \return All registered `account`s.
     expect<lmdb::key_stream<account_status, account, cursor::close_accounts>>
       get_accounts(cursor::accounts cur = nullptr) noexcept;

tests/unit/db/chain.test.cpp

@@ -73,7 +73,13 @@ LWS_CASE("db::storage::sync_chain")
     {
       return MONERO_UNWRAP(MONERO_UNWRAP(db.start_read()).get_account(account)).second;
     };
+
+    const auto get_height = [&db] (const lws::db::block_id height) -> std::vector<lws::db::account_id>
+    {
+      return MONERO_UNWRAP(MONERO_UNWRAP(db.start_read()).accounts_at_height(height));
+    };
 
+    const auto scan_height = lws::db::block_id(lmdb::to_native(last_block.id) + 4);
     const crypto::hash chain[5] = {
       last_block.hash,
       crypto::rand<crypto::hash>(),
@@ -90,17 +96,27 @@ LWS_CASE("db::storage::sync_chain")
     {
       const lws::account accounts[1] = {lws::account{get_account(), {}, {}}};
       EXPECT(accounts[0].scan_height() == last_block.id);
-      EXPECT(db.update(last_block.id, chain, accounts, nullptr));
-      EXPECT(get_account().scan_height == lws::db::block_id(std::uint64_t(last_block.id) + 4));
+      const auto updated = db.update(last_block.id, chain, accounts, nullptr);
+      EXPECT(updated);
+      EXPECT(updated->spend_pubs.empty());
+      EXPECT(updated->confirm_pubs.empty());
+      EXPECT(updated->accounts_updated == 1);
+      EXPECT(get_account().scan_height == scan_height);
+
+      const auto height = get_height(scan_height);
+      EXPECT(height.size() == 1);
+      EXPECT(height[0] == lws::db::account_id(1));
     }
 
     SECTION("Verify Append")
     {
       lws_test::test_chain(lest_env, MONERO_UNWRAP(db.start_read()), last_block.id, chain);
+      EXPECT(get_height(lws::db::block_id(0)).empty());
     }
 
     SECTION("Fork Chain")
     {
+      const auto fork_height = lws::db::block_id(lmdb::to_native(last_block.id) + 1);
       const crypto::hash fchain[5] = {
         chain[0],
         chain[1],
@@ -111,7 +127,11 @@ LWS_CASE("db::storage::sync_chain")
 
       EXPECT(db.sync_chain(last_block.id, fchain));
       lws_test::test_chain(lest_env, MONERO_UNWRAP(db.start_read()), last_block.id, fchain);
-      EXPECT(get_account().scan_height == lws::db::block_id(std::uint64_t(last_block.id) + 1));
+      EXPECT(get_account().scan_height == fork_height);
+
+      const auto height = get_height(fork_height);
+      EXPECT(height.size() == 1);
+      EXPECT(height[0] == lws::db::account_id(1));
     }
 
     SECTION("Fork past checkpoint")
@@ -129,6 +149,11 @@ LWS_CASE("db::storage::sync_chain")
 
       const auto sync_result = db.sync_chain(lws::db::block_id(point->first), fchain);
       EXPECT(sync_result == lws::error::bad_blockchain);
+      EXPECT(get_account().scan_height == scan_height);
+
+      const auto height = get_height(scan_height);
+      EXPECT(height.size() == 1);
+      EXPECT(height[0] == lws::db::account_id(1));
     }
 
     SECTION("Old Blocks dont rollback height")
@@ -137,11 +162,19 @@ LWS_CASE("db::storage::sync_chain")
       const auto old_block = lws::db::block_id(lmdb::to_native(last_block.id) - 5);
       const auto new_block = lws::db::block_id(lmdb::to_native(last_block.id) + 4);
       EXPECT(accounts[0].scan_height() == new_block);
-      EXPECT(db.update(old_block, chain, accounts, nullptr));
+      const auto updated = db.update(old_block, chain, accounts, nullptr);
+      EXPECT(updated);
+      EXPECT(updated->spend_pubs.empty());
+      EXPECT(updated->confirm_pubs.empty());
+      EXPECT(updated->accounts_updated == 1);
       EXPECT(get_account().scan_height == new_block);
 
+      const auto height = get_height(scan_height);
+      EXPECT(height.size() == 1);
+      EXPECT(height[0] == lws::db::account_id(1));
+
       accounts[0].updated(old_block);
-      EXPECT(accounts[0].scan_height() == new_block);
+      EXPECT(accounts[0].scan_height() == scan_height);
     }
   }
 }

tests/unit/db/webhook.test.cpp

@@ -101,6 +101,11 @@ LWS_CASE("db::storage::*_webhook")
       MONERO_UNWRAP(MONERO_UNWRAP(db.start_read()).get_last_block());
     MONERO_UNWRAP(db.add_account(account, view));
 
+    const auto get_height = [&db] (const lws::db::block_id height) -> std::vector<lws::db::account_id>
+    {
+      return MONERO_UNWRAP(MONERO_UNWRAP(db.start_read()).accounts_at_height(height));
+    };
+
     const boost::uuids::uuid id = boost::uuids::random_generator{}();
     {
       lws::db::webhook_value value{
@@ -233,6 +238,7 @@ LWS_CASE("db::storage::*_webhook")
 
     SECTION("rescan with existing event")
     {
+      const auto scan_height = lws::db::block_id(lmdb::to_native(last_block.id) + 1);
       crypto::hash chain[2] = {
         last_block.hash,
         crypto::rand<crypto::hash>()
@@ -264,16 +270,39 @@ LWS_CASE("db::storage::*_webhook")
       EXPECT(updated->confirm_pubs[0].tx_info.pub == outs[0].pub);
       EXPECT(updated->confirm_pubs[0].tx_info.payment_id.short_ == outs[0].payment_id.short_);
 
-      // issue a rescan, and ensure that 
+      full_account.updated(scan_height);
+      EXPECT(full_account.scan_height() == scan_height);
+      EXPECT(get_height(last_block.id).empty());
+      auto height = get_height(scan_height);
+      EXPECT(height.size() == 1);
+      EXPECT(height[0] == lws::db::account_id(1));
+
+      updated = db.update(last_block.id, chain, {std::addressof(full_account), 1}, nullptr);
+      EXPECT(updated.has_value());
+      EXPECT(updated->spend_pubs.empty());
+      EXPECT(updated->confirm_pubs.empty());
+      EXPECT(updated->accounts_updated == 1);
+      EXPECT(get_height(last_block.id).empty());
+      height = get_height(scan_height);
+      EXPECT(height.size() == 1);
+      EXPECT(height[0] == lws::db::account_id(1));
+
+      // issue a rescan, and ensure that hooks are not triggered
       const std::size_t chain_size = std::end(chain) - std::begin(chain);
       const auto new_height = lws::db::block_id(lmdb::to_native(last_block.id) - chain_size);
       const auto rescanned =
         db.rescan(new_height, {std::addressof(full_account.db_address()), 1});
       EXPECT(rescanned.has_value());
       EXPECT(rescanned->size() == 1);
 
+      EXPECT(get_height(last_block.id).empty());
+      EXPECT(get_height(scan_height).empty());
+      height = get_height(new_height);
+      EXPECT(height.size() == 1);
+      EXPECT(height[0] == lws::db::account_id(1));
+
       full_account.updated(new_height);
-      EXPECT(full_account.scan_height() == last_block.id);
+      EXPECT(full_account.scan_height() == scan_height);
 
       full_account = lws::db::test::make_account(account, view);
       full_account.updated(new_height);
@@ -283,6 +312,13 @@ LWS_CASE("db::storage::*_webhook")
       EXPECT(updated->spend_pubs.empty());
       EXPECT(updated->accounts_updated == 1);
       EXPECT(updated->confirm_pubs.size() == 0);
+
+      EXPECT(get_height(last_block.id).empty());
+      EXPECT(get_height(scan_height).empty());
+      EXPECT(get_height(new_height).empty());
+      height = get_height(lws::db::block_id(lmdb::to_native(new_height) + 1));
+      EXPECT(height.size() == 1);
+      EXPECT(height[0] == lws::db::account_id(1));
     }
 
     SECTION("Add db spend")