Skip to content

Commit

Permalink
Merge pull request #707 from evoskuil/master
Browse files Browse the repository at this point in the history 
Fix validate/confirm threadpool/strand initialization order.
  • Loading branch information
@evoskuil
evoskuil authored Jan 5, 2025
2 parents aec9f18 + 557ee45 commit a84e314
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 94 deletions.
4 changes: 2 additions & 2 deletions console/executor_test_reader.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ void executor::read_test(bool dump) const
if (cancel_)
return;

size_t found{};
////size_t found{};
auto address_it = store_.address.it(key);
if (address_it.self().is_terminal())
{
Expand Down Expand Up @@ -179,7 +179,7 @@ void executor::read_test(bool dump) const
sp_tx_fk = spend.parent_fk;
}

++found;
////++found;
outs.push_back(out
{
key,
Expand Down
14 changes: 7 additions & 7 deletions include/bitcoin/node/chasers/chaser_confirm.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,15 @@ class BCN_API chaser_confirm
bool get_is_strong(bool& strong, const uint256_t& fork_work,
size_t fork_point) const NOEXCEPT;

// These are thread safe.
const bool concurrent_;
network::asio::strand independent_strand_;

// These are protected by strand.
network::threadpool threadpool_;
neutrino_header neutrino_{};
bool filters_{};
bool mature_{};
bool filters_{};
neutrino_header neutrino_{};
network::threadpool threadpool_;

// These are thread safe.
network::asio::strand independent_strand_;
const bool concurrent_;
};

} // namespace node
Expand Down
23 changes: 8 additions & 15 deletions include/bitcoin/node/chasers/chaser_validate.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,24 +66,17 @@ class BCN_API chaser_validate
return backlog_ < maximum_backlog_;
}

bool set_neutrino(const database::header_link& link,
const system::chain::block& block) NOEXCEPT;

bool set_prevouts(size_t height,
const system::chain::block& block) NOEXCEPT;
// These are protected by strand.
bool mature_{};
size_t backlog_{};
network::threadpool threadpool_;

// These are thread safe.
const bool concurrent_;
const size_t maximum_backlog_;
const uint64_t initial_subsidy_;
const uint32_t subsidy_interval_;
network::asio::strand independent_strand_;

// These are protected by strand.
network::threadpool threadpool_;
size_t backlog_{};
bool filters_{};
bool mature_{};
const uint32_t subsidy_interval_;
const uint64_t initial_subsidy_;
const size_t maximum_backlog_;
const bool concurrent_;
};

} // namespace node
Expand Down
21 changes: 14 additions & 7 deletions src/chasers/chaser_confirm.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ BC_PUSH_WARNING(NO_THROW_IN_NOEXCEPT)
// Higher priority than validator ensures locality to validator reads.
chaser_confirm::chaser_confirm(full_node& node) NOEXCEPT
: chaser(node),
concurrent_(node.config().node.concurrent_confirmation),
threadpool_(one, node.config().node.priority_()),
independent_strand_(threadpool_.service().get_executor())
independent_strand_(threadpool_.service().get_executor()),
concurrent_(node.config().node.concurrent_confirmation)
{
}

Expand Down Expand Up @@ -207,8 +207,10 @@ void chaser_confirm::do_bump(height_t) NOEXCEPT
fault(error::confirm2);
return;
}


/////////////////////////////////////////
// Confirmation query.
/////////////////////////////////////////
if ((ec = query.block_confirmable(link)))
{
if (ec == database::error::integrity)
Expand Down Expand Up @@ -243,6 +245,12 @@ void chaser_confirm::do_bump(height_t) NOEXCEPT
fault(error::confirm6);
return;
}

if (!set_organized(link, height))
{
fault(error::confirm7);
return;
}

notify(error::success, chase::confirmable, height);
fire(events::block_confirmed, height);
Expand All @@ -253,7 +261,7 @@ void chaser_confirm::do_bump(height_t) NOEXCEPT
{
if (!query.set_strong(link))
{
fault(error::confirm7);
fault(error::confirm8);
return;
}

Expand All @@ -271,7 +279,7 @@ void chaser_confirm::do_bump(height_t) NOEXCEPT
// database::error::unknown_state [shouldn't be here]
// database::error::unassociated [shouldn't be here]
// database::error::unvalidated [shouldn't be here]
fault(error::confirm8);
fault(error::confirm9);
return;
}

Expand All @@ -286,7 +294,7 @@ void chaser_confirm::do_bump(height_t) NOEXCEPT
// first) here. Candidate chain reorganizations will result in reported heights
// moving in any direction. Each is treated as independent and only one
// representing a stronger chain is considered.

////
////// Compute relative work, set fork and fork_point, and invoke reorganize.
////void chaser_confirm::do_validated(height_t height) NOEXCEPT
////{
Expand Down Expand Up @@ -613,7 +621,6 @@ bool chaser_confirm::get_is_strong(bool& strong, const uint256_t& fork_work,
// neutrino
// ----------------------------------------------------------------------------

// This can only fail if prevouts are not fully populated.
bool chaser_confirm::update_neutrino(const header_link& link) NOEXCEPT
{
// neutrino_.link is only used for this assertion, should compile away.
Expand Down
102 changes: 39 additions & 63 deletions src/chasers/chaser_validate.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,19 +43,18 @@ BC_PUSH_WARNING(NO_THROW_IN_NOEXCEPT)
// Higher priority than downloader (net) ensures locality to downloader writes.
chaser_validate::chaser_validate(full_node& node) NOEXCEPT
: chaser(node),
concurrent_(node.config().node.concurrent_validation),
maximum_backlog_(node.config().node.maximum_concurrency_()),
initial_subsidy_(node.config().bitcoin.initial_subsidy()),
subsidy_interval_(node.config().bitcoin.subsidy_interval_blocks),
threadpool_(node.config().node.threads_(), node.config().node.priority_()),
independent_strand_(threadpool_.service().get_executor())
independent_strand_(threadpool_.service().get_executor()),
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)
{
}

code chaser_validate::start() NOEXCEPT
{
const auto& query = archive();
filters_ = query.neutrino_enabled();
set_position(query.get_fork());
SUBSCRIBE_EVENTS(handle_event, _1, _2, _3);
return error::success;
Expand Down Expand Up @@ -200,61 +199,61 @@ void chaser_validate::do_bump(height_t) NOEXCEPT
}
}

// unstranded (concurrent by block)
// Unstranded (concurrent by block).
void chaser_validate::validate_block(const header_link& link) NOEXCEPT
{
if (closed())
return;

code ec{};
chain::context ctx{};
auto& query = archive();
const auto block = query.get_block(link);

if (!block)
{
POST(complete_block, error::validate1, link, zero);
return;
ec = error::validate1;
}

chain::context ctx{};
if (!query.get_context(ctx, link))
else if (!query.get_context(ctx, link))
{
POST(complete_block, error::validate2, link, zero);
return;
ec = error::validate2;
}

if (!block->populate())
else if (!block->populate())
{
// input.metadata.locked set invalid for relative locktime (bip68).
// Otherwise internal spends do not require confirmability checks.
POST(complete_block, error::validate3, link, ctx.height);
return;
ec = system::error::relative_time_locked;
}
else if (!query.populate(*block))
{
// Prepopulated prevouts are bypassed here, including metadata.
// .input.metadata.parent (tx link) and .coinbase (bool) are set here.
POST(complete_block, error::validate4, link, ctx.height);
return;
ec = system::error::missing_previous_output;
}

code ec{};
if (((ec = block->accept(ctx, subsidy_interval_, initial_subsidy_))) ||
((ec = block->connect(ctx))))
else if ((ec = block->accept(ctx, subsidy_interval_, initial_subsidy_)))
{
if (!query.set_block_unconfirmable(link))
ec = error::validate3;
}
else if ((ec = block->connect(ctx)))
{
if (!query.set_block_unconfirmable(link))
ec = error::validate5;
ec = error::validate4;
}
else if (!query.set_block_valid(link, block->fees()))
{
ec = error::validate5;
}
else if (!query.set_prevouts(ctx.height, *block))
{
ec = error::validate6;
}
else if (query.set_filter_body(link, *block))
{
ec = error::validate7;
}
else
{
set_neutrino(link, *block);
set_prevouts(ctx.height, *block);

fire(events::block_validated, ctx.height);
}

// Return to strand to handle result.
POST(complete_block, ec, link, ctx.height);
}

Expand All @@ -268,7 +267,14 @@ void chaser_validate::complete_block(const code& ec, const header_link& link,

if (ec)
{
if (ec == error::validate7)
// Differentiated fault codes for troubleshooting.
if (ec == error::validate1 ||
ec == error::validate2 ||
ec == error::validate3 ||
ec == error::validate4 ||
ec == error::validate5 ||
ec == error::validate6 ||
ec == error::validate7)
{
fault(ec);
return;
Expand All @@ -287,36 +293,6 @@ void chaser_validate::complete_block(const code& ec, const header_link& link,
handle_event(ec, chase::bump, height_t{});
}

// setters
// ----------------------------------------------------------------------------
// unstranded (concurrent by block)

bool chaser_validate::set_neutrino(const header_link& link,
const chain::block& block) NOEXCEPT
{
if (!filters_)
return true;

// Avoid computing the filter if already stored.
auto& query = archive();
if (!query.to_filter(link).is_terminal())
return true;

// Only fails if prevouts are not fully populated.
data_chunk filter{};
if (!neutrino::compute_filter(filter, block))
return false;

return query.set_filter_body(link, filter);
}

bool chaser_validate::set_prevouts(size_t, const chain::block&) NOEXCEPT
{
// TODO: need to be able to differentiate internal vs. store.
// This tells us what to cache, skip internal and set store populated.
return {};
}

// Strand.
// ----------------------------------------------------------------------------

Expand Down

0 comments on commit a84e314

Please sign in to comment.