Skip to content

Commit 92c8e18

Browse files
committed
Merge bitcoin/bitcoin#25494: indexes: Stop using node internal types
7878f97 indexes, refactor: Remove CChainState use in index CommitInternal method (Ryan Ofsky) ee3a079 indexes, refactor: Remove CBlockIndex* uses in index Rewind methods (Ryan Ofsky) dc971be indexes, refactor: Remove CBlockIndex* uses in index WriteBlock methods (Ryan Ofsky) bef4e40 indexes, refactor: Remove CBlockIndex* uses in index Init methods (Ryan Ofsky) addb4f2 indexes, refactor: Remove CBlockIndex* uses in coinstatsindex LookUpOne function (Ryan Ofsky) 33b4d48 indexes, refactor: Pass Chain interface instead of CChainState class to indexes (Ryan Ofsky) a0b5b4a interfaces, refactor: Add more block information to block connected notifications (Ryan Ofsky) Pull request description: Start transitioning index code away from using internal node types like `CBlockIndex` and `CChain` so index code is less coupled to node code and index code will later be able to stop locking cs_main and sync without having to deal with validationinterface race conditions, and so new indexes are easier to write and can run as plugins or separate processes. This PR contains the first 7 commits from bitcoin/bitcoin#24230 (comment) which have been split off for easier review. Previous review comments can be found in #24230 ACKs for top commit: MarcoFalke: ACK 7878f97 though did not review the last commit 🤼 mzumsande: Code Review ACK 7878f97 Tree-SHA512: f84ac2eb6dca2c305566ddeb35ea14d0b71c00860c0fd752bbcf1a0188be833d8c2a6ac9d3ef6ab5b46fbd02d7a24cbb8f60cf12464cb8ba208e22287f709989
2 parents 6900162 + 7878f97 commit 92c8e18

21 files changed

+273
-142
lines changed

src/Makefile.am

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ BITCOIN_CORE_H = \
171171
interfaces/ipc.h \
172172
interfaces/node.h \
173173
interfaces/wallet.h \
174+
kernel/chain.h \
174175
kernel/chainstatemanager_opts.h \
175176
kernel/checks.h \
176177
kernel/coinstats.h \
@@ -365,6 +366,7 @@ libbitcoin_node_a_SOURCES = \
365366
index/coinstatsindex.cpp \
366367
index/txindex.cpp \
367368
init.cpp \
369+
kernel/chain.cpp \
368370
kernel/checks.cpp \
369371
kernel/coinstats.cpp \
370372
kernel/context.cpp \
@@ -882,6 +884,7 @@ libbitcoinkernel_la_SOURCES = \
882884
flatfile.cpp \
883885
fs.cpp \
884886
hash.cpp \
887+
kernel/chain.cpp \
885888
kernel/checks.cpp \
886889
kernel/coinstats.cpp \
887890
kernel/context.cpp \

src/index/base.cpp

Lines changed: 44 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,10 @@
44

55
#include <chainparams.h>
66
#include <index/base.h>
7+
#include <interfaces/chain.h>
8+
#include <kernel/chain.h>
79
#include <node/blockstorage.h>
10+
#include <node/context.h>
811
#include <node/interface_ui.h>
912
#include <shutdown.h>
1013
#include <tinyformat.h>
@@ -31,6 +34,15 @@ static void FatalError(const char* fmt, const Args&... args)
3134
StartShutdown();
3235
}
3336

37+
CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash)
38+
{
39+
CBlockLocator locator;
40+
bool found = chain.findBlock(block_hash, interfaces::FoundBlock().locator(locator));
41+
assert(found);
42+
assert(!locator.IsNull());
43+
return locator;
44+
}
45+
3446
BaseIndex::DB::DB(const fs::path& path, size_t n_cache_size, bool f_memory, bool f_wipe, bool f_obfuscate) :
3547
CDBWrapper(path, n_cache_size, f_memory, f_wipe, f_obfuscate)
3648
{}
@@ -49,6 +61,9 @@ void BaseIndex::DB::WriteBestBlock(CDBBatch& batch, const CBlockLocator& locator
4961
batch.Write(DB_BEST_BLOCK, locator);
5062
}
5163

64+
BaseIndex::BaseIndex(std::unique_ptr<interfaces::Chain> chain)
65+
: m_chain{std::move(chain)} {}
66+
5267
BaseIndex::~BaseIndex()
5368
{
5469
Interrupt();
@@ -175,12 +190,15 @@ void BaseIndex::ThreadSync()
175190
}
176191

177192
CBlock block;
193+
interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex);
178194
if (!ReadBlockFromDisk(block, pindex, consensus_params)) {
179195
FatalError("%s: Failed to read block %s from disk",
180196
__func__, pindex->GetBlockHash().ToString());
181197
return;
198+
} else {
199+
block_info.data = &block;
182200
}
183-
if (!WriteBlock(block, pindex)) {
201+
if (!CustomAppend(block_info)) {
184202
FatalError("%s: Failed to write block %s to index database",
185203
__func__, pindex->GetBlockHash().ToString());
186204
return;
@@ -197,22 +215,20 @@ void BaseIndex::ThreadSync()
197215

198216
bool BaseIndex::Commit()
199217
{
200-
CDBBatch batch(GetDB());
201-
if (!CommitInternal(batch) || !GetDB().WriteBatch(batch)) {
202-
return error("%s: Failed to commit latest %s state", __func__, GetName());
203-
}
204-
return true;
205-
}
206-
207-
bool BaseIndex::CommitInternal(CDBBatch& batch)
208-
{
209-
LOCK(cs_main);
210218
// Don't commit anything if we haven't indexed any block yet
211219
// (this could happen if init is interrupted).
212-
if (m_best_block_index == nullptr) {
213-
return false;
220+
bool ok = m_best_block_index != nullptr;
221+
if (ok) {
222+
CDBBatch batch(GetDB());
223+
ok = CustomCommit(batch);
224+
if (ok) {
225+
GetDB().WriteBestBlock(batch, GetLocator(*m_chain, m_best_block_index.load()->GetBlockHash()));
226+
ok = GetDB().WriteBatch(batch);
227+
}
228+
}
229+
if (!ok) {
230+
return error("%s: Failed to commit latest %s state", __func__, GetName());
214231
}
215-
GetDB().WriteBestBlock(batch, m_chainstate->m_chain.GetLocator(m_best_block_index));
216232
return true;
217233
}
218234

@@ -221,6 +237,10 @@ bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_ti
221237
assert(current_tip == m_best_block_index);
222238
assert(current_tip->GetAncestor(new_tip->nHeight) == new_tip);
223239

240+
if (!CustomRewind({current_tip->GetBlockHash(), current_tip->nHeight}, {new_tip->GetBlockHash(), new_tip->nHeight})) {
241+
return false;
242+
}
243+
224244
// In the case of a reorg, ensure persisted block locator is not stale.
225245
// Pruning has a minimum of 288 blocks-to-keep and getting the index
226246
// out of sync may be possible but a users fault.
@@ -268,8 +288,8 @@ void BaseIndex::BlockConnected(const std::shared_ptr<const CBlock>& block, const
268288
return;
269289
}
270290
}
271-
272-
if (WriteBlock(*block, pindex)) {
291+
interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex, block.get());
292+
if (CustomAppend(block_info)) {
273293
SetBestBlockIndex(pindex);
274294
} else {
275295
FatalError("%s: Failed to write block %s to index",
@@ -346,13 +366,18 @@ void BaseIndex::Interrupt()
346366
m_interrupt();
347367
}
348368

349-
bool BaseIndex::Start(CChainState& active_chainstate)
369+
bool BaseIndex::Start()
350370
{
351-
m_chainstate = &active_chainstate;
371+
// m_chainstate member gives indexing code access to node internals. It is
372+
// removed in followup https://github.com/bitcoin/bitcoin/pull/24230
373+
m_chainstate = &m_chain->context()->chainman->ActiveChainstate();
352374
// Need to register this ValidationInterface before running Init(), so that
353375
// callbacks are not missed if Init sets m_synced to true.
354376
RegisterValidationInterface(this);
355-
if (!Init()) {
377+
if (!Init()) return false;
378+
379+
const CBlockIndex* index = m_best_block_index.load();
380+
if (!CustomInit(index ? std::make_optional(interfaces::BlockKey{index->GetBlockHash(), index->nHeight}) : std::nullopt)) {
356381
return false;
357382
}
358383

src/index/base.h

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,16 @@
66
#define BITCOIN_INDEX_BASE_H
77

88
#include <dbwrapper.h>
9+
#include <interfaces/chain.h>
910
#include <threadinterrupt.h>
1011
#include <validationinterface.h>
1112

1213
class CBlock;
1314
class CBlockIndex;
1415
class CChainState;
16+
namespace interfaces {
17+
class Chain;
18+
} // namespace interfaces
1519

1620
struct IndexSummary {
1721
std::string name;
@@ -59,6 +63,9 @@ class BaseIndex : public CValidationInterface
5963
std::thread m_thread_sync;
6064
CThreadInterrupt m_interrupt;
6165

66+
/// Read best block locator and check that data needed to sync has not been pruned.
67+
bool Init();
68+
6269
/// Sync the index with the block index starting from the current best block.
6370
/// Intended to be run in its own thread, m_thread_sync, and can be
6471
/// interrupted with m_interrupt. Once the index gets in sync, the m_synced
@@ -76,30 +83,32 @@ class BaseIndex : public CValidationInterface
7683
/// getting corrupted.
7784
bool Commit();
7885

86+
/// Loop over disconnected blocks and call CustomRewind.
87+
bool Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip);
88+
7989
virtual bool AllowPrune() const = 0;
8090

8191
protected:
92+
std::unique_ptr<interfaces::Chain> m_chain;
8293
CChainState* m_chainstate{nullptr};
8394

8495
void BlockConnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex) override;
8596

8697
void ChainStateFlushed(const CBlockLocator& locator) override;
8798

88-
const CBlockIndex* CurrentIndex() { return m_best_block_index.load(); };
89-
9099
/// Initialize internal state from the database and block index.
91-
[[nodiscard]] virtual bool Init();
100+
[[nodiscard]] virtual bool CustomInit(const std::optional<interfaces::BlockKey>& block) { return true; }
92101

93102
/// Write update index entries for a newly connected block.
94-
virtual bool WriteBlock(const CBlock& block, const CBlockIndex* pindex) { return true; }
103+
[[nodiscard]] virtual bool CustomAppend(const interfaces::BlockInfo& block) { return true; }
95104

96105
/// Virtual method called internally by Commit that can be overridden to atomically
97106
/// commit more index state.
98-
virtual bool CommitInternal(CDBBatch& batch);
107+
virtual bool CustomCommit(CDBBatch& batch) { return true; }
99108

100109
/// Rewind index to an earlier chain tip during a chain reorg. The tip must
101110
/// be an ancestor of the current best block.
102-
virtual bool Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip);
111+
[[nodiscard]] virtual bool CustomRewind(const interfaces::BlockKey& current_tip, const interfaces::BlockKey& new_tip) { return true; }
103112

104113
virtual DB& GetDB() const = 0;
105114

@@ -110,6 +119,7 @@ class BaseIndex : public CValidationInterface
110119
void SetBestBlockIndex(const CBlockIndex* block);
111120

112121
public:
122+
BaseIndex(std::unique_ptr<interfaces::Chain> chain);
113123
/// Destructor interrupts sync thread if running and blocks until it exits.
114124
virtual ~BaseIndex();
115125

@@ -124,7 +134,7 @@ class BaseIndex : public CValidationInterface
124134

125135
/// Start initializes the sync state and registers the instance as a
126136
/// ValidationInterface so that it stays in sync with blockchain updates.
127-
[[nodiscard]] bool Start(CChainState& active_chainstate);
137+
[[nodiscard]] bool Start();
128138

129139
/// Stops the instance from staying in sync with blockchain updates.
130140
void Stop();

src/index/blockfilterindex.cpp

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include <index/blockfilterindex.h>
1010
#include <node/blockstorage.h>
1111
#include <util/system.h>
12+
#include <validation.h>
1213

1314
using node::UndoReadFromDisk;
1415

@@ -94,9 +95,9 @@ struct DBHashKey {
9495

9596
static std::map<BlockFilterType, BlockFilterIndex> g_filter_indexes;
9697

97-
BlockFilterIndex::BlockFilterIndex(BlockFilterType filter_type,
98+
BlockFilterIndex::BlockFilterIndex(std::unique_ptr<interfaces::Chain> chain, BlockFilterType filter_type,
9899
size_t n_cache_size, bool f_memory, bool f_wipe)
99-
: m_filter_type(filter_type)
100+
: BaseIndex(std::move(chain)), m_filter_type(filter_type)
100101
{
101102
const std::string& filter_name = BlockFilterTypeName(filter_type);
102103
if (filter_name.empty()) throw std::invalid_argument("unknown filter_type");
@@ -109,7 +110,7 @@ BlockFilterIndex::BlockFilterIndex(BlockFilterType filter_type,
109110
m_filter_fileseq = std::make_unique<FlatFileSeq>(std::move(path), "fltr", FLTR_FILE_CHUNK_SIZE);
110111
}
111112

112-
bool BlockFilterIndex::Init()
113+
bool BlockFilterIndex::CustomInit(const std::optional<interfaces::BlockKey>& block)
113114
{
114115
if (!m_db->Read(DB_FILTER_POS, m_next_filter_pos)) {
115116
// Check that the cause of the read failure is that the key does not exist. Any other errors
@@ -124,10 +125,10 @@ bool BlockFilterIndex::Init()
124125
m_next_filter_pos.nFile = 0;
125126
m_next_filter_pos.nPos = 0;
126127
}
127-
return BaseIndex::Init();
128+
return true;
128129
}
129130

130-
bool BlockFilterIndex::CommitInternal(CDBBatch& batch)
131+
bool BlockFilterIndex::CustomCommit(CDBBatch& batch)
131132
{
132133
const FlatFilePos& pos = m_next_filter_pos;
133134

@@ -141,7 +142,7 @@ bool BlockFilterIndex::CommitInternal(CDBBatch& batch)
141142
}
142143

143144
batch.Write(DB_FILTER_POS, pos);
144-
return BaseIndex::CommitInternal(batch);
145+
return true;
145146
}
146147

147148
bool BlockFilterIndex::ReadFilterFromDisk(const FlatFilePos& pos, const uint256& hash, BlockFilter& filter) const
@@ -214,22 +215,25 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&
214215
return data_size;
215216
}
216217

217-
bool BlockFilterIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
218+
bool BlockFilterIndex::CustomAppend(const interfaces::BlockInfo& block)
218219
{
219220
CBlockUndo block_undo;
220221
uint256 prev_header;
221222

222-
if (pindex->nHeight > 0) {
223+
if (block.height > 0) {
224+
// pindex variable gives indexing code access to node internals. It
225+
// will be removed in upcoming commit
226+
const CBlockIndex* pindex = WITH_LOCK(cs_main, return m_chainstate->m_blockman.LookupBlockIndex(block.hash));
223227
if (!UndoReadFromDisk(block_undo, pindex)) {
224228
return false;
225229
}
226230

227231
std::pair<uint256, DBVal> read_out;
228-
if (!m_db->Read(DBHeightKey(pindex->nHeight - 1), read_out)) {
232+
if (!m_db->Read(DBHeightKey(block.height - 1), read_out)) {
229233
return false;
230234
}
231235

232-
uint256 expected_block_hash = pindex->pprev->GetBlockHash();
236+
uint256 expected_block_hash = *Assert(block.prev_hash);
233237
if (read_out.first != expected_block_hash) {
234238
return error("%s: previous block header belongs to unexpected block %s; expected %s",
235239
__func__, read_out.first.ToString(), expected_block_hash.ToString());
@@ -238,18 +242,18 @@ bool BlockFilterIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex
238242
prev_header = read_out.second.header;
239243
}
240244

241-
BlockFilter filter(m_filter_type, block, block_undo);
245+
BlockFilter filter(m_filter_type, *Assert(block.data), block_undo);
242246

243247
size_t bytes_written = WriteFilterToDisk(m_next_filter_pos, filter);
244248
if (bytes_written == 0) return false;
245249

246250
std::pair<uint256, DBVal> value;
247-
value.first = pindex->GetBlockHash();
251+
value.first = block.hash;
248252
value.second.hash = filter.GetHash();
249253
value.second.header = filter.ComputeHeader(prev_header);
250254
value.second.pos = m_next_filter_pos;
251255

252-
if (!m_db->Write(DBHeightKey(pindex->nHeight), value)) {
256+
if (!m_db->Write(DBHeightKey(block.height), value)) {
253257
return false;
254258
}
255259

@@ -283,17 +287,15 @@ static bool CopyHeightIndexToHashIndex(CDBIterator& db_it, CDBBatch& batch,
283287
return true;
284288
}
285289

286-
bool BlockFilterIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip)
290+
bool BlockFilterIndex::CustomRewind(const interfaces::BlockKey& current_tip, const interfaces::BlockKey& new_tip)
287291
{
288-
assert(current_tip->GetAncestor(new_tip->nHeight) == new_tip);
289-
290292
CDBBatch batch(*m_db);
291293
std::unique_ptr<CDBIterator> db_it(m_db->NewIterator());
292294

293295
// During a reorg, we need to copy all filters for blocks that are getting disconnected from the
294296
// height index to the hash index so we can still find them when the height index entries are
295297
// overwritten.
296-
if (!CopyHeightIndexToHashIndex(*db_it, batch, m_name, new_tip->nHeight, current_tip->nHeight)) {
298+
if (!CopyHeightIndexToHashIndex(*db_it, batch, m_name, new_tip.height, current_tip.height)) {
297299
return false;
298300
}
299301

@@ -303,7 +305,7 @@ bool BlockFilterIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex*
303305
batch.Write(DB_FILTER_POS, m_next_filter_pos);
304306
if (!m_db->WriteBatch(batch)) return false;
305307

306-
return BaseIndex::Rewind(current_tip, new_tip);
308+
return true;
307309
}
308310

309311
static bool LookupOne(const CDBWrapper& db, const CBlockIndex* block_index, DBVal& result)
@@ -467,12 +469,12 @@ void ForEachBlockFilterIndex(std::function<void (BlockFilterIndex&)> fn)
467469
for (auto& entry : g_filter_indexes) fn(entry.second);
468470
}
469471

470-
bool InitBlockFilterIndex(BlockFilterType filter_type,
472+
bool InitBlockFilterIndex(std::function<std::unique_ptr<interfaces::Chain>()> make_chain, BlockFilterType filter_type,
471473
size_t n_cache_size, bool f_memory, bool f_wipe)
472474
{
473475
auto result = g_filter_indexes.emplace(std::piecewise_construct,
474476
std::forward_as_tuple(filter_type),
475-
std::forward_as_tuple(filter_type,
477+
std::forward_as_tuple(make_chain(), filter_type,
476478
n_cache_size, f_memory, f_wipe));
477479
return result.second;
478480
}

0 commit comments

Comments
 (0)