diff --git a/include/bitcoin/node/chasers/chaser_confirm.hpp b/include/bitcoin/node/chasers/chaser_confirm.hpp index f6ecbf5bc..14d67eec3 100644 --- a/include/bitcoin/node/chasers/chaser_confirm.hpp +++ b/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_; }; diff --git a/include/bitcoin/node/chasers/chaser_validate.hpp b/include/bitcoin/node/chasers/chaser_validate.hpp index 6a684d212..d6dc0f066 100644 --- a/include/bitcoin/node/chasers/chaser_validate.hpp +++ b/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_; }; diff --git a/include/bitcoin/node/error.hpp b/include/bitcoin/node/error.hpp index cce33949c..9b17a7f89 100644 --- a/include/bitcoin/node/error.hpp +++ b/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, diff --git a/include/bitcoin/node/settings.hpp b/include/bitcoin/node/settings.hpp index cdcafa337..17ab8a2d0 100644 --- a/include/bitcoin/node/settings.hpp +++ b/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; diff --git a/src/chasers/chaser_check.cpp b/src/chasers/chaser_check.cpp index 610d1d9f5..f380b75bb 100644 --- a/src/chasers/chaser_check.cpp +++ b/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. diff --git a/src/chasers/chaser_confirm.cpp b/src/chasers/chaser_confirm.cpp index f266f8b23..bceae085a 100644 --- a/src/chasers/chaser_confirm.cpp +++ b/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(value)); //// - //// if (concurrent_ || mature_) - //// { - //// // TODO: value is branch point. - //// POST(do_validated, std::get(value)); - //// } - //// + //// // TODO: value is branch point. + //// POST(do_validated, std::get(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(value)); - - if (concurrent_ || mature_) - { - POST(do_validated, std::get(value)); - } - + POST(do_validated, std::get(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()); diff --git a/src/chasers/chaser_validate.cpp b/src/chasers/chaser_validate.cpp index 94d5bcc55..debfbf9b6 100644 --- a/src/chasers/chaser_validate.cpp +++ b/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(value)); - - if (concurrent_ || mature_) - { - POST(do_checked, std::get(value)); - } - + POST(do_checked, std::get(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; } diff --git a/src/error.cpp b/src/error.cpp index 7a986704a..a681c2e3d 100644 --- a/src/error.cpp +++ b/src/error.cpp @@ -50,6 +50,7 @@ DEFINE_ERROR_T_MESSAGE_MAP(error) /// faults { protocol1, "protocol1" }, + { protocol2, "protocol2" }, { header1, "header1" }, { organize1, "organize1" }, { organize2, "organize2" }, diff --git a/src/parser.cpp b/src/parser.cpp index a7f95e375..3e33f11e0 100644 --- a/src/parser.cpp +++ b/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(&configured.node.priority_validation), - "Set the validation threadpool to high priority, defaults to false." - ) - ( - "node.concurrent_validation", - value(&configured.node.concurrent_validation), - "Perform validation concurrently with download, defaults to false." - ) - ( - "node.concurrent_confirmation", - value(&configured.node.concurrent_confirmation), - "Perform confirmation concurrently with download, defaults to false." + "node.priority", + value(&configured.node.priority), + "Set the validation threadpool to high priority, defaults to true." ) ( "node.headers_first", diff --git a/src/protocols/protocol_block_in_31800.cpp b/src/protocols/protocol_block_in_31800.cpp index 6ec54fed6..428273b1a 100644 --- a/src/protocols/protocol_block_in_31800.cpp +++ b/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()) diff --git a/src/settings.cpp b/src/settings.cpp index c594c78dd..da5693777 100644 --- a/src/settings.cpp +++ b/src/settings.cpp @@ -75,10 +75,8 @@ std::filesystem::path settings::events_file() const NOEXCEPT namespace node { settings::settings() NOEXCEPT - : headers_first{ true }, - priority_validation{ false }, - concurrent_validation{ false }, - concurrent_confirmation{ false }, + : priority{ true }, + headers_first{ true }, allowed_deviation{ 1.5 }, allocation_multiple{ 20 }, snapshot_bytes{ 200'000'000'000 }, @@ -125,7 +123,7 @@ network::wall_clock::duration settings::currency_window() const NOEXCEPT network::thread_priority settings::priority_() const NOEXCEPT { - return priority_validation ? network::thread_priority::high : + return priority ? network::thread_priority::high : network::thread_priority::normal; } diff --git a/test/settings.cpp b/test/settings.cpp index 9a7c350a7..9cfec95fb 100644 --- a/test/settings.cpp +++ b/test/settings.cpp @@ -54,10 +54,8 @@ BOOST_AUTO_TEST_CASE(settings__node__default_context__expected) using namespace network; const node::settings node{}; + BOOST_REQUIRE_EQUAL(node.priority, true); BOOST_REQUIRE_EQUAL(node.headers_first, true); - BOOST_REQUIRE_EQUAL(node.priority_validation, false); - BOOST_REQUIRE_EQUAL(node.concurrent_validation, false); - BOOST_REQUIRE_EQUAL(node.concurrent_confirmation, false); BOOST_REQUIRE_EQUAL(node.allowed_deviation, 1.5); BOOST_REQUIRE_EQUAL(node.maximum_height, 0_u32); BOOST_REQUIRE_EQUAL(node.allocation_multiple, 20_u16); @@ -76,7 +74,7 @@ BOOST_AUTO_TEST_CASE(settings__node__default_context__expected) BOOST_REQUIRE_EQUAL(node.maximum_concurrency_(), 50'000_size); BOOST_REQUIRE(node.sample_period() == steady_clock::duration(seconds(10))); BOOST_REQUIRE(node.currency_window() == steady_clock::duration(minutes(60))); - BOOST_REQUIRE(node.priority_() == network::thread_priority::normal); + BOOST_REQUIRE(node.priority_() == network::thread_priority::high); } BOOST_AUTO_TEST_SUITE_END()