Comments, use iterator bool() and avoid deadlock iterations.

libbitcoin · Jun 11, 2024 · e35091a · e35091a
1 parent 0c88f8b
commit e35091a
Show file tree

Hide file tree

Showing 7 changed files with 44 additions and 32 deletions.
diff --git a/include/bitcoin/database/impl/primitives/hashmap.ipp b/include/bitcoin/database/impl/primitives/hashmap.ipp
@@ -150,6 +150,7 @@ Link CLASS::first(const Key& key) const NOEXCEPT
 TEMPLATE
 typename CLASS::iterator CLASS::it(const Key& key) const NOEXCEPT
 {
+    // Expensive (27%), avoid construction for use with first().
     // key is passed and retained by reference, origin must remain in scope.
     return { manager_.get(), head_.top(key), key };
 }

diff --git a/include/bitcoin/database/impl/query/archive.ipp b/include/bitcoin/database/impl/query/archive.ipp
@@ -87,14 +87,14 @@ inline bool CLASS::is_malleated64(const block& block) const NOEXCEPT
         return false;
 
     // Pass l-value to iterator.
+    const auto transactions = *block.transactions_ptr();
     const auto link = to_header(block.hash());
     auto it = store_.txs.it(link);
-    const auto transactions = *block.transactions_ptr();
     do
     {
         // Non-malleable is final so don't continue with that type association.
         table::txs::slab txs{};
-        if (!store_.txs.get(it.self(), txs) || !txs.malleable)
+        if (!store_.txs.get(it, txs) || !txs.malleable)
             return false;
 
         if (txs.tx_fks.size() != transactions.size())

diff --git a/include/bitcoin/database/impl/query/confirm.ipp b/include/bitcoin/database/impl/query/confirm.ipp
@@ -236,8 +236,9 @@ TEMPLATE
 inline error::error_t CLASS::spent_prevout(const foreign_point& point,
     const tx_link& self) const NOEXCEPT
 {
+    // Expensive (6%).
     auto it = store_.spend.it(point);
-    if (it.self().is_terminal())
+    if (!it)
         return error::success;
 
     table::spend::get_parent spend{};
@@ -254,6 +255,7 @@ inline error::error_t CLASS::spent_prevout(const foreign_point& point,
         if (!to_block(spend.parent_fk).is_terminal())
             return error::confirmed_double_spend;
     }
+    // Expensive (37%).
     while (it.advance());
     return error::success;
 }
@@ -263,7 +265,12 @@ TEMPLATE
 inline error::error_t CLASS::unspendable_prevout(const point_link& link,
     uint32_t sequence, uint32_t version, const context& ctx) const NOEXCEPT
 {
-    const auto strong = to_strong(get_point_key(link));
+    // Modest (1.24%), and with 4.77 conflict ratio.
+    const auto key = get_point_key(link);
+
+    // Expensize (21.31%).
+    const auto strong = to_strong(key);
+
     if (strong.block.is_terminal())
         return strong.tx.is_terminal() ? error::missing_previous_output :
             error::unconfirmed_spend;

diff --git a/include/bitcoin/database/impl/query/optional.ipp b/include/bitcoin/database/impl/query/optional.ipp
@@ -36,14 +36,14 @@ bool CLASS::get_confirmed_balance(uint64_t& out,
     const hash_digest& key) const NOEXCEPT
 {
     auto it = store_.address.it(key);
-    if (it.self().is_terminal())
+    if (!it)
         return false;
 
     out = zero;
     do
     {
         table::address::record address{};
-        if (!store_.address.get(it.self(), address))
+        if (!store_.address.get(it, address))
             return false;
 
         // Failure or overflow returns maximum value.
@@ -66,14 +66,14 @@ bool CLASS::to_address_outputs(output_links& out,
     const hash_digest& key) const NOEXCEPT
 {
     auto it = store_.address.it(key);
-    if (it.self().is_terminal())
+    if (!it)
         return false;
 
     out.clear();
     do
     {
         table::address::record address{};
-        if (!store_.address.get(it.self(), address))
+        if (!store_.address.get(it, address))
         {
             out.clear();
             return false;
@@ -91,14 +91,14 @@ bool CLASS::to_unspent_outputs(output_links& out,
     const hash_digest& key) const NOEXCEPT
 {
     auto it = store_.address.it(key);
-    if (it.self().is_terminal())
-        return {};
+    if (!it)
+        return false;
 
     out.clear();
     do
     {
         table::address::record address{};
-        if (!store_.address.get(it.self(), address))
+        if (!store_.address.get(it, address))
         {
             out.clear();
             return false;
@@ -117,15 +117,15 @@ bool CLASS::to_minimum_unspent_outputs(output_links& out,
     const hash_digest& key, uint64_t minimum) const NOEXCEPT
 {
     auto it = store_.address.it(key);
-    if (it.self().is_terminal())
-        return {};
+    if (!it)
+        return false;
 
     out.clear();
     do
     {
         table::address::record address{};
-        if (!store_.address.get(it.self(), address))
-            return {};
+        if (!store_.address.get(it, address))
+            return false;
 
         // Confirmed and not spent, but possibly immature.
         if (is_confirmed_output(address.output_fk) &&

diff --git a/include/bitcoin/database/impl/query/translate.ipp b/include/bitcoin/database/impl/query/translate.ipp
@@ -197,6 +197,8 @@ TEMPLATE
 header_link CLASS::to_block(const tx_link& link) const NOEXCEPT
 {
     table::strong_tx::record strong{};
+
+    // Expensive (8%).
     if (!store_.strong_tx.get(store_.strong_tx.first(link), strong))
         return {};
 
@@ -215,9 +217,9 @@ inline strong_pair CLASS::to_strong(const hash_digest& tx_hash) const NOEXCEPT
     strong_pair strong{ {}, it.self() };
     do
     {
-        const auto link = to_block(it.self());
-        if (!link.is_terminal())
-            return { link, it.self() };
+        strong.block = to_block((strong.tx = it.self()));
+        if (!strong.block.is_terminal())
+            return strong;
     }
     while (it.advance());
     return strong;
@@ -260,8 +262,7 @@ inline header_links CLASS::to_blocks(const tx_link& link) const NOEXCEPT
     block_txs strongs{};
     do
     {
-        if (store_.strong_tx.get(it.self(), strong) &&
-            !contains(strongs, strong))
+        if (store_.strong_tx.get(it, strong) && !contains(strongs, strong))
             strongs.push_back(strong);
     }
     while(it.advance());
@@ -370,13 +371,14 @@ TEMPLATE
 spend_links CLASS::to_spenders(const foreign_point& point) const NOEXCEPT
 {
     auto it = store_.spend.it(point);
-    if (it.self().is_terminal())
+    if (!it)
         return {};
 
     // Iterate transactions that spend the point, saving each spender.
     spend_links spenders{};
     do
-    {
+    {   // BUGBUG: Deadlock due to holding iterator while querying own table.
+        // TODO: refactor to make safe and also pass boolean result code.
         spenders.push_back(to_spender(to_spend_tx(it.self()), point));
     }
     while (it.advance());

diff --git a/include/bitcoin/database/impl/query/validate.ipp b/include/bitcoin/database/impl/query/validate.ipp
@@ -218,13 +218,13 @@ code CLASS::get_tx_state(const tx_link& link,
     const context& ctx) const NOEXCEPT
 {
     auto it = store_.validated_tx.it(link);
-    if (it.self().is_terminal())
+    if (!it)
         return error::unvalidated;
 
     table::validated_tx::slab_get_code valid{};
     do
     {
-        if (!store_.validated_tx.get(it.self(), valid))
+        if (!store_.validated_tx.get(it, valid))
             return error::integrity;
 
         if (is_sufficient(ctx, valid.ctx))
@@ -239,13 +239,13 @@ code CLASS::get_tx_state(uint64_t& fee, size_t& sigops, const tx_link& link,
     const context& ctx) const NOEXCEPT
 {
     auto it = store_.validated_tx.it(link);
-    if (it.self().is_terminal())
+    if (!it)
         return error::unvalidated;
 
     table::validated_tx::slab valid{};
     do
     {
-        if (!store_.validated_tx.get(it.self(), valid))
+        if (!store_.validated_tx.get(it, valid))
             return error::integrity;
 
         if (is_sufficient(ctx, valid.ctx))

diff --git a/test/primitives/iterator.cpp b/test/primitives/iterator.cpp
@@ -33,6 +33,7 @@ BOOST_AUTO_TEST_CASE(iterator__self__empty__terminal)
     test::chunk_storage file;
     const slab_iterate iterator{ file.get(), start, key0 };
     BOOST_REQUIRE(iterator.self().is_terminal());
+    BOOST_REQUIRE(!iterator);
 }
 
 BOOST_AUTO_TEST_CASE(iterator__self__overflow__terminal)
@@ -51,6 +52,7 @@ BOOST_AUTO_TEST_CASE(iterator__self__overflow__terminal)
     test::chunk_storage file{ data };
     const slab_iterate iterator{ file.get(), start, key0 };
     BOOST_REQUIRE(iterator.self().is_terminal());
+    BOOST_REQUIRE(!iterator);
 }
 
 BOOST_AUTO_TEST_CASE(iterator__advance__record__expected)
@@ -68,13 +70,13 @@ BOOST_AUTO_TEST_CASE(iterator__advance__record__expected)
     };
     test::chunk_storage file{ data };
     record_iterate iterator{ file.get(), start, key2 };
-    BOOST_REQUIRE(!iterator.self().is_terminal());
+    BOOST_REQUIRE(iterator);
     BOOST_REQUIRE_EQUAL(iterator.self(), 0x00u);
     BOOST_REQUIRE(iterator.advance());
-    BOOST_REQUIRE(!iterator.self().is_terminal());
+    BOOST_REQUIRE(iterator);
     BOOST_REQUIRE_EQUAL(iterator.self(), 0x01u);
     BOOST_REQUIRE(!iterator.advance());
-    BOOST_REQUIRE(iterator.self().is_terminal());
+    BOOST_REQUIRE(!iterator);
     BOOST_REQUIRE_EQUAL(iterator.self(), link::terminal);
 }
 
@@ -94,13 +96,13 @@ BOOST_AUTO_TEST_CASE(iterator__advance__slab__expected)
     };
     test::chunk_storage file{ data };
     slab_iterate iterator{ file.get(), start, key2 };
-    BOOST_REQUIRE(!iterator.self().is_terminal());
+    BOOST_REQUIRE(iterator);
     BOOST_REQUIRE_EQUAL(iterator.self(), 0x00u);
     BOOST_REQUIRE(iterator.advance());
-    BOOST_REQUIRE(!iterator.self().is_terminal());
+    BOOST_REQUIRE(iterator);
     BOOST_REQUIRE_EQUAL(iterator.self(), 0x03u);
     BOOST_REQUIRE(!iterator.advance());
-    BOOST_REQUIRE(iterator.self().is_terminal());
+    BOOST_REQUIRE(!iterator);
     BOOST_REQUIRE_EQUAL(iterator.self(), link::terminal);
 }