Skip to content

Commit

Permalink
Merge pull request #726 from evoskuil/master
Browse files Browse the repository at this point in the history 
Don't fire events for bypassed validate/confirm, shrink initial point table.
  • Loading branch information
@evoskuil
evoskuil authored Feb 1, 2025
2 parents ba7fe2d + 53ca381 commit 669f38e
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 71 deletions.
4 changes: 2 additions & 2 deletions include/bitcoin/node/chasers/chaser_validate.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ class BCN_API chaser_validate
virtual code populate(bool bypass, const system::chain::block& block,
const system::chain::context& ctx) NOEXCEPT;
virtual void tracked_complete_block(const code& ec,
const database::header_link& link, size_t height) NOEXCEPT;
const database::header_link& link, size_t height, bool bypassed) NOEXCEPT;
virtual void complete_block(const code& ec,
const database::header_link& link, size_t height) NOEXCEPT;
const database::header_link& link, size_t height, bool bypassed) NOEXCEPT;

// Override base class strand because it sits on the network thread pool.
network::asio::strand& strand() NOEXCEPT override;
Expand Down
19 changes: 10 additions & 9 deletions src/chasers/chaser_confirm.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ void chaser_confirm::do_bump(height_t) NOEXCEPT
{
const auto link = query.to_candidate(height);
auto ec = query.get_block_state(link);
auto bypassed = true;

if (ec == database::error::unassociated)
{
Expand Down Expand Up @@ -224,13 +225,6 @@ 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());
Expand All @@ -249,6 +243,9 @@ void chaser_confirm::do_bump(height_t) NOEXCEPT
fault(error::confirm6);
return;
}

// Confirmable query executed successfully.
bypassed = false;
}
else if (ec == database::error::block_confirmable)
{
Expand Down Expand Up @@ -284,8 +281,12 @@ void chaser_confirm::do_bump(height_t) NOEXCEPT
set_position(height);

notify(error::success, chase::confirmable, height);
fire(events::block_confirmed, height);
////LOGV("Block confirmed: " << height);

if (!bypassed)
{
fire(events::block_confirmed, height);
////LOGV("Block confirmed: " << height);
}
}
}

Expand Down
27 changes: 13 additions & 14 deletions src/chasers/chaser_validate.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ void chaser_validate::do_bump(height_t) NOEXCEPT
}
else if (ec == database::error::block_unconfirmable)
{
complete_block(ec, link, height);
complete_block(ec, link, height, true);
return;
}

Expand All @@ -179,7 +179,7 @@ void chaser_validate::do_bump(height_t) NOEXCEPT

if (bypass && !filter_)
{
complete_block(database::error::success, link, height);
complete_block(database::error::success, link, height, true);
}
else
{
Expand Down Expand Up @@ -227,7 +227,7 @@ void chaser_validate::validate_block(const header_link& link,
}

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

code chaser_validate::populate(bool bypass, const chain::block& block,
Expand Down Expand Up @@ -279,16 +279,16 @@ code chaser_validate::validate(bool bypass, const chain::block& block,

// The size of the job is not relevant to the backlog cost.
void chaser_validate::tracked_complete_block(const code& ec,
const header_link& link, size_t height) NOEXCEPT
const header_link& link, size_t height, bool bypassed) NOEXCEPT
{
BC_ASSERT(stranded());

--backlog_;
complete_block(ec, link, height);
complete_block(ec, link, height, bypassed);
}

void chaser_validate::complete_block(const code& ec, const header_link& link,
size_t height) NOEXCEPT
size_t height, bool bypassed) NOEXCEPT
{
BC_ASSERT(stranded());

Expand All @@ -305,21 +305,20 @@ void chaser_validate::complete_block(const code& ec, const header_link& 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);
// ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
////fault(ec);

return;
}

notify(ec, chase::valid, possible_wide_cast<height_t>(height));
fire(events::block_validated, height);
////LOGV("Block validated: " << height);

if (!bypassed)
{
fire(events::block_validated, height);
////LOGV("Block validated: " << height);
}

// Prevent stall by posting internal event, avoid hitting external handlers.
if (is_zero(backlog_))
Expand Down
9 changes: 5 additions & 4 deletions src/parser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,9 @@ parser::parser(system::chain::selection context) NOEXCEPT
configured.database.output_size = 25'300'000'000;
configured.database.output_rate = 5;

configured.database.point_buckets = 1'750'905'073;
configured.database.point_size = 24'000'000'000;
// Full size too big for mini, so reduced to compressed size.
configured.database.point_buckets = 546'188'501;
configured.database.point_size = 8'389'074'978;
configured.database.point_rate = 5;

configured.database.puts_size = 6'300'000'000;
Expand Down Expand Up @@ -697,12 +698,12 @@ options_metadata parser::load_settings() THROWS
(
"database.point_buckets",
value<uint32_t>(&configured.database.point_buckets),
"The number of buckets in the point table head, defaults to '1750905073'."
"The number of buckets in the point table head, defaults to '546188501'."
)
(
"database.point_size",
value<uint64_t>(&configured.database.point_size),
"The minimum allocation of the point table body, defaults to '24000000000'."
"The minimum allocation of the point table body, defaults to '8389074978'."
)
(
"database.point_rate",
Expand Down
46 changes: 4 additions & 42 deletions src/protocols/protocol_block_in_31800.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,21 +304,6 @@ 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
Expand All @@ -330,11 +315,6 @@ 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() << ")"
Expand Down Expand Up @@ -363,18 +343,6 @@ 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))
{
Expand All @@ -385,17 +353,11 @@ 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));
Expand All @@ -411,10 +373,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())
Expand Down

0 comments on commit 669f38e

Please sign in to comment.