Merge pull request #772 from evoskuil/master

Narrow scope of reorganization lock to just get fork.

Author: evoskuil

include/bitcoin/node/chasers/chaser.hpp

@@ -94,7 +94,7 @@ class BCN_API chaser
     virtual code reload(const store::event_handler& handler) NOEXCEPT;
 
     /// Get reorganization lock.
-    virtual lock get_reorganization_lock() NOEXCEPT;
+    virtual lock get_reorganization_lock() const NOEXCEPT;
 
     /// Events.
     /// -----------------------------------------------------------------------

include/bitcoin/node/chasers/chaser_confirm.hpp

@@ -70,9 +70,10 @@ class BCN_API chaser_confirm
         size_t top) NOEXCEPT;
 
     // getters
-    bool get_fork_work(uint256_t& fork_work, header_links& fork,
-        height_t fork_top) const NOEXCEPT;
-    bool get_is_strong(bool& strong, const uint256_t& fork_work,
+    header_links get_fork(height_t fork_top) const NOEXCEPT;
+    bool get_work(uint256_t& fork_work,
+        const header_links& fork) const NOEXCEPT;
+    bool get_strong(bool& strong, const uint256_t& fork_work,
         size_t fork_point) const NOEXCEPT;
 };
 

include/bitcoin/node/full_node.hpp

@@ -141,7 +141,7 @@ class BCN_API full_node
 
     /// Get reorganization lock.
     /// Used to prevent candidate chain reorganization during confirmation.
-    virtual lock get_reorganization_lock() NOEXCEPT;
+    virtual lock get_reorganization_lock() const NOEXCEPT;
 
 protected:
     /// Session attachments.
@@ -164,8 +164,8 @@ class BCN_API full_node
         event_value value) NOEXCEPT;
 
     // These are thread safe.
+    mutable std::mutex reorganization_mutex_{};
     const configuration& config_;
-    std::mutex reorganization_mutex_{};
     memory_controller memory_;
     query& query_;
 

src/chasers/chaser.cpp

@@ -83,7 +83,7 @@ code chaser::reload(const store::event_handler& handler) NOEXCEPT
     return node_.reload(handler);
 }
 
-lock chaser::get_reorganization_lock() NOEXCEPT
+lock chaser::get_reorganization_lock() const NOEXCEPT
 {
     return node_.get_reorganization_lock();
 }

src/chasers/chaser_confirm.cpp

@@ -159,25 +159,21 @@ void chaser_confirm::do_bumped(height_t height) NOEXCEPT
     if (closed())
         return;
 
-    // Prevents organizer from popping candidates (does not block push).
-    get_reorganization_lock();
+    // If empty height is not on a candidate fork (may have been reorganized).
+    auto fork = get_fork(height);
+    if (fork.empty())
+        return;
 
-    // Scan from candidate height to first confirmed, return links and work.
     uint256_t work{};
-    header_links fork{};
-    if (!get_fork_work(work, fork, height))
+    if (!get_work(work, fork))
     {
         fault(error::confirm1);
         return;
     }
 
-    // No longer a candidate fork (may have been reorganized).
-    if (fork.empty())
-        return;
-
     bool strong{};
     const auto fork_point = height - fork.size();
-    if (!get_is_strong(strong, work, fork_point))
+    if (!get_strong(strong, work, fork_point))
     {
         fault(error::confirm2);
         return;
@@ -427,42 +423,56 @@ bool chaser_confirm::roll_back(const header_links& popped, size_t fork_point,
 
 // Private getters
 // ----------------------------------------------------------------------------
-// These are subject to intervening/concurrent candidate chain reorganization.
 
-bool chaser_confirm::get_fork_work(uint256_t& fork_work, header_links& fork,
+// TODO: move into database library with internal lock.
+chaser_confirm::header_links chaser_confirm::get_fork(
     height_t fork_top) const NOEXCEPT
 {
     BC_ASSERT(stranded());
     const auto& query = archive();
     header_link link{};
-    fork_work = zero;
-    fork.clear();
+    header_links out{};
+
+    // Prevents organizer from popping candidates (does not block push).
+    get_reorganization_lock();
 
     // Walk down candidates from fork_top to fork point (highest common).
-    for (link = query.to_candidate(fork_top); !link.is_terminal() &&
-        !query.is_confirmed_block(link); link = query.to_candidate(--fork_top))
+    for (link = query.to_candidate(fork_top);
+        !link.is_terminal() && !query.is_confirmed_block(link);
+         link = query.to_candidate(--fork_top))
     {
-        uint32_t bits{};
-        if (!query.get_bits(bits, link))
-            return false;
-
-        fork_work += chain::header::proof(bits);
-        fork.push_back(link);
+        out.push_back(link);
     }
 
     // Terminal candidate from previously valid height implies regression.
     // This is ok, it just means that the fork is no longer a candidate.
     if (link.is_terminal())
+        out.clear();
+
+    return out;
+}
+
+bool chaser_confirm::get_work(uint256_t& fork_work,
+    const header_links& fork) const NOEXCEPT
+{
+    BC_ASSERT(stranded());
+    const auto& query = archive();
+
+    // Walk down candidates from fork_top to fork point (highest common).
+    for (const auto& link: fork)
     {
-        fork_work = zero;
-        fork.clear();
+        uint32_t bits{};
+        if (!query.get_bits(bits, link))
+            return false;
+
+        fork_work += chain::header::proof(bits);
     }
 
     return true;
 }
 
 // A fork with greater work will cause confirmed reorganization.
-bool chaser_confirm::get_is_strong(bool& strong, const uint256_t& fork_work,
+bool chaser_confirm::get_strong(bool& strong, const uint256_t& fork_work,
     size_t fork_point) const NOEXCEPT
 {
     BC_ASSERT(stranded());

src/full_node.cpp

@@ -370,7 +370,7 @@ network::memory& full_node::get_memory() NOEXCEPT
     return memory_;
 }
 
-lock full_node::get_reorganization_lock() NOEXCEPT
+lock full_node::get_reorganization_lock() const NOEXCEPT
 {
     return lock{ reorganization_mutex_ };
 }