From e35091ac57e47c8298263f4f3e371b5d474e4822 Mon Sep 17 00:00:00 2001 From: evoskuil Date: Mon, 10 Jun 2024 20:49:23 -0400 Subject: [PATCH] Comments, use iterator bool() and avoid deadlock iterations. --- .../database/impl/primitives/hashmap.ipp | 1 + .../bitcoin/database/impl/query/archive.ipp | 4 ++-- .../bitcoin/database/impl/query/confirm.ipp | 11 ++++++++-- .../bitcoin/database/impl/query/optional.ipp | 22 +++++++++---------- .../bitcoin/database/impl/query/translate.ipp | 16 ++++++++------ .../bitcoin/database/impl/query/validate.ipp | 8 +++---- test/primitives/iterator.cpp | 14 +++++++----- 7 files changed, 44 insertions(+), 32 deletions(-) diff --git a/include/bitcoin/database/impl/primitives/hashmap.ipp b/include/bitcoin/database/impl/primitives/hashmap.ipp index 5f667083..efce745d 100644 --- 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 index 8f4cae01..139abd24 100644 --- 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 index 938f1c2d..47227266 100644 --- 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 index 248689de..65fdbfde 100644 --- 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 index 29cf1667..08b823da 100644 --- 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 index b58f47d1..fd923d25 100644 --- 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 index a0d935cc..08c1286d 100644 --- 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); }