Merge pull request #647 from evoskuil/master

Simplified malleability, confirmation when validation is current.

console/executor.cpp

@@ -122,7 +122,6 @@ const std::unordered_map<uint8_t, std::string> executor::fired_
     { events::block_validated,     "block_validated....." },
     { events::block_confirmed,     "block_confirmed....." },
     { events::block_unconfirmable, "block_unconfirmable." },
-    { events::block_malleated,     "block_malleated....." },
     { events::validate_bypassed,   "validate_bypassed..." },
     { events::confirm_bypassed,    "confirm_bypassed...." },
 

include/bitcoin/node/chase.hpp

@@ -69,7 +69,7 @@ enum class chase
     /// -----------------------------------------------------------------------
 
     /// A candidate header has been disassociated due to malleation (header_t).
-    /// Issued by 'header' and handled by 'check'.
+    /// Issued by 'header' and handled by 'check' (redownload).
     header,
 
     /// A new candidate branch exists from given branch point (height_t).
@@ -95,8 +95,8 @@ enum class chase
     /// Issued by 'organize' and handled by 'validate' (disorgs candidates).
     disorganized,
 
-    /// stored block was determined to be malleated (invalid) (header_t).
-    /// Issued by 'validate' and 'confirm', handled by 'chaser' (redownload).
+    /// unarchived block was determined to be malleated (header_t).
+    /// Issued by 'block_in_31800', handled by 'organize' (redownload).
     malleated,
 
     /// Validation.

include/bitcoin/node/chasers/chaser.hpp

@@ -19,6 +19,7 @@
 #ifndef LIBBITCOIN_NODE_CHASERS_CHASER_HPP
 #define LIBBITCOIN_NODE_CHASERS_CHASER_HPP
 
+#include <bitcoin/database.hpp>
 #include <bitcoin/network.hpp>
 #include <bitcoin/node/configuration.hpp>
 #include <bitcoin/node/define.hpp>
@@ -125,6 +126,9 @@ class BCN_API chaser
     /// Header timestamp is within configured span from current time.
     bool is_current(uint32_t timestamp) const NOEXCEPT;
 
+    /// Header's timestamp is within configured span from current time.
+    bool is_current(const database::header_link& link) const NOEXCEPT;
+
     /// The height is at or below the top checkpoint.
     bool is_under_checkpoint(size_t height) const NOEXCEPT;
 

include/bitcoin/node/chasers/chaser_block.hpp

@@ -52,7 +52,7 @@ class BCN_API chaser_block
     virtual code validate(const system::chain::block& block,
         const chain_state& state) const NOEXCEPT;
 
-    /// Handle malleted message (nop).
+    /// Notify check chaser to redownload the block (nop).
     virtual void do_malleated(header_t link) NOEXCEPT;
 
     /// Determine if state is top of a storable branch (always true).

include/bitcoin/node/chasers/chaser_header.hpp

@@ -55,7 +55,7 @@ class BCN_API chaser_header
     virtual code validate(const system::chain::header& header,
         const chain_state& state) const NOEXCEPT;
 
-    /// Disassociate malleated block and notify to redownload header.
+    /// Notify check chaser to redownload the block.
     virtual void do_malleated(header_t link) NOEXCEPT;
 
     /// Determine if state is top of a storable branch.

include/bitcoin/node/chasers/chaser_organize.hpp

@@ -74,7 +74,7 @@ class chaser_organize
     virtual code validate(const Block& block,
         const chain_state& state) const NOEXCEPT = 0;
 
-    /// Disassociate malleated block and notify repeat header in current job.
+    /// Notify check chaser to redownload the block.
     virtual void do_malleated(header_t link) NOEXCEPT = 0;
 
     /// Determine if state is top of a storable branch.

include/bitcoin/node/events.hpp

@@ -39,7 +39,6 @@ enum events : uint8_t
     block_validated,     // block checked, accepted, connected
     block_confirmed,     // block checked, accepted, connected, confirmable
     block_unconfirmable, // block invalid (after headers-first archive)
-    block_malleated,     // block invalid but malleable (after headers-first)
     validate_bypassed,   // block checked, accepted [assumed]
     confirm_bypassed,    // block checked, accepted, connected [assumed]
 

include/bitcoin/node/impl/chasers/chaser_organize.ipp

@@ -101,7 +101,7 @@ bool CLASS::handle_event(const code&, chase event_, event_value value) NOEXCEPT
         }
         case chase::malleated:
         {
-            // Re-obtain the malleated block if it is still a candidate.
+            // Re-obtain the malleated block if it is still a candidate (virtual).
             POST(do_malleated, possible_narrow_cast<header_t>(value));
             break;
         }
@@ -211,6 +211,9 @@ void CLASS::do_organize(typename Block::cptr& block_ptr,
         return;
     };
 
+    // blocks-first may return malleation error, in which case another peer may
+    // return the good block of the same hash. headers-first cannot detect
+    // malleation here, so the block_in protocol sends chase::malleated.
     if (const auto ec = validate(*block_ptr, *state))
     {
         handler(ec, height);
@@ -345,7 +348,7 @@ void CLASS::do_organize(typename Block::cptr& block_ptr,
         // arrivals. This bumps validation for current strong headers.
         notify(error::success, chase::bump, add1(branch_point));
 
-        // This is just to prevent stall, the check chaser races ahead.
+        // This is to prevent download stall, the check chaser races ahead.
         // Start block downloads, which upon completion bumps validation.
         notify(error::success, chase_object(), branch_point);
     }
@@ -366,8 +369,7 @@ void CLASS::do_disorganize(header_t link) NOEXCEPT
     // Skip already reorganized out, get height.
     // ........................................................................
 
-    // Upon restart chain validation will hit unconfirmable or gapped block.
-    // Gapping occurs with a malleated64 from confirm in blocks first handling.
+    // Upon restart chain validation will hit unconfirmable block.
     if (closed())
         return;
 
@@ -574,14 +576,13 @@ TEMPLATE
 code CLASS::push_block(const Block& block,
     const system::chain::context& context) const NOEXCEPT
 {
-    // set milestone and sets strong if milestone or checkpoint.
-    const auto checkpoint = is_under_checkpoint(context.height);
-    const auto milestone = is_under_milestone(context.height);
-    const auto is_strong = is_block() && (checkpoint || milestone);
+    // set_code invokes set_strong when checked.
+    const auto milestone = is_under_checkpoint(context.height);
+    const auto checked = is_block() && is_under_checkpoint(context.height);
 
     auto& query = archive();
     database::header_link link{};
-    const auto ec = query.set_code(link, block, context, milestone, is_strong);
+    const auto ec = query.set_code(link, block, context, milestone, checked);
     if (ec)
         return ec;
 

src/chasers/chaser.cpp

@@ -18,6 +18,7 @@
  */
 #include <bitcoin/node/chasers/chaser.hpp>
 
+#include <bitcoin/database.hpp>
 #include <bitcoin/network.hpp>
 #include <bitcoin/node/configuration.hpp>
 #include <bitcoin/node/define.hpp>
@@ -135,6 +136,14 @@ bool chaser::is_under_checkpoint(size_t height) const NOEXCEPT
     return height <= top_checkpoint_height_;
 }
 
+// get_timestamp error results in false (ok).
+bool chaser::is_current(const database::header_link& link) const NOEXCEPT
+{
+    uint32_t timestamp{};
+    return archive().get_timestamp(timestamp, link) &&
+        node_.is_current(timestamp);
+}
+
 // Position.
 // ----------------------------------------------------------------------------
 

src/chasers/chaser_block.cpp

@@ -70,10 +70,9 @@ code chaser_block::validate(const block& block,
 
     // Transaction/witness commitments are required under checkpoint.
     // This ensures that the block/header hash represents expected txs.
-    const auto checked = is_under_checkpoint(state.height()) &&
-        !block.is_malleable64();
+    const auto checked = is_under_checkpoint(state.height());
 
-    // Transaction commitments and malleated32 are checked under bypass.
+    // Transaction commitments and malleation are checked under bypass.
     if ((ec = block.check(checked)))
         return ec;
 
@@ -97,33 +96,16 @@ code chaser_block::validate(const block& block,
     return block.connect(ctx);
 }
 
-// The archived malleable block was found to be invalid (treat as malleated).
-// The block/header hash cannot be marked unconfirmable due to malleability, so
-// disassociate the block and then disorganize the chain to malleation point.
-// This will disorganize the candidate chain to match the confirmed.
-void chaser_block::do_malleated(header_t link) NOEXCEPT
+void chaser_block::do_malleated(header_t) NOEXCEPT
 {
-    BC_ASSERT(stranded());
-    auto& query = archive();
-
-    // If not disassociated, validation/confirmation will be reattempted.
-    // This could happen due to shutdown before this step is completed.
-    if (!query.set_dissasociated(link))
-    {
-        fault(error::set_dissasociated);
-        return;
-    }
-
-    // Treat as disorganization, but block is only gapped not invalidated.
-    do_disorganize(link);
+    // blocks are guarded against malleation before being stored.
 }
 
 bool chaser_block::is_storable(const chain_state&) const NOEXCEPT
 {
     return true;
 }
 
-
 // Milestone methods.
 // ----------------------------------------------------------------------------
 

src/chasers/chaser_check.cpp

@@ -238,14 +238,15 @@ void chaser_check::do_headers(height_t) NOEXCEPT
         notify(error::success, chase::download, added);
 }
 
-// The archived malleable block was found to be invalid (treat as malleated).
+// A malleable block was found to be malleated, so re-download.
 void chaser_check::do_header(header_t link) NOEXCEPT
 {
     BC_ASSERT(stranded());
 
     association out{};
     if (!archive().get_unassociated(out, link))
     {
+        // False return may be the result of existing association (still error).
         fault(error::get_unassociated);
         return;
     }

src/chasers/chaser_confirm.cpp

@@ -69,12 +69,12 @@ bool chaser_confirm::handle_event(const code&, chase event_,
             POST(do_validated, possible_narrow_cast<height_t>(value));
             break;
         }
-        ////case chase::valid:
-        ////{
-        ////    // value is individual height.
-        ////    POST(do_validated, possible_narrow_cast<height_t>(value));
-        ////    break;
-        ////}
+        case chase::valid:
+        {
+            // value is individual height.
+            POST(do_validated, possible_narrow_cast<height_t>(value));
+            break;
+        }
         case chase::stop:
         {
             return false;

src/chasers/chaser_header.cpp

@@ -81,30 +81,15 @@ code chaser_header::validate(const header& header,
     return system::error::block_success;
 }
 
-// The archived malleable block was found to be invalid (treat as malleated).
-// The block/header hash cannot be marked unconfirmable due to malleability, so
-// disassociate the block and then notify check chaser to reisuse the download.
-// This must be issued here in order to ensure proper bypass/regress ordering.
+// A malleable block was found to be invalid due to malleation.
 void chaser_header::do_malleated(header_t link) NOEXCEPT
 {
     BC_ASSERT(stranded());
-    auto& query = archive();
 
-    // If not disassociated, validation/confirmation will be reattempted.
-    // This could happen due to shutdown before this step is completed.
-    if (!query.set_dissasociated(link))
-    {
-        fault(error::set_dissasociated);
-        return;
-    }
-
-    // Header is no longer in the candidate chain, so do not announce.
-    if (!query.is_candidate_header(link))
-        return;
-
-    // Announce a singleton header that requires download.
+    // Announce a single header that requires (re)download.
     // Since it is in the candidate chain, it must presently be missing.
-    notify(error::success, chase::header, link);
+    if (archive().is_candidate_header(link))
+        notify(error::success, chase::header, link);
 }
 
 // Storable methods (private).

src/chasers/chaser_validate.cpp

@@ -173,7 +173,11 @@ void chaser_validate::do_bump(height_t) NOEXCEPT
         {
             update_position(height);
             ////fire(events::validate_bypassed, height);
-            notify(ec, chase::valid, height);
+
+            // Don't confirm until validations are current.
+            if (is_current(link))
+                notify(ec, chase::valid, height);
+
             continue;
         }
 
@@ -193,7 +197,10 @@ void chaser_validate::do_bump(height_t) NOEXCEPT
         // Retain last height in validation sequence, update neutrino.
         update_position(height);
         fire(events::block_validated, height);
-        notify(ec, chase::valid, height);
+
+        // Don't confirm until validations are current.
+        if (is_current(link))
+            notify(ec, chase::valid, height);
     }
 }