highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-20 23:29:12 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#33078: kernel: improve BlockChecked ownership semantics

Browse Source 
1d9f1cb4bd kernel: improve BlockChecked ownership semantics (stickies-v)
9ba1fff29e kernel: refactor: ConnectTip to pass block pointer by value (stickies-v)

Pull request description:

  Subscribers to the BlockChecked validation interface event may need access to the block outside of the callback scope. Currently, this is only possible by copying the block, which makes exposing this validation interface event publicly either cumbersome or with significant copy overhead.

  By using shared_ptr, we make the shared ownership explicit and allow users to safely use the block outside of the callback scope. By using a const-ref shared_ptr, no atomic reference count cost is incurred if a subscriber does not require block ownership.

  For example: in  #30595, this would allow us to drop the `kernel_BlockPointer` handle entirely, and generalize everything into `kernel_Block`. This PoC is implemented in https://github.com/stickies-v/bitcoin/commits/kernel/remove-blockpointer/.

  ---

  ### Performance

  I have added a benchmark in a [separate branch](https://github.com/stickies-v/bitcoin/commits/2025-07/validation-interface-ownership-benched/), to ensure this change does not lead to a problematic performance regression. Since most of the overhead comes from the subscribers, I have added scenarios for `One`, `Two`, and `Ten` subscribers. From these results, it appears there is no meaningful performance difference on my machine.

  When `BlockChecked()` takes a `const CBlock&` reference _(master)_:
  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |              170.09 |        5,879,308.26 |    0.3% |      0.01 | `BlockCheckedOne`
  |            1,603.95 |          623,460.10 |    0.5% |      0.01 | `BlockCheckedTen`
  |              336.00 |        2,976,173.37 |    1.1% |      0.01 | `BlockCheckedTwo`

  When `BlockChecked()` takes a `const std::shared_ptr<const CBlock>&` _(this PR)_:
  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |              172.20 |        5,807,155.33 |    0.1% |      0.01 | `BlockCheckedOne`
  |            1,596.79 |          626,254.52 |    0.0% |      0.01 | `BlockCheckedTen`
  |              333.38 |        2,999,603.17 |    0.3% |      0.01 | `BlockCheckedTwo`

ACKs for top commit:
  achow101:
    ACK 1d9f1cb4bd
  w0xlt:
    reACK 1d9f1cb4bd
  ryanofsky:
    Code review ACK 1d9f1cb4bd. These all seem like simple changes that make sense
  TheCharlatan:
    ACK 1d9f1cb4bd
  yuvicc:
    Code Review ACK 1d9f1cb4bd

Tree-SHA512: 7ed0cccb7883dbb1885917ef749ab7cae5d60ee803b7e3145b2954d885e81ba8c9d5ab1edb9694ce6b308235c621094c029024eaf99f1aab1b47311c40958095
This commit is contained in:
Ava Chow
2025-08-20 10:45:36 -07:00
parent bc797d2271 1d9f1cb4bd
commit 04c115dfde
9 changed files with 42 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
src/bitcoin-chainstate.cpp
View File

@@ -200,9 +200,9 @@ int main(int argc, char* argv[])
explicit submitblock_StateCatcher(const uint256& hashIn) : hash(hashIn), found(false), state() {}
protected:
void BlockChecked(const CBlock& block, const BlockValidationState& stateIn) override
void BlockChecked(const std::shared_ptr<const CBlock>& block, const BlockValidationState& stateIn) override
{
if (block.GetHash() != hash)
if (block->GetHash() != hash)
return;
found = true;
state = stateIn;

6
src/net_processing.cpp
View File

@@ -513,7 +513,7 @@ public:
EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex);
void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
void BlockChecked(const CBlock& block, const BlockValidationState& state) override
void BlockChecked(const std::shared_ptr<const CBlock>& block, const BlockValidationState& state) override
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock) override
EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex);
@@ -2071,11 +2071,11 @@ void PeerManagerImpl::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlock
* Handle invalid block rejection and consequent peer discouragement, maintain which
* peers announce compact blocks.
*/
void PeerManagerImpl::BlockChecked(const CBlock& block, const BlockValidationState& state)
void PeerManagerImpl::BlockChecked(const std::shared_ptr<const CBlock>& block, const BlockValidationState& state)
{
LOCK(cs_main);
const uint256 hash(block.GetHash());
const uint256 hash(block->GetHash());
std::map<uint256, std::pair<NodeId, bool>>::iterator it = mapBlockSource.find(hash);
// If the block failed validation, we know where it came from and we're still connected

6
src/rpc/mining.cpp
View File

@@ -1030,9 +1030,9 @@ public:
explicit submitblock_StateCatcher(const uint256 &hashIn) : hash(hashIn), state() {}
protected:
void BlockChecked(const CBlock& block, const BlockValidationState& stateIn) override {
if (block.GetHash() != hash)
return;
void BlockChecked(const std::shared_ptr<const CBlock>& block, const BlockValidationState& stateIn) override
{
if (block->GetHash() != hash) return;
found = true;
state = stateIn;
}

5
src/test/util/mining.cpp
View File

@@ -18,6 +18,7 @@
#include <versionbits.h>
#include <algorithm>
#include <memory>
using node::BlockAssembler;
using node::NodeContext;
@@ -83,9 +84,9 @@ struct BlockValidationStateCatcher : public CValidationInterface {
m_state{} {}
protected:
void BlockChecked(const CBlock& block, const BlockValidationState& state) override
void BlockChecked(const std::shared_ptr<const CBlock>& block, const BlockValidationState& state) override
{
if (block.GetHash() != m_hash) return;
if (block->GetHash() != m_hash) return;
m_state = state;
}
};

11
src/test/validationinterface_tests.cpp
View File

@@ -12,11 +12,12 @@
#include <validationinterface.h>
#include <atomic>
#include <memory>
BOOST_FIXTURE_TEST_SUITE(validationinterface_tests, ChainTestingSetup)
struct TestSubscriberNoop final : public CValidationInterface {
void BlockChecked(const CBlock&, const BlockValidationState&) override {}
void BlockChecked(const std::shared_ptr<const CBlock>&, const BlockValidationState&) override {}
};
BOOST_AUTO_TEST_CASE(unregister_validation_interface_race)
@@ -25,10 +26,9 @@ BOOST_AUTO_TEST_CASE(unregister_validation_interface_race)
// Start thread to generate notifications
std::thread gen{[&] {
const CBlock block_dummy;
BlockValidationState state_dummy;
while (generate) {
m_node.validation_signals->BlockChecked(block_dummy, state_dummy);
m_node.validation_signals->BlockChecked(std::make_shared<const CBlock>(), state_dummy);
}
}};
@@ -60,15 +60,14 @@ public:
{
if (m_on_destroy) m_on_destroy();
}
void BlockChecked(const CBlock& block, const BlockValidationState& state) override
void BlockChecked(const std::shared_ptr<const CBlock>& block, const BlockValidationState& state) override
{
if (m_on_call) m_on_call();
}
void Call()
{
CBlock block;
BlockValidationState state;
m_signals.BlockChecked(block, state);
m_signals.BlockChecked(std::make_shared<const CBlock>(), state);
}
std::function<void()> m_on_call;
std::function<void()> m_on_destroy;

26
src/validation.cpp
View File

@@ -3108,7 +3108,12 @@ public:
*
* The block is added to connectTrace if connection succeeds.
*/
bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool)
bool Chainstate::ConnectTip(
BlockValidationState& state,
CBlockIndex* pindexNew,
std::shared_ptr<const CBlock> block_to_connect,
ConnectTrace& connectTrace,
DisconnectedBlockTransactions& disconnectpool)
{
AssertLockHeld(cs_main);
if (m_mempool) AssertLockHeld(m_mempool->cs);
@@ -3116,18 +3121,15 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew,
assert(pindexNew->pprev == m_chain.Tip());
// Read block from disk.
const auto time_1{SteadyClock::now()};
std::shared_ptr<const CBlock> pthisBlock;
if (!pblock) {
if (!block_to_connect) {
std::shared_ptr<CBlock> pblockNew = std::make_shared<CBlock>();
if (!m_blockman.ReadBlock(*pblockNew, *pindexNew)) {
return FatalError(m_chainman.GetNotifications(), state, _("Failed to read block."));
}
pthisBlock = pblockNew;
block_to_connect = std::move(pblockNew);
} else {
LogDebug(BCLog::BENCH, " - Using cached block\n");
pthisBlock = pblock;
}
const CBlock& blockConnecting = *pthisBlock;
// Apply the block atomically to the chain state.
const auto time_2{SteadyClock::now()};
SteadyClock::time_point time_3;
@@ -3137,9 +3139,9 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew,
Ticks<MillisecondsDouble>(time_2 - time_1));
{
CCoinsViewCache view(&CoinsTip());
bool rv = ConnectBlock(blockConnecting, state, pindexNew, view);
bool rv = ConnectBlock(*block_to_connect, state, pindexNew, view);
if (m_chainman.m_options.signals) {
m_chainman.m_options.signals->BlockChecked(blockConnecting, state);
m_chainman.m_options.signals->BlockChecked(block_to_connect, state);
}
if (!rv) {
if (state.IsInvalid())
@@ -3175,8 +3177,8 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew,
Ticks<MillisecondsDouble>(m_chainman.time_chainstate) / m_chainman.num_blocks_total);
// Remove conflicting transactions from the mempool.;
if (m_mempool) {
m_mempool->removeForBlock(blockConnecting.vtx, pindexNew->nHeight);
disconnectpool.removeForBlock(blockConnecting.vtx);
m_mempool->removeForBlock(block_to_connect->vtx, pindexNew->nHeight);
disconnectpool.removeForBlock(block_to_connect->vtx);
}
// Update m_chain & related variables.
m_chain.SetTip(*pindexNew);
@@ -3202,7 +3204,7 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew,
m_chainman.MaybeCompleteSnapshotValidation();
}
connectTrace.BlockConnected(pindexNew, std::move(pthisBlock));
connectTrace.BlockConnected(pindexNew, std::move(block_to_connect));
return true;
}
@@ -4515,7 +4517,7 @@ bool ChainstateManager::ProcessNewBlock(const std::shared_ptr<const CBlock>& blo
}
if (!ret) {
if (m_options.signals) {
m_options.signals->BlockChecked(*block, state);
m_options.signals->BlockChecked(block, state);
}
LogError("%s: AcceptBlock FAILED (%s)\n", __func__, state.ToString());
return false;

7
src/validation.h
View File

@@ -799,7 +799,12 @@ public:
protected:
bool ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
bool ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
bool ConnectTip(
BlockValidationState& state,
CBlockIndex* pindexNew,
std::shared_ptr<const CBlock> block_to_connect,
ConnectTrace& connectTrace,
DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
void InvalidBlockFound(CBlockIndex* pindex, const BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
CBlockIndex* FindMostWorkChain() EXCLUSIVE_LOCKS_REQUIRED(cs_main);

6
src/validationinterface.cpp
View File

@@ -17,6 +17,7 @@
#include <util/task_runner.h>
#include <future>
#include <memory>
#include <unordered_map>
#include <utility>
@@ -245,9 +246,10 @@ void ValidationSignals::ChainStateFlushed(ChainstateRole role, const CBlockLocat
locator.IsNull() ? "null" : locator.vHave.front().ToString());
}
void ValidationSignals::BlockChecked(const CBlock& block, const BlockValidationState& state) {
void ValidationSignals::BlockChecked(const std::shared_ptr<const CBlock>& block, const BlockValidationState& state)
{
LOG_EVENT("%s: block hash=%s state=%s", __func__,
block.GetHash().ToString(), state.ToString());
block->GetHash().ToString(), state.ToString());
m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockChecked(block, state); });
}

4
src/validationinterface.h
View File

@@ -150,7 +150,7 @@ protected:
* is guaranteed to be the current best block at the time the
* callback was generated (not necessarily now).
*/
virtual void BlockChecked(const CBlock&, const BlockValidationState&) {}
virtual void BlockChecked(const std::shared_ptr<const CBlock>&, const BlockValidationState&) {}
/**
* Notifies listeners that a block which builds directly on our current tip
* has been received and connected to the headers tree, though not validated yet.
@@ -224,7 +224,7 @@ public:
void BlockConnected(ChainstateRole, const std::shared_ptr<const CBlock> &, const CBlockIndex *pindex);
void BlockDisconnected(const std::shared_ptr<const CBlock> &, const CBlockIndex* pindex);
void ChainStateFlushed(ChainstateRole, const CBlockLocator &);
void BlockChecked(const CBlock&, const BlockValidationState&);
void BlockChecked(const std::shared_ptr<const CBlock>&, const BlockValidationState&);
void NewPoWValidBlock(const CBlockIndex *, const std::shared_ptr<const CBlock>&);
};