Make fee chaser safe against currency gaps.

include/bitcoin/node/chasers/chaser_estimate.hpp

@@ -55,11 +55,11 @@ class BCN_API chaser_estimate
     virtual bool handle_chase(const code& ec, chase event_,
         event_value value) NOEXCEPT;
 
-    virtual void do_organized(header_t value) NOEXCEPT;
-    virtual void do_reorganized(header_t value) NOEXCEPT;
+    virtual void do_initialize(header_t link) NOEXCEPT;
+    virtual void do_organized(header_t link) NOEXCEPT;
+    virtual void do_reorganized(header_t link) NOEXCEPT;
 
 private:
-    bool initialize() NOEXCEPT;
     void do_estimate(size_t target, estimator::mode mode,
         const estimate_handler& handler) NOEXCEPT;
 

include/bitcoin/node/error.hpp

@@ -59,6 +59,11 @@ enum error_t : uint8_t
     duplicate_block,
     duplicate_header,
 
+    /// fee estimation
+    estimate_disabled,
+    estimate_premature,
+    estimate_false,
+
     /// faults (terminal, code error and store corruption assumed)
     protocol1,
     protocol2,
@@ -98,10 +103,11 @@ enum error_t : uint8_t
     confirm10,
     confirm11,
     confirm12,
-    estimate_failed,
-    estimates_failed,
-    estimates_disabled,
-    estimates_premature
+    estimates_initialize,
+    estimates_push1,
+    estimates_push2,
+    estimates_pop1,
+    estimates_pop2
 };
 
 // No current need for error_code equivalence mapping.

src/chasers/chaser_estimate.cpp

@@ -66,7 +66,7 @@ void chaser_estimate::estimate(size_t target, estimator::mode mode,
 {
     if (!node_settings().fee_estimate_enabled())
     {
-        handler(error::estimates_disabled, {});
+        handler(error::estimate_disabled, {});
         return;
     }
 
@@ -82,13 +82,13 @@ void chaser_estimate::do_estimate(size_t target, estimator::mode mode,
     // Check this under strand so that chase can initialize first.
     if (!initialized())
     {
-        handler(error::estimates_premature, {});
+        handler(error::estimate_premature, {});
         return;
     }
 
     const auto value = estimator_->estimate(target, mode);
     const auto ec = (value < to_unsigned(max_int64) ? error::success :
-        error::estimate_failed);
+        error::estimate_false);
 
     // Successful value is always castable to int64_t.
     handler(ec, value);
@@ -120,19 +120,39 @@ bool chaser_estimate::handle_chase(const code&, chase event_,
 
     switch (event_)
     {
-        // chase::block is only sent when current (may need to file gaps).
+        // chase::block is only sent when current. This is captured as a cheap
+        // way to test currency for initialization. Once initialized it is not
+        // used again. chase::organized is used instead, to ensure that there
+        // are no push() gaps due to falling out of currency.
         case chase::block:
         {
-            BC_ASSERT(std::holds_alternative<header_t>(value));
-            POST(do_organized, std::get<header_t>(value));
+            if (!initialized())
+            {
+                BC_ASSERT(std::holds_alternative<header_t>(value));
+                POST(do_initialize, std::get<header_t>(value));
+            }
+
             break;
         }
-        case chase::reorganized:
+        case chase::organized:
         {
-            BC_ASSERT(std::holds_alternative<header_t>(value));
-            POST(do_reorganized, std::get<header_t>(value));
+            if (initialized())
+            {
+                BC_ASSERT(std::holds_alternative<header_t>(value));
+                POST(do_organized, std::get<header_t>(value));
+            }
+
             break;
         }
+        case chase::reorganized:
+        {
+            if (initialized())
+            {
+                BC_ASSERT(std::holds_alternative<header_t>(value));
+                POST(do_reorganized, std::get<header_t>(value));
+                break;
+            }
+        }
         case chase::stop:
         {
             return false;
@@ -146,48 +166,76 @@ bool chaser_estimate::handle_chase(const code&, chase event_,
     return true;
 }
 
-void chaser_estimate::do_organized(header_t) NOEXCEPT
+void chaser_estimate::do_initialize(header_t) NOEXCEPT
 {
     BC_ASSERT(stranded());
 
-    // TODO: make gap safe (get estimator top and adjust).
-    if (initialized() || initialize())
-        estimator_->push(archive());
-}
-
-void chaser_estimate::do_reorganized(header_t) NOEXCEPT
-{
-    BC_ASSERT(stranded());
-
-    // TODO: make gap safe (get estimator top and adjust).
     if (initialized())
-        estimator_->pop(archive());
-}
-
-// utility
-// ----------------------------------------------------------------------------
-// private
-
-bool chaser_estimate::initialize() NOEXCEPT
-{
-    BC_ASSERT(stranded());
+        return;
 
     // Preempt initialize fault when horizon exceeds chain length.
     const auto horizon = node_settings().fee_estimate_horizon_();
     if (horizon > add1(archive().get_top_confirmed()))
-        return false;
+        return;
 
     // Heap-allocate the estimator due to size.
     estimator_ = std::make_unique<estimator>();
     if (!estimator_->initialize(stopping_, archive(), horizon))
     {
-        fault(error::estimates_failed);
+        fault(error::estimates_initialize);
         estimator_.release();
-        return false;
+        return;
     }
 
     initialized_.store(true, std::memory_order_relaxed);
-    return true;
+}
+
+void chaser_estimate::do_organized(header_t link) NOEXCEPT
+{
+    BC_ASSERT(stranded());
+
+    if (initialized())
+    {
+        const auto& query = archive();
+        const auto height = query.get_height(link);
+
+        if (height.is_terminal())
+        {
+            fault(error::estimates_push1);
+            return;
+        }
+
+        // Organization events backlog during initialization.
+        if (height.value <= estimator_->top_height())
+            return;
+
+        if (!estimator_->push(query))
+            fault(error::estimates_push2);
+    }
+}
+
+void chaser_estimate::do_reorganized(header_t link) NOEXCEPT
+{
+    BC_ASSERT(stranded());
+
+    if (initialized())
+    {
+        const auto& query = archive();
+        const auto height = query.get_height(link);
+
+        if (height.is_terminal())
+        {
+            fault(error::estimates_pop1);
+            return;
+        }
+
+        // Organization events backlog during initialization.
+        if (height.value > estimator_->top_height())
+            return;
+
+        if (!estimator_->push(query))
+            fault(error::estimates_pop2);
+    }
 }
 
 BC_POP_WARNING()

src/error.cpp

@@ -49,6 +49,11 @@ DEFINE_ERROR_T_MESSAGE_MAP(error)
     { duplicate_block, "duplicate block" },
     { duplicate_header, "duplicate header" },
 
+    // fee estimation
+    { estimate_disabled, "estimate_disabled" },
+    { estimate_premature, "estimate_premature" },
+    { estimate_false, "estimate_false" },
+
     // faults
     { protocol1, "protocol1" },
     { protocol2, "protocol2" },
@@ -88,10 +93,11 @@ DEFINE_ERROR_T_MESSAGE_MAP(error)
     { confirm10, "confirm10" },
     { confirm11, "confirm11" },
     { confirm12, "confirm12" },
-    { estimate_failed, "estimate_failed" },
-    { estimates_failed, "estimates_failed" },
-    { estimates_disabled, "estimates_disabled" },
-    { estimates_premature, "estimates_premature" }
+    { estimates_initialize, "estimates_initialize" },
+    { estimates_push1, "estimates_push1" },
+    { estimates_push2, "estimates_push2" },
+    { estimates_pop1, "estimates_pop1" },
+    { estimates_pop2, "estimates_pop2" }
 };
 
 DEFINE_ERROR_T_CATEGORY(error, "node", "node code")

test/error.cpp

@@ -166,6 +166,33 @@ BOOST_AUTO_TEST_CASE(error_t__code__duplicate_header__true_expected_message)
     BOOST_REQUIRE_EQUAL(ec.message(), "duplicate header");
 }
 
+BOOST_AUTO_TEST_CASE(error_t__code__estimate_disabled__true_expected_message)
+{
+    constexpr auto value = error::estimate_disabled;
+    const auto ec = code(value);
+    BOOST_REQUIRE(ec);
+    BOOST_REQUIRE(ec == value);
+    BOOST_REQUIRE_EQUAL(ec.message(), "estimate_disabled");
+}
+
+BOOST_AUTO_TEST_CASE(error_t__code__estimate_premature__true_expected_message)
+{
+    constexpr auto value = error::estimate_premature;
+    const auto ec = code(value);
+    BOOST_REQUIRE(ec);
+    BOOST_REQUIRE(ec == value);
+    BOOST_REQUIRE_EQUAL(ec.message(), "estimate_premature");
+}
+
+BOOST_AUTO_TEST_CASE(error_t__code__estimate_false__true_expected_message)
+{
+    constexpr auto value = error::estimate_false;
+    const auto ec = code(value);
+    BOOST_REQUIRE(ec);
+    BOOST_REQUIRE(ec == value);
+    BOOST_REQUIRE_EQUAL(ec.message(), "estimate_false");
+}
+
 // faults
 
 BOOST_AUTO_TEST_CASE(error_t__code__protocol1__true_expected_message)
@@ -225,40 +252,49 @@ BOOST_AUTO_TEST_CASE(error_t__code__confirm1__true_expected_message)
     BOOST_REQUIRE_EQUAL(ec.message(), "confirm1");
 }
 
-BOOST_AUTO_TEST_CASE(error_t__code__estimate_failed__true_expected_message)
+BOOST_AUTO_TEST_CASE(error_t__code__estimates_initialize__true_expected_message)
+{
+    constexpr auto value = error::estimates_initialize;
+    const auto ec = code(value);
+    BOOST_REQUIRE(ec);
+    BOOST_REQUIRE(ec == value);
+    BOOST_REQUIRE_EQUAL(ec.message(), "estimates_initialize");
+}
+
+BOOST_AUTO_TEST_CASE(error_t__code__estimates_push1__true_expected_message)
 {
-    constexpr auto value = error::estimate_failed;
+    constexpr auto value = error::estimates_push1;
     const auto ec = code(value);
     BOOST_REQUIRE(ec);
     BOOST_REQUIRE(ec == value);
-    BOOST_REQUIRE_EQUAL(ec.message(), "estimate_failed");
+    BOOST_REQUIRE_EQUAL(ec.message(), "estimates_push1");
 }
 
-BOOST_AUTO_TEST_CASE(error_t__code__estimates_failed__true_expected_message)
+BOOST_AUTO_TEST_CASE(error_t__code__estimates_push2__true_expected_message)
 {
-    constexpr auto value = error::estimates_failed;
+    constexpr auto value = error::estimates_push2;
     const auto ec = code(value);
     BOOST_REQUIRE(ec);
     BOOST_REQUIRE(ec == value);
-    BOOST_REQUIRE_EQUAL(ec.message(), "estimates_failed");
+    BOOST_REQUIRE_EQUAL(ec.message(), "estimates_push2");
 }
 
-BOOST_AUTO_TEST_CASE(error_t__code__estimates_disabled__true_expected_message)
+BOOST_AUTO_TEST_CASE(error_t__code__estimates_pop1__true_expected_message)
 {
-    constexpr auto value = error::estimates_disabled;
+    constexpr auto value = error::estimates_pop1;
     const auto ec = code(value);
     BOOST_REQUIRE(ec);
     BOOST_REQUIRE(ec == value);
-    BOOST_REQUIRE_EQUAL(ec.message(), "estimates_disabled");
+    BOOST_REQUIRE_EQUAL(ec.message(), "estimates_pop1");
 }
 
-BOOST_AUTO_TEST_CASE(error_t__code__estimates_premature__true_expected_message)
+BOOST_AUTO_TEST_CASE(error_t__code__estimates_pop2__true_expected_message)
 {
-    constexpr auto value = error::estimates_premature;
+    constexpr auto value = error::estimates_pop2;
     const auto ec = code(value);
     BOOST_REQUIRE(ec);
     BOOST_REQUIRE(ec == value);
-    BOOST_REQUIRE_EQUAL(ec.message(), "estimates_premature");
+    BOOST_REQUIRE_EQUAL(ec.message(), "estimates_pop2");
 }
 
 BOOST_AUTO_TEST_SUITE_END()