Merge pull request #724 from evoskuil/master

Remove mature_, concurrent_, and associated settings.

Author: evoskuil

include/bitcoin/node/chasers/chaser_confirm.hpp

@@ -85,12 +85,10 @@ class BCN_API chaser_confirm
         size_t fork_point) const NOEXCEPT;
 
     // These are protected by strand.
-    bool mature_{};
     network::threadpool threadpool_;
 
     // These are thread safe.
     network::asio::strand independent_strand_;
-    const bool concurrent_;
     bool prevout_;
 };
 

include/bitcoin/node/chasers/chaser_validate.hpp

@@ -75,7 +75,6 @@ class BCN_API chaser_validate
     }
 
     // These are protected by strand.
-    bool mature_{};
     size_t backlog_{};
     network::threadpool threadpool_;
 
@@ -84,7 +83,6 @@ class BCN_API chaser_validate
     const uint32_t subsidy_interval_;
     const uint64_t initial_subsidy_;
     const size_t maximum_backlog_;
-    const bool concurrent_;
     const bool filter_;
 };
 

include/bitcoin/node/error.hpp

@@ -60,6 +60,7 @@ enum error_t : uint8_t
 
     /// faults (terminal, code error and store corruption assumed)
     protocol1,
+    protocol2,
     header1,
     organize1,
     organize2,

include/bitcoin/node/settings.hpp

@@ -72,10 +72,8 @@ class BCN_API settings
     settings(system::chain::selection context) NOEXCEPT;
 
     /// Properties.
+    bool priority;
     bool headers_first;
-    bool priority_validation;
-    bool concurrent_validation;
-    bool concurrent_confirmation;
     float allowed_deviation;
     uint16_t allocation_multiple;
     uint64_t snapshot_bytes;

src/chasers/chaser_check.cpp

@@ -341,8 +341,7 @@ size_t chaser_check::set_unassociated() NOEXCEPT
 
     // Defer new work issuance until gaps filled and confirmation caught up.
     if (position() < requested_ ||
-        requested_ >= maximum_height_
-        /*||confirmed_ < requested_*/)
+        confirmed_ < requested_)
         return {};
 
     // Inventory size gets set only once.

src/chasers/chaser_confirm.cpp

@@ -44,7 +44,6 @@ chaser_confirm::chaser_confirm(full_node& node) NOEXCEPT
   : chaser(node),
     threadpool_(one, node.config().node.priority_()),
     independent_strand_(threadpool_.service().get_executor()),
-    concurrent_(node.config().node.concurrent_confirmation),
     prevout_(node.archive().prevout_enabled())
 {
 }
@@ -92,34 +91,21 @@ bool chaser_confirm::handle_event(const code&, chase event_,
         ////{
         ////    BC_ASSERT(std::holds_alternative<height_t>(value));
         ////
-        ////    if (concurrent_ || mature_)
-        ////    {
-        ////        // TODO: value is branch point.
-        ////        POST(do_validated, std::get<height_t>(value));
-        ////    }
-        ////
+        ////    // TODO: value is branch point.
+        ////    POST(do_validated, std::get<height_t>(value));
         ////    break;
         ////}
         case chase::start:
         case chase::bump:
         {
-            if (concurrent_ || mature_)
-            {
-                POST(do_bump, height_t{});
-            }
-
+            POST(do_bump, height_t{});
             break;
         }
         case chase::valid:
         {
             // value is validated block height.
             BC_ASSERT(std::holds_alternative<height_t>(value));
-
-            if (concurrent_ || mature_)
-            {
-                POST(do_validated, std::get<height_t>(value));
-            }
-
+            POST(do_validated, std::get<height_t>(value));
             break;
         }
         case chase::regressed:
@@ -240,6 +226,13 @@ void chaser_confirm::do_bump(height_t) NOEXCEPT
                     return;
                 }
 
+                // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+                // Failure here was previously result of bug in hashmap, which
+                // caused both iteration across full prevout table and missing
+                // prevout tx records intermittently in confirmation query.
+                // This would prevent subsequent confirmation progress. This
+                // has been resolved.
+                // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
                 notify(ec, chase::unconfirmable, link);
                 fire(events::block_unconfirmable, height);
                 LOGR("Unconfirmable block [" << height << "] " << ec.message());

src/chasers/chaser_validate.cpp

@@ -48,7 +48,6 @@ chaser_validate::chaser_validate(full_node& node) NOEXCEPT
     subsidy_interval_(node.config().bitcoin.subsidy_interval_blocks),
     initial_subsidy_(node.config().bitcoin.initial_subsidy()),
     maximum_backlog_(node.config().node.maximum_concurrency_()),
-    concurrent_(node.config().node.concurrent_validation),
     filter_(node.archive().neutrino_enabled())
 {
 }
@@ -95,23 +94,14 @@ bool chaser_validate::handle_event(const code&, chase event_,
         case chase::start:
         case chase::bump:
         {
-            if (concurrent_ || mature_)
-            {
-                POST(do_bump, height_t{});
-            }
-
+            POST(do_bump, height_t{});
             break;
         }
         case chase::checked:
         {
             // value is checked block height.
             BC_ASSERT(std::holds_alternative<height_t>(value));
-
-            if (concurrent_ || mature_)
-            {
-                POST(do_checked, std::get<height_t>(value));
-            }
-
+            POST(do_checked, std::get<height_t>(value));
             break;
         }
         case chase::regressed:
@@ -316,6 +306,16 @@ void chaser_validate::complete_block(const code& ec, const header_link& link,
         notify(ec, chase::unvalid, link);
         fire(events::block_unconfirmable, height);
         LOGR("Invalid block [" << height << "] " << ec.message());
+
+        // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+        // Stop the network in case of an unexpected invalidity (debugging).
+        // This is considered a bug, not an invalid block arrival (for now).
+        // This usually manifests as accept failure invalid_witness (!checked),
+        // in which case the witness data is simply not present, but bip141 is
+        // active and the output indicates a witness transaction.
+        fault(ec);
+        // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+
         return;
     }
 

src/error.cpp

@@ -50,6 +50,7 @@ DEFINE_ERROR_T_MESSAGE_MAP(error)
 
     /// faults
     { protocol1, "protocol1" },
+    { protocol2, "protocol2" },
     { header1, "header1" },
     { organize1, "organize1" },
     { organize2, "organize2" },

src/parser.cpp

@@ -906,19 +906,9 @@ options_metadata parser::load_settings() THROWS
         "The number of threads in the validation threadpool, defaults to 16."
     )
     (
-        "node.priority_validation",
-        value<bool>(&configured.node.priority_validation),
-        "Set the validation threadpool to high priority, defaults to false."
-    )
-    (
-        "node.concurrent_validation",
-        value<bool>(&configured.node.concurrent_validation),
-        "Perform validation concurrently with download, defaults to false."
-    )
-    (
-        "node.concurrent_confirmation",
-        value<bool>(&configured.node.concurrent_confirmation),
-        "Perform confirmation concurrently with download, defaults to false."
+        "node.priority",
+        value<bool>(&configured.node.priority),
+        "Set the validation threadpool to high priority, defaults to true."
     )
     (
         "node.headers_first",

src/protocols/protocol_block_in_31800.cpp

@@ -304,6 +304,21 @@ bool protocol_block_in_31800::handle_receive_block(const code& ec,
     const auto checked = is_under_checkpoint(height) ||
         query.is_milestone(link);
 
+    // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+    // Failed on block [363353] with block_internal_double_spend (!checked).
+    // The block object remains in scope during the call, but the pointer owns
+    // the memory. There is a local pointer object copied above, but it may be
+    // deleted once it is no longer referenced. This makes the block reference
+    // below weak. The message pointer object is also held by the calling
+    // closure, however it can be deleted by the compiler due to subsequent
+    // non-use. The copied block pointer holds the block object, but it may be
+    // also deleted if not referenced. Passing a block pointer into check,
+    // allowing block->check() to be called would resolve that issue, but not
+    // the issue of calling set_code (see below). But it's not clear how ~block
+    // could have occured here with query.set_code(*block) pending below.
+    // It hasn't failed when using a prevout table, which seems unrelated.
+    // But if it's a race to overwrite freed block memory, it's very possible.
+    // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
     // Tx commitments and malleation are checked under bypass. Invalidity is
     // only stored when a strong header has been stored, later to be found out
     // as invalid and not malleable. Stored invalidity prevents repeat
@@ -315,6 +330,11 @@ bool protocol_block_in_31800::handle_receive_block(const code& ec,
             code == system::error::invalid_witness_commitment ||
             code == system::error::block_malleated)
         {
+            // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+            // These references to block didn't keep it in scope.
+            // But because block is const they could precede the check. Could
+            // be a compiler optimization as these are called by check(true).
+            // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
             LOGR("Uncommitted block [" << encode_hash(hash) << ":" << height
                 << "] from [" << authority() << "] " << code.message()
                 << " txs(" << block->transactions() << ")"
@@ -323,22 +343,12 @@ bool protocol_block_in_31800::handle_receive_block(const code& ec,
             return false;
         }
 
-        // TODO: why were we not setting this block as unconfirmable?
-        ////if (code == system::error::forward_reference)
-        ////{
-        ////    LOGR("Disordered block [" << encode_hash(hash) << ":" << height
-        ////        << " txs(" << block->transactions() << ")"
-        ////        << " segregated(" << block->is_segregated() << ").");
-        ////    stop(code);
-        ////    return false;
-        ////}
-
         // Actual block represented by the hash is unconfirmable.
         if (!query.set_block_unconfirmable(link))
         {
             LOGF("Failure setting block unconfirmable [" << encode_hash(hash)
                 << ":" << height << "] from [" << authority() << "].");
-            fault(error::protocol1);
+            stop(fault(error::protocol1));
             return false;
         }
 
@@ -353,6 +363,18 @@ bool protocol_block_in_31800::handle_receive_block(const code& ec,
     // Commit block.txs.
     // ........................................................................
 
+    // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+    // For early segwit blocks this often fails to set the witness.
+    // There is no failure code, the witness is just empty and stored empty.
+    // That causes a validation failure with invalid_witness (!checked).
+    // It may have also failed in a way that invalidates confirmation queries.
+    // Passing a block pointer here would only hold the block in scope until
+    // it iterates transactions, with no subsequent reference. Then transaction
+    // pointers could hold themselves in scope, however block owns their memory
+    // and ~block() frees that memory even if the transaction pointer is alive.
+    // It hasn't failed when using a prevout table, which seems unrelated.
+    // But if it's a race to overwrite freed block memory, it's very possible.
+    // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
     // This invokes set_strong when checked. 
     if (const auto code = query.set_code(*block, link, checked))
     {
@@ -363,6 +385,23 @@ bool protocol_block_in_31800::handle_receive_block(const code& ec,
         return false;
     }
 
+    // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+    // TODO: verify that ~block() free during contained object access is the
+    // problem by switching to the default memory allocator w/non-prevout run
+    // and the code below disabled.
+    // TODO: pass shared_ptr in to check(block) and set_code(block) from here.
+    // This may guarantee the pointer (vs. block&) lifetime until return.
+    // This is an attempt to keep the shared pointer in scope.
+    // Given that block is const this could also be reordered prior to check().
+    // But this is not called in this scope, only by message deserialization.
+    // Passing the block pointer through the store
+    // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+    if (!block->is_valid())
+    {
+        stop(fault(error::protocol2));
+        return false;
+    }
+
     // Advance.
     // ........................................................................
 
@@ -372,8 +411,10 @@ bool protocol_block_in_31800::handle_receive_block(const code& ec,
     notify(ec, chase::checked, height);
     fire(events::block_archived, height);
 
+    // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
     // block->serialized_size may keep block in scope during set_code above.
     // However the compiler may reorder this calculation since block is const.
+    // ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
     count(block->serialized_size(true));
     map_->erase(it);
     if (is_idle())