Fix several bugs: (#119)

* lws::account height update should only go up.
  * Webhook confirmations can start after first new block
  * Webhook confirmations could face a rescan

src/db/account.cpp

@@ -165,7 +165,7 @@ namespace lws
 
   void account::updated(db::block_id new_height) noexcept
   {
-    height_ = new_height;
+    height_ = std::max(height_, new_height);
     spends_.clear();
     spends_.shrink_to_fit();
     outputs_.clear();

src/db/account.h

@@ -88,7 +88,7 @@ namespace lws
     //! \return A copy of `this`.
     account clone() const;
 
-    //! \return A copy of `this` with a new height and `outputs().empty()`.
+    //! \post `height() == max(new_height, height())`, `outputs().empty()`, and `spends.empty()`.
     void updated(db::block_id new_height) noexcept;
 
     //! \return Unique ID from the account database, possibly `db::account_id::kInvalid`.

src/db/storage.cpp

@@ -2537,38 +2537,42 @@ namespace db
         const webhook_event event =
           MONERO_UNWRAP(events_by_account_id.get_value<webhook_event>(value));
 
-        MDB_val rkey = lmdb::to_val(hook_key);
-        MDB_val rvalue = lmdb::to_val(event.link_webhook);
-        MONERO_LMDB_CHECK(mdb_cursor_get(&webhooks_cur, &rkey, &rvalue, MDB_GET_BOTH));
-
-        MDB_val okey = lmdb::to_val(user);
-        MDB_val ovalue = lmdb::to_val(event.link);
-        MONERO_LMDB_CHECK(mdb_cursor_get(&outputs_cur, &okey, &ovalue, MDB_GET_BOTH));
-
-        events.push_back(
-          webhook_tx_confirmation{
-            MONERO_UNWRAP(webhooks.get_key(rkey)),
-            MONERO_UNWRAP(webhooks.get_value(rvalue)),
-            MONERO_UNWRAP(outputs.get_value<output>(ovalue))
-          }
-        );
-
-        const std::uint32_t requested_confirmations =
-          events.back().value.second.confirmations;
-
-        events.back().value.second.confirmations =
-          lmdb::to_native(begin) - lmdb::to_native(event.link.tx.height) + 1;
-
-        // copy next blocks from first
-        for (const auto block_num : boost::counting_range(lmdb::to_native(begin) + 1, lmdb::to_native(end)))
+        const block_id this_begin = std::max(begin, event.link.tx.height);
+        if (this_begin < end)
         {
+          MDB_val rkey = lmdb::to_val(hook_key);
+          MDB_val rvalue = lmdb::to_val(event.link_webhook);
+          MONERO_LMDB_CHECK(mdb_cursor_get(&webhooks_cur, &rkey, &rvalue, MDB_GET_BOTH));
+
+          MDB_val okey = lmdb::to_val(user);
+          MDB_val ovalue = lmdb::to_val(event.link);
+          MONERO_LMDB_CHECK(mdb_cursor_get(&outputs_cur, &okey, &ovalue, MDB_GET_BOTH));
+
+          events.push_back(
+            webhook_tx_confirmation{
+              MONERO_UNWRAP(webhooks.get_key(rkey)),
+              MONERO_UNWRAP(webhooks.get_value(rvalue)),
+              MONERO_UNWRAP(outputs.get_value<output>(ovalue))
+            }
+          );
+
+          const std::uint32_t requested_confirmations =
+            events.back().value.second.confirmations;
+
+          events.back().value.second.confirmations =
+            lmdb::to_native(this_begin) - lmdb::to_native(event.link.tx.height) + 1;
+
+          // copy next blocks from first
+          for (const auto block_num : boost::counting_range(lmdb::to_native(this_begin) + 1, lmdb::to_native(end)))
+          {
+            if (requested_confirmations <= events.back().value.second.confirmations)
+              break;
+            events.push_back(events.back());
+            ++(events.back().value.second.confirmations);
+          }
           if (requested_confirmations <= events.back().value.second.confirmations)
-            break;
-          events.push_back(events.back());
-          ++(events.back().value.second.confirmations);
-	      }
-        if (requested_confirmations <= events.back().value.second.confirmations)
-          MONERO_LMDB_CHECK(mdb_cursor_del(&events_cur, 0));
+            MONERO_LMDB_CHECK(mdb_cursor_del(&events_cur, 0));
+        }
         err = mdb_cursor_get(&events_cur, &key, &value, MDB_NEXT_DUP);
       }
       return success();
@@ -2777,7 +2781,7 @@ namespace db
 
         const block_id existing_height = existing->scan_height;
 
-        existing->scan_height = block_id(last_update);
+        existing->scan_height = std::max(existing_height, block_id(last_update));
         value = lmdb::to_val(*existing);
         MONERO_LMDB_CHECK(mdb_cursor_put(accounts_cur.get(), &key, &value, MDB_CURRENT));
 

tests/unit/db/chain.test.cpp

@@ -130,6 +130,19 @@ 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);
     }
+
+    SECTION("Old Blocks dont rollback height")
+    {
+      lws::account accounts[1] {lws::account{get_account(), {}, {}}};
+      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));
+      EXPECT(get_account().scan_height == new_block);
+
+      accounts[0].updated(old_block);
+      EXPECT(accounts[0].scan_height() == new_block);
+    }
   }
 }
 

tests/unit/db/webhook.test.cpp

@@ -157,7 +157,7 @@ LWS_CASE("db::storage::*_webhook")
       EXPECT(outs.size() == 1);
 
       lws::db::block_info head = last_block;
-      for (unsigned i = 0; i < 1; ++i)
+      for (unsigned i = 0; i < 4; ++i)
       {
         crypto::hash chain[2] = {head.hash, crypto::rand<crypto::hash>()};
 
@@ -184,24 +184,26 @@ LWS_CASE("db::storage::*_webhook")
         else
           EXPECT(updated->confirm_pubs.empty());
 
-        full_account.updated(head.id);
-        head = {lws::db::block_id(lmdb::to_native(head.id) + 1), chain[1]};
+        const auto next = lws::db::block_id(lmdb::to_native(head.id) + 1);
+        full_account.updated(next);
+        head = {next, chain[1]};
       }
     }
 
     SECTION("storage::update(...) all at once")
     {
-      const crypto::hash chain[5] = {
+      const crypto::hash chain[6] = {
         last_block.hash,
         crypto::rand<crypto::hash>(),
         crypto::rand<crypto::hash>(),
         crypto::rand<crypto::hash>(),
+        crypto::rand<crypto::hash>(),
         crypto::rand<crypto::hash>()
       };
 
       lws::account full_account = lws::db::test::make_account(account, view);
       full_account.updated(last_block.id);
-      EXPECT(add_out(full_account, last_block.id, 500));
+      EXPECT(add_out(full_account, lws::db::block_id(lmdb::to_native(last_block.id) + 1), 500));
 
       const std::vector<lws::db::output> outs = full_account.outputs();
       EXPECT(outs.size() == 1);
@@ -228,6 +230,61 @@ LWS_CASE("db::storage::*_webhook")
         EXPECT(updated->confirm_pubs[i].tx_info.payment_id.short_ == outs[0].payment_id.short_);
       }
     }
+
+    SECTION("rescan with existing event")
+    {
+      crypto::hash chain[2] = {
+        last_block.hash,
+        crypto::rand<crypto::hash>()
+      };
+
+      lws::account full_account = lws::db::test::make_account(account, view);
+      full_account.updated(last_block.id);
+      EXPECT(add_out(full_account, last_block.id, 500));
+
+      const std::vector<lws::db::output> outs = full_account.outputs();
+      EXPECT(outs.size() == 1);
+
+      auto updated = db.update(last_block.id, chain, {std::addressof(full_account), 1}, nullptr);
+      EXPECT(updated.has_value());
+      EXPECT(updated->spend_pubs.empty());
+      EXPECT(updated->accounts_updated == 1);
+      EXPECT(updated->confirm_pubs.size() == 1);
+
+      EXPECT(updated->confirm_pubs[0].key.user == lws::db::account_id(1));
+      EXPECT(updated->confirm_pubs[0].key.type == lws::db::webhook_type::tx_confirmation);
+      EXPECT(updated->confirm_pubs[0].value.first.payment_id == 500);
+      EXPECT(updated->confirm_pubs[0].value.first.event_id == id);
+      EXPECT(updated->confirm_pubs[0].value.second.url == "http://the_url");
+      EXPECT(updated->confirm_pubs[0].value.second.token == "the_token");
+      EXPECT(updated->confirm_pubs[0].value.second.confirmations == 1);
+
+      EXPECT(updated->confirm_pubs[0].tx_info.link == outs[0].link);
+      EXPECT(updated->confirm_pubs[0].tx_info.spend_meta.id == outs[0].spend_meta.id);
+      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 
+      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);
+
+      full_account.updated(new_height);
+      EXPECT(full_account.scan_height() == last_block.id);
+
+      full_account = lws::db::test::make_account(account, view);
+      full_account.updated(new_height);
+      EXPECT(full_account.scan_height() == new_height);
+      updated = db.update(new_height, chain, {std::addressof(full_account), 1}, nullptr);
+      EXPECT(updated.has_value());
+      EXPECT(updated->spend_pubs.empty());
+      EXPECT(updated->accounts_updated == 1);
+      EXPECT(updated->confirm_pubs.size() == 0);
+    }
+
     SECTION("Add db spend")
     {
       const boost::uuids::uuid other_id = boost::uuids::random_generator{}();

tests/unit/scanner.test.cpp

@@ -194,7 +194,7 @@ namespace
     transaction out{};
     EXPECT(
       cryptonote::construct_tx_and_get_tx_key(
-        keys, subaddresses, sources, destinations, keys.m_account_address, {}, out.tx, 0, unused_key,
+        keys, subaddresses, sources, destinations, keys.m_account_address, {}, out.tx, /* 0, */ unused_key,
         out.additional_keys, true, {rct::RangeProofType::RangeProofBulletproof, 2}, use_view_tag
       )
     );