Merge pull request #580 from evoskuil/master

Incorporate considerations for merkle tree malleation.

include/bitcoin/node/chasers/chaser_preconfirm.hpp

@@ -48,10 +48,8 @@ class BCN_API chaser_preconfirm
     virtual void do_checked(size_t height) NOEXCEPT;
 
 private:
-    using block_ptr = system::chain::block::cptr;
-
     bool is_under_milestone(size_t height) const NOEXCEPT;
-    code validate(const block_ptr& block,
+    code validate(const system::chain::block& block,
         const database::context& ctx) const NOEXCEPT;
 
     // These are thread safe.

include/bitcoin/node/define.hpp

@@ -23,7 +23,6 @@
 #include <functional>
 #include <memory>
 #include <utility>
-#include <variant>
 
 /// Pulls in common /node headers (excluding settings/config/parser/full_node).
 #include <bitcoin/node/chase.hpp>
@@ -64,16 +63,21 @@ typedef std::shared_ptr<database::associations> map_ptr;
 typedef std::function<void(const code&, const map_ptr&)> map_handler;
 
 /// Node events.
-typedef std::variant<uint32_t, uint64_t> event_link;
+typedef uint64_t event_link;
 typedef network::subscriber<chase, event_link> event_subscriber;
 typedef event_subscriber::handler event_handler;
 
-/// Use for event_link variants.
+/// Use for event_link variants (all unsigned integral integers).
+/// std::variant is inconsistent with interpretation of size_t as redundant or
+/// unique with respect to uint32_t and/or uint64_t (specifically macOS). So
+/// instead these are implicitly casted to event_link (uint64_t) and explicitly
+/// casted from event_link using system::possible_narrow_cast. This is no less
+/// type-safe as using std::variant in cases where the types are overloaded.
+using count_t = size_t;
+using height_t = size_t;
 using channel_t = uint64_t;
 using header_t = database::header_link::integer;
 using transaction_t = database::tx_link::integer;
-////using count_t = size_t;  // quantity
-////using header_t = size_t; // height
 
 } // namespace node
 } // namespace libbitcoin

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

@@ -86,14 +86,14 @@ const typename CLASS::block_tree& CLASS::tree() const NOEXCEPT
 TEMPLATE
 void CLASS::handle_event(const code&, chase event_, event_link value) NOEXCEPT
 {
+    using namespace system;
     switch (event_)
     {
         case chase::unchecked:
         case chase::unpreconfirmed:
         case chase::unconfirmed:
         {
-            BC_ASSERT(std::holds_alternative<header_t>(value));
-            POST(do_disorganize, std::get<header_t>(value));
+            POST(do_disorganize, possible_narrow_cast<header_t>(value));
             break;
         }
         case chase::header:
@@ -150,7 +150,7 @@ void CLASS::do_organize(typename Block::cptr& block_ptr,
         return;
     }
 
-    // If exists test for prior invalidity.
+    // If exists (by hash) test for prior invalidity.
     const auto link = query.to_header(hash);
     if (!link.is_terminal())
     {
@@ -162,6 +162,11 @@ void CLASS::do_organize(typename Block::cptr& block_ptr,
             return;
         }
 
+        // block_unconfirmable is not set when merkle tree is malleable, in
+        // which case the header may be archived in an undetermined state. Not
+        // setting block_unconfirmable for only delays ineviable invalidity
+        // discovery and consequential deorganization at that block. Though
+        // this may cycle until a strong candidate chain is located.
         const auto ec = query.get_header_state(link);
         if (ec == database::error::block_unconfirmable)
         {
@@ -382,30 +387,22 @@ void CLASS::do_disorganize(header_t link) NOEXCEPT
 
     // Mark candidates above and pop at/above height.
     // ........................................................................
+    // Block (link) may be marked as unconfirmable, but if it is not then none
+    // above can be marked, and none can be marked if malleable, so don't mark.
+    // Stored state of each remains unknown until invalid and not malleable.
 
-    // Pop from top down to and including header marking each as unconfirmable.
-    // Unconfirmability isn't necessary for validation but adds query context.
-    for (auto index = query.get_top_candidate(); index > height; --index)
-    {
-        if (!query.set_block_unconfirmable(query.to_candidate(index)) ||
-            !query.pop_candidate())
-        {
-            close(error::store_integrity);
-            return;
-        }
-    }
-
-    // Candidate at height is already marked as unconfirmable by notifier.
+    // Pop from top down to and including invalidating header.
+    for (auto index = query.get_top_candidate(); index >= height; --index)
     {
         if (!query.pop_candidate())
         {
             close(error::store_integrity);
             return;
         }
-
-        fire(events::block_disorganized, height);
     }
 
+    fire(events::block_disorganized, height);
+
     // Reset top chain state cache to fork point.
     // ........................................................................
 

src/chasers/chaser_check.cpp

@@ -62,18 +62,17 @@ code chaser_check::start() NOEXCEPT
 void chaser_check::handle_event(const code&, chase event_,
     event_link value) NOEXCEPT
 {
+    using namespace system;
     switch (event_)
     {
         case chase::header:
         {
-            BC_ASSERT(std::holds_alternative<size_t>(value));
-            POST(do_add_headers, std::get<size_t>(value));
+            POST(do_add_headers, possible_narrow_cast<size_t>(value));
             break;
         }
         case chase::disorganized:
         {
-            BC_ASSERT(std::holds_alternative<size_t>(value));
-            POST(do_purge_headers, std::get<size_t>(value));
+            POST(do_purge_headers, possible_narrow_cast<size_t>(value));
             break;
         }
         ////case chase::header:

src/chasers/chaser_confirm.cpp

@@ -51,24 +51,22 @@ void chaser_confirm::handle_event(const code&, chase event_,
     event_link value) NOEXCEPT
 {
     // These can come out of order, advance in order synchronously.
+    using namespace system;
     switch (event_)
     {
         case chase::block:
         {
-            BC_ASSERT(std::holds_alternative<size_t>(value));
-            POST(do_preconfirmed, std::get<size_t>(value));
+            POST(do_preconfirmed, possible_narrow_cast<size_t>(value));
             break;
         }
         case chase::preconfirmed:
         {
-            BC_ASSERT(std::holds_alternative<size_t>(value));
-            POST(do_preconfirmed, std::get<size_t>(value));
+            POST(do_preconfirmed, possible_narrow_cast<size_t>(value));
             break;
         }
         case chase::disorganized:
         {
-            BC_ASSERT(std::holds_alternative<size_t>(value));
-            POST(do_disorganized, std::get<size_t>(value));
+            POST(do_disorganized, possible_narrow_cast<size_t>(value));
             break;
         }
         case chase::header:
@@ -117,6 +115,9 @@ void chaser_confirm::do_preconfirmed(size_t height) NOEXCEPT
     BC_ASSERT(stranded());
     auto& query = archive();
 
+    // TODO: Do not set block_unconfirmable if its identifier is malleable.
+    ////if (!block->is_malleable() && !query.set_block_unconfirmable(link))...
+
     // Does the height currently represent a strong branch?
     // In case of stronger branch reorganization the branch may become
     // invalidated during processing (popping). How is safety assured?

src/chasers/chaser_preconfirm.cpp

@@ -66,6 +66,7 @@ void chaser_preconfirm::handle_event(const code&, chase event_,
 {
     // These come out of order, advance in order asynchronously.
     // Asynchronous completion results in out of order notification.
+    using namespace system;
     switch (event_)
     {
         case chase::bump:
@@ -75,14 +76,12 @@ void chaser_preconfirm::handle_event(const code&, chase event_,
         }
         case chase::checked:
         {
-            BC_ASSERT(std::holds_alternative<size_t>(value));
-            POST(do_height_checked, std::get<size_t>(value));
+            POST(do_height_checked, possible_narrow_cast<size_t>(value));
             break;
         }
         case chase::disorganized:
         {
-            BC_ASSERT(std::holds_alternative<size_t>(value));
-            POST(do_disorganized, std::get<size_t>(value));
+            POST(do_disorganized, possible_narrow_cast<size_t>(value));
             break;
         }
         case chase::header:
@@ -185,16 +184,20 @@ void chaser_preconfirm::do_checked(size_t) NOEXCEPT
         }
 
         code ec{};
-        if ((ec = validate(block, ctx)))
+        if ((ec = validate(*block, ctx)))
         {
-            if (!query.set_block_unconfirmable(link))
+            // Do not set block_unconfirmable if its identifier is malleable.
+            const auto malleable = block->is_malleable();
+            if (!malleable && !query.set_block_unconfirmable(link))
             {
                 close(error::store_integrity);
                 return;
             }
 
             notify(ec, chase::unpreconfirmed, link);
-            LOGN("Unpreconfirmed [" << height << "] " << ec.message());
+
+            LOGN("Unpreconfirmed [" << height << "] " << ec.message()
+                << (malleable ? " [MALLEABLE]." : ""));
             return;
         }
 
@@ -213,7 +216,7 @@ void chaser_preconfirm::do_checked(size_t) NOEXCEPT
 // utility
 // ----------------------------------------------------------------------------
 
-code chaser_preconfirm::validate(const block_ptr& block,
+code chaser_preconfirm::validate(const chain::block& block,
     const database::context& ctx) const NOEXCEPT
 {
     const chain::context context
@@ -228,14 +231,14 @@ code chaser_preconfirm::validate(const block_ptr& block,
 
     // Assumes all preceding candidates are associated.
     code ec{ system::error::missing_previous_output };
-    if (!archive().populate(*block))
+    if (!archive().populate(block))
         return ec;
 
-    if ((ec = block->accept(context, subsidy_interval_blocks_,
+    if ((ec = block.accept(context, subsidy_interval_blocks_,
         initial_subsidy_)))
         return ec;
 
-    return block->connect(context);
+    return block.connect(context);
 }
 
 BC_POP_WARNING()

src/chasers/chaser_template.cpp

@@ -53,12 +53,12 @@ void chaser_template::handle_event(const code&, chase event_,
     event_link value) NOEXCEPT
 {
     // TODO: also handle confirmed/unconfirmed.
+    using namespace system;
     switch (event_)
     {
         case chase::transaction:
         {
-            BC_ASSERT(std::holds_alternative<transaction_t>(value));
-            POST(do_transaction, std::get<transaction_t>(value));
+            POST(do_transaction, possible_narrow_cast<transaction_t>(value));
             break;
         }
         case chase::header:

src/chasers/chaser_transaction.cpp

@@ -53,12 +53,12 @@ code chaser_transaction::start() NOEXCEPT
 void chaser_transaction::handle_event(const code&, chase event_,
     event_link value) NOEXCEPT
 {
+    using namespace system;
     switch (event_)
     {
         case chase::confirmed:
         {
-            BC_ASSERT(std::holds_alternative<header_t>(value));
-            POST(do_confirmed, std::get<header_t>(value));
+            POST(do_confirmed, possible_narrow_cast<header_t>(value));
             break;
         }
         case chase::header:

src/protocols/protocol_block_in_31800.cpp

@@ -82,6 +82,7 @@ bool protocol_block_in_31800::is_idle() const NOEXCEPT
 void protocol_block_in_31800::handle_event(const code&,
     chase event_, event_link value) NOEXCEPT
 {
+    using namespace system;
     if (stopped())
         return;
 
@@ -102,8 +103,7 @@ void protocol_block_in_31800::handle_event(const code&,
         {
             // It was determined to be the slowest channel with work.
             // If value identifies this channel, split work and stop.
-            BC_ASSERT(std::holds_alternative<channel_t>(value));
-            if (std::get<channel_t>(value) == identifier())
+            if (possible_narrow_cast<channel_t>(value) == identifier())
             {
                 POST(do_split, size_t{});
             }
@@ -320,11 +320,19 @@ bool protocol_block_in_31800::handle_receive_block(const code& ec,
 
     if (const auto error = validate(block, ctx))
     {
-        query.set_block_unconfirmable(link);
+        // Do not set block_unconfirmable if its identifier is malleable.
+        const auto malleable = block.is_malleable();
+        if (!malleable && !query.set_block_unconfirmable(link))
+        {
+            stop(node::error::store_integrity);
+            return false;
+        }
+
         notify(error::success, chase::unchecked, link);
 
         LOGR("Invalid block [" << encode_hash(hash) << ":" << ctx.height
-            << "] from [" << authority() << "] " << error.message());
+            << "] from [" << authority() << "] " << error.message() <<
+            (malleable ? " [MALLEABLE]." : ""));
 
         stop(error);
         return false;