Merge pull request #569 from evoskuil/master

Add/use node.headers_first, simplify attach, use perf param, logging.

.github/workflows/ci.yml

@@ -755,7 +755,7 @@ jobs:
           $BC_TEST_SINGLETON = $BC_TEST_EXES.FullName;
           Write-Host "Executing $BC_TEST_SINGLETON $env:BOOST_UNIT_TEST_OPTIONS" -ForegroundColor Yellow;
           try {
-            Invoke-Expression "$BC_TEST_SINGLETON --run_test=${{ matrix.tests }} $env:BOOST_UNIT_TEST_OPTIONS"
+            Invoke-Expression "$BC_TEST_SINGLETON --log_level=warning --run_test=${{ matrix.tests }} $env:BOOST_UNIT_TEST_OPTIONS"
           }
           catch {
             $ERR = $_;

builds/cmake/CMakeLists.txt

@@ -45,55 +45,55 @@ set( CMAKE_CXX_STANDARD_REQUIRED ON )
 # Warn on all stuff.
 check_cxx_compiler_flag( "-Wall" HAS_FLAG_WALL )
 if ( HAS_FLAG_WALL )
-    add_compile_options( "-Wall" )
+    add_compile_options( $<$<COMPILE_LANGUAGE:CXX>:-Wall> )
 else()
     message( FATAL_ERROR "Compiler does not support -Wall" )
 endif()
 
 # Warn on extra stuff.
 check_cxx_compiler_flag( "-Wextra" HAS_FLAG_WEXTRA )
 if ( HAS_FLAG_WEXTRA )
-    add_compile_options( "-Wextra" )
+    add_compile_options( $<$<COMPILE_LANGUAGE:CXX>:-Wextra> )
 else()
     message( FATAL_ERROR "Compiler does not support -Wextra" )
 endif()
 
 # Disallow warning on style order of declarations.
 check_cxx_compiler_flag( "-Wno-reorder" HAS_FLAG_WNO-REORDER )
 if ( HAS_FLAG_WNO-REORDER )
-    add_compile_options( "-Wno-reorder" )
+    add_compile_options( $<$<COMPILE_LANGUAGE:CXX>:-Wno-reorder> )
 else()
     message( FATAL_ERROR "Compiler does not support -Wno-reorder" )
 endif()
 
 # Suppress warning for incomplete field initialization.
 check_cxx_compiler_flag( "-Wno-missing-field-initializers" HAS_FLAG_WNO-MISSING-FIELD-INITIALIZERS )
 if ( HAS_FLAG_WNO-MISSING-FIELD-INITIALIZERS )
-    add_compile_options( "-Wno-missing-field-initializers" )
+    add_compile_options( $<$<COMPILE_LANGUAGE:CXX>:-Wno-missing-field-initializers> )
 else()
     message( FATAL_ERROR "Compiler does not support -Wno-missing-field-initializers" )
 endif()
 
 # Conform to style.
 check_cxx_compiler_flag( "-Wno-missing-braces" HAS_FLAG_WNO-MISSING-BRACES )
 if ( HAS_FLAG_WNO-MISSING-BRACES )
-    add_compile_options( "-Wno-missing-braces" )
+    add_compile_options( $<$<COMPILE_LANGUAGE:CXX>:-Wno-missing-braces> )
 else()
     message( FATAL_ERROR "Compiler does not support -Wno-missing-braces" )
 endif()
 
 # Ignore comments within comments or commenting of backslash extended lines.
 check_cxx_compiler_flag( "-Wno-comment" HAS_FLAG_WNO-COMMENT )
 if ( HAS_FLAG_WNO-COMMENT )
-    add_compile_options( "-Wno-comment" )
+    add_compile_options( $<$<COMPILE_LANGUAGE:CXX>:-Wno-comment> )
 else()
     message( FATAL_ERROR "Compiler does not support -Wno-comment" )
 endif()
 
 # Suppress warning for copy of implicitly generated copy constructor.
 check_cxx_compiler_flag( "-Wno-deprecated-copy" HAS_FLAG_WNO-DEPRECATED-COPY )
 if ( HAS_FLAG_WNO-DEPRECATED-COPY )
-    add_compile_options( "-Wno-deprecated-copy" )
+    add_compile_options( $<$<COMPILE_LANGUAGE:CXX>:-Wno-deprecated-copy> )
 else()
     message( FATAL_ERROR "Compiler does not support -Wno-deprecated-copy" )
 endif()
@@ -102,7 +102,7 @@ endif()
 if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
     check_cxx_compiler_flag( "-Wno-mismatched-tags" HAS_FLAG_WNO-MISMATCHED-TAGS )
     if ( HAS_FLAG_WNO-MISMATCHED-TAGS )
-        add_compile_options( "-Wno-mismatched-tags" )
+        add_compile_options( $<$<COMPILE_LANGUAGE:CXX>:-Wno-mismatched-tags> )
     else()
         message( FATAL_ERROR "Compiler does not support -Wno-mismatched-tags" )
     endif()
@@ -362,6 +362,7 @@ if (with-tests)
 
     add_test( NAME libbitcoin-node-test COMMAND libbitcoin-node-test
             --run_test=*
+            --log_level=warning
             --show_progress=no
             --detect_memory_leak=0
             --report_level=no

builds/cmake/CMakePresets.json

@@ -79,8 +79,8 @@
             "description": "Factored size optimization settings.",
             "hidden": true,
             "cacheVariables": {
-                "CMAKE_C_FLAGS": "$env{CMAKE_C_FLAGS} -Os -s",
-                "CMAKE_CXX_FLAGS": "$env{CMAKE_CXX_FLAGS} -Os -s"
+                "CMAKE_C_FLAGS": "$env{CMAKE_C_FLAGS} -Os",
+                "CMAKE_CXX_FLAGS": "$env{CMAKE_CXX_FLAGS} -Os"
             }
         },
         {

builds/msvc/vs2022/libbitcoin-node-test/libbitcoin-node-test.props

@@ -15,7 +15,7 @@
       <PreprocessorDefinitions Condition="'$(DefaultLinkage)' == 'dynamic'">BOOST_TEST_DYN_LINK;%(PreprocessorDefinitions)</PreprocessorDefinitions>
     </ClCompile>
     <PostBuildEvent Condition="'$(DebugOrRelease)' == 'release'">
-      <Command>"$(TargetPath)" --run_test=* --show_progress=no --build_info=yes</Command>
+      <Command>"$(TargetPath)" --log_level=warning --run_test=* --show_progress=no --build_info=yes</Command>
     </PostBuildEvent>
   </ItemDefinitionGroup>
 

include/bitcoin/node/chasers/chaser_block.hpp

@@ -52,7 +52,6 @@ class BCN_API chaser_block
 
     /// Determine if Block is top of a storable branch.
     virtual bool is_storable(const system::chain::block& block,
-        size_t height, const system::hash_digest& hash,
         const system::chain::chain_state& state) const NOEXCEPT;
 
 private:

include/bitcoin/node/chasers/chaser_header.hpp

@@ -54,7 +54,6 @@ class BCN_API chaser_header
 
     /// Determine if Block is top of a storable branch.
     virtual bool is_storable(const system::chain::header& header,
-        size_t height, const system::hash_digest& hash,
         const system::chain::chain_state& state) const NOEXCEPT;
 };
 

include/bitcoin/node/chasers/chaser_organize.hpp

@@ -72,8 +72,7 @@ class chaser_organize
         const chain_state& state) const NOEXCEPT = 0;
 
     /// Determine if Block is top of a storable branch.
-    virtual bool is_storable(const Block& block, size_t height,
-        const system::hash_digest& hash,
+    virtual bool is_storable(const Block& block,
         const chain_state& state) const NOEXCEPT = 0;
 
     /// Constant access to Block tree.
@@ -94,7 +93,7 @@ class chaser_organize
 
 private:
     // Store Block into logical tree cache.
-    void cache(const typename Block::cptr& block,
+    void cache(const typename Block::cptr& block_ptr,
         const chain_state::ptr& state) NOEXCEPT;
 
     // Obtain chain state for given header hash, nullptr if not found. 

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

@@ -93,7 +93,6 @@ const typename CLASS::block_tree& CLASS::tree() const NOEXCEPT
 TEMPLATE
 void CLASS::handle_event(const code&, chase event_, link value) NOEXCEPT
 {
-    // Posted due to block/header invalidation.
     // Block chaser doesn't need to capture unchecked/unconnected (but okay).
     if (event_ == chase::unchecked ||
         event_ == chase::unconnected ||
@@ -146,8 +145,8 @@ void CLASS::do_disorganize(header_t header) NOEXCEPT
     {
         const auto link = query.to_candidate(index);
 
-        LOGN("Invalidating candidate [" << index << ":"
-            << encode_hash(query.get_header_key(link)) << "].");
+        LOGN("Invalidating candidate ["
+            << encode_hash(query.get_header_key(link))<< ":" << index << "].");
 
         if (!query.set_block_unconfirmable(link) || !query.pop_candidate())
         {
@@ -156,8 +155,8 @@ void CLASS::do_disorganize(header_t header) NOEXCEPT
         }
     }
 
-    LOGN("Invalidating candidate [" << height << ":"
-        << encode_hash(query.get_header_key(header)) << "].");
+    LOGN("Invalidating candidate ["
+        << encode_hash(query.get_header_key(header)) << ":" << height << "].");
 
     // Candidate at height is already marked as unconfirmable by notifier.
     if (!query.pop_candidate())
@@ -268,38 +267,49 @@ void CLASS::do_organize(typename Block::cptr& block_ptr,
             return;
         }
 
-        if (tree_.contains(hash))
+        const auto it = tree_.find(hash);
+        if (it != tree_.end())
         {
+            const auto height = it->second.state->height();
             if constexpr (is_same_type<Block, chain::block>)
-                handler(error::duplicate_block, {});
+                handler(error::duplicate_block, height);
             else
-                handler(error::duplicate_header, {});
+                handler(error::duplicate_header, height);
 
             return;
         }
 
-        // If header exists test for prior invalidity.
+        // If exists test for prior invalidity.
         const auto link = query.to_header(hash);
         if (!link.is_terminal())
         {
+            size_t height{};
+            if (!query.get_height(height, link))
+            {
+                handler(error::store_integrity, {});
+                close(error::store_integrity);
+                return;
+            }
+
             const auto ec = query.get_header_state(link);
             if (ec == database::error::block_unconfirmable)
             {
-                handler(ec, {});
+                handler(ec, height);
                 return;
             }
 
             if constexpr (is_same_type<Block, chain::block>)
             {
+                // Blocks are only duplicates if txs are associated.
                 if (ec != database::error::unassociated)
                 {
-                    handler(error::duplicate_block, {});
+                    handler(error::duplicate_block, height);
                     return;
                 }
             }
             else
             {
-                handler(error::duplicate_header, {});
+                handler(error::duplicate_header, height);
                 return;
             }
         }
@@ -364,7 +374,7 @@ void CLASS::do_organize(typename Block::cptr& block_ptr,
             return;
         }
 
-        if (!is_storable(block, height, hash, *state))
+        if (!is_storable(block, *state))
         {
             cache(block_ptr, state);
             handler(error::success, height);
@@ -393,13 +403,6 @@ void CLASS::do_organize(typename Block::cptr& block_ptr,
             return;
         }
 
-        // If a long candidate chain is first created using headers-first and then
-        // blocks-first is executed (after a restart/config) it can result in up to
-        // the entire blockchain being cached into memory before becoming strong,
-        // which means stronger than the candidate chain. While switching config
-        // between modes by varying network protocol is supported, blocks-first is
-        // inherently inefficient and weak on this aspect of DoS protection. This
-        // is acceptable for its purpose and consistent with early implementations.
         if (!strong)
         {
             // New top of current weak branch.
@@ -486,10 +489,10 @@ void CLASS::do_organize(typename Block::cptr& block_ptr,
 // ----------------------------------------------------------------------------
 
 TEMPLATE
-void CLASS::cache(const typename Block::cptr& block,
+void CLASS::cache(const typename Block::cptr& block_ptr,
     const system::chain::chain_state::ptr& state) NOEXCEPT
 {
-    tree_.insert({ block->hash(), { block, state } });
+    tree_.insert({ block_ptr->hash(), { block_ptr, state } });
 }
 
 TEMPLATE

include/bitcoin/node/protocols/protocol_block_in.hpp

@@ -19,6 +19,7 @@
 #ifndef LIBBITCOIN_NODE_PROTOCOLS_PROTOCOL_BLOCK_IN_HPP
 #define LIBBITCOIN_NODE_PROTOCOLS_PROTOCOL_BLOCK_IN_HPP
 
+#include <unordered_set>
 #include <bitcoin/network.hpp>
 #include <bitcoin/node/define.hpp>
 #include <bitcoin/node/protocols/protocol.hpp>
@@ -50,29 +51,27 @@ class BCN_API protocol_block_in
     void start() NOEXCEPT override;
 
 protected:
+    using hashmap = std::unordered_set<system::hash_digest>;
+
     struct track
     {
-        const size_t announced;
-        const system::hash_digest last;
-        system::hashes hashes;
+        hashmap ids{};
+        size_t announced{};
+        system::hash_digest last{};
     };
 
-    typedef std::shared_ptr<track> track_ptr;
-
     /// Accept incoming inventory message.
     virtual bool handle_receive_inventory(const code& ec,
         const network::messages::inventory::cptr& message) NOEXCEPT;
 
     /// Accept incoming block message.
     virtual bool handle_receive_block(const code& ec,
-        const network::messages::block::cptr& message,
-        const track_ptr& tracker) NOEXCEPT;
-    virtual void complete() NOEXCEPT;
+        const network::messages::block::cptr& message) NOEXCEPT;
     virtual void handle_organize(const code& ec, size_t height,
         const system::chain::block::cptr& block_ptr) NOEXCEPT;
 
 private:
-    static system::hashes to_hashes(
+    static hashmap to_hashes(
         const network::messages::get_data& getter) NOEXCEPT;
 
     network::messages::get_blocks create_get_inventory() const NOEXCEPT;
@@ -84,6 +83,9 @@ class BCN_API protocol_block_in
     network::messages::get_data create_get_data(
         const network::messages::inventory& message) const NOEXCEPT;
 
+    // This is protected by strand.
+    track tracker_{};
+
     // This is thread safe.
     const network::messages::inventory::type_id block_type_;
 };

include/bitcoin/node/protocols/protocol_block_in_31800.hpp

@@ -42,10 +42,11 @@ class BCN_API protocol_block_in_31800
     BC_PUSH_WARNING(NO_THROW_IN_NOEXCEPT)
     template <typename Session>
     protocol_block_in_31800(Session& session,
-        const channel_ptr& channel, bool) NOEXCEPT
+        const channel_ptr& channel, bool performance) NOEXCEPT
       : node::protocol(session, channel),
         network::tracker<protocol_block_in_31800>(session.log),
-        drop_stalled_(to_bool(session.config().node.sample_period_seconds)),
+        drop_stalled_(performance && 
+            to_bool(session.config().node.sample_period_seconds)),
         use_deviation_(session.config().node.allowed_deviation > 0.0),
         block_type_(session.config().network.witness_node() ?
             type_id::witness_block : type_id::block),

include/bitcoin/node/sessions/attach.hpp

@@ -29,6 +29,8 @@
 namespace libbitcoin {
 namespace node {
 
+class session_outbound;
+
 /// Session base class template for protocol attachment.
 /// node::session does not derive from network::session (siblings).
 /// This avoids the diamond inheritance problem between network/node.
@@ -58,26 +60,34 @@ class attach
         const network::channel::ptr& channel) NOEXCEPT override
     {
         auto& self = *this;
+        const auto headers_first = config().node.headers_first;
         const auto version = channel->negotiated_version();
-        ////constexpr auto performance = false;
+
+        // Only session_outbound channels compete on performance.
+        auto performance = false;
+        if constexpr (is_same_type<Session, node::session_outbound>)
+        {
+            performance = true;
+        }
 
         // Attach appropriate alert, reject, ping, and/or address protocols.
         Session::attach_protocols(channel);
 
         // Very hard to find < 31800 peer to connect with.
-        if (version >= network::messages::level::bip130)
+        if (headers_first && version >= network::messages::level::bip130)
         {
             // Headers-first synchronization (parallel block download).
             channel->attach<protocol_header_in_70012>(self)->start();
             channel->attach<protocol_header_out_70012>(self)->start();
-            ////channel->attach<protocol_block_in_31800>(self, performance)->start();
+            channel->attach<protocol_block_in_31800>(self, performance)->start();
         }
-        else if (version >= network::messages::level::headers_protocol)
+        else if (headers_first && 
+            version >= network::messages::level::headers_protocol)
         {
             // Headers-first synchronization (parallel block download).
             channel->attach<protocol_header_in_31800>(self)->start();
             channel->attach<protocol_header_out_31800>(self)->start();
-            ////channel->attach<protocol_block_in_31800>(self, performance)->start();
+            channel->attach<protocol_block_in_31800>(self, performance)->start();
         }
         else
         {

include/bitcoin/node/sessions/session_outbound.hpp

@@ -46,9 +46,6 @@ class BCN_API session_outbound
         chaser::chase event_, chaser::link value) NOEXCEPT;
     virtual void split(chaser::channel_t channel) NOEXCEPT;
 
-    void attach_protocols(
-        const network::channel::ptr& channel) NOEXCEPT override;
-
 private:
     void do_performance(uint64_t channel, uint64_t speed,
         const network::result_handler& handler) NOEXCEPT;