Skip to content

Commit 3fbbb9a

Browse files
ryanofskyryanofsky
committed
refactor: Get rid of more redundant chain methods
This just drops three interfaces::Chain methods replacing them with other calls. Motivation for removing these chain methods: - Need to get rid of findFirstBlockWithTimeAndHeight for #10102, which doesn't support overloaded methods - Followup from bitcoin/bitcoin#16426 (comment) - phantomcircuit comments about findNextBlock test http://www.erisian.com.au/bitcoin-core-dev/log-2020-06-06.html#l-214 Behavior is not changing in any way here. A TODO comment in ScanForWalletTransactions was removed, but just because it was invalid (see bitcoin/bitcoin#19195 (comment)), not because it was implemented.
1 parent 5c4911e commit 3fbbb9a

File tree

4 files changed

+40
-76
lines changed

4 files changed

+40
-76
lines changed

‎src/interfaces/chain.h‎

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,10 @@ class FoundBlock
4444
FoundBlock& time(int64_t& time) { m_time = &time; return *this; }
4545
FoundBlock& maxTime(int64_t& max_time) { m_max_time = &max_time; return *this; }
4646
FoundBlock& mtpTime(int64_t& mtp_time) { m_mtp_time = &mtp_time; return *this; }
47+
//! Return whether block is in the active (most-work) chain.
48+
FoundBlock& inActiveChain(bool& in_active_chain) { m_in_active_chain = &in_active_chain; return *this; }
49+
//! Return next block in the active chain if current block is in the active chain.
50+
FoundBlock& nextBlock(const FoundBlock& next_block) { m_next_block = &next_block; return *this; }
4751
//! Read block data from disk. If the block exists but doesn't have data
4852
//! (for example due to pruning), the CBlock variable will be set to null.
4953
FoundBlock& data(CBlock& data) { m_data = &data; return *this; }
@@ -53,6 +57,8 @@ class FoundBlock
5357
int64_t* m_time = nullptr;
5458
int64_t* m_max_time = nullptr;
5559
int64_t* m_mtp_time = nullptr;
60+
bool* m_in_active_chain = nullptr;
61+
const FoundBlock* m_next_block = nullptr;
5662
CBlock* m_data = nullptr;
5763
};
5864

@@ -77,9 +83,9 @@ class FoundBlock
7783
//! wallet cache it, fee estimation being driven by node mempool, wallet
7884
//! should be the consumer.
7985
//!
80-
//! * The `guessVerificationProgress`, `getBlockHeight`, `getBlockHash`, etc
81-
//! methods can go away if rescan logic is moved on the node side, and wallet
82-
//! only register rescan request.
86+
//! * `guessVerificationProgress` and similar methods can go away if rescan
87+
//! logic moves out of the wallet, and the wallet just requests scans from the
88+
//! node (https://github.com/bitcoin/bitcoin/issues/11756)
8389
class Chain
8490
{
8591
public:
@@ -90,25 +96,13 @@ class Chain
9096
//! any blocks)
9197
virtual Optional<int> getHeight() = 0;
9298

93-
//! Get block height above genesis block. Returns 0 for genesis block,
94-
//! 1 for following block, and so on. Returns nullopt for a block not
95-
//! included in the current chain.
96-
virtual Optional<int> getBlockHeight(const uint256& hash) = 0;
97-
9899
//! Get block hash. Height must be valid or this function will abort.
99100
virtual uint256 getBlockHash(int height) = 0;
100101

101102
//! Check that the block is available on disk (i.e. has not been
102103
//! pruned), and contains transactions.
103104
virtual bool haveBlockOnDisk(int height) = 0;
104105

105-
//! Return height of the first block in the chain with timestamp equal
106-
//! or greater than the given time and height equal or greater than the
107-
//! given height, or nullopt if there is no block with a high enough
108-
//! timestamp and height. Also return the block hash as an optional output parameter
109-
//! (to avoid the cost of a second lookup in case this information is needed.)
110-
virtual Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) = 0;
111-
112106
//! Get locator for the current chain tip.
113107
virtual CBlockLocator getTipLocator() = 0;
114108

@@ -130,11 +124,6 @@ class Chain
130124
//! information.
131125
virtual bool findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, const FoundBlock& block={}) = 0;
132126

133-
//! Find next block if block is part of current chain. Also flag if
134-
//! there was a reorg and the specified block hash is no longer in the
135-
//! current chain, and optionally return block information.
136-
virtual bool findNextBlock(const uint256& block_hash, int block_height, const FoundBlock& next={}, bool* reorg=nullptr) = 0;
137-
138127
//! Find ancestor of block at specified height and optionally return
139128
//! ancestor information.
140129
virtual bool findAncestorByHeight(const uint256& block_hash, int ancestor_height, const FoundBlock& ancestor_out={}) = 0;

‎src/node/interfaces.cpp‎

Lines changed: 2 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,8 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<Rec
311311
if (block.m_time) *block.m_time = index->GetBlockTime();
312312
if (block.m_max_time) *block.m_max_time = index->GetBlockTimeMax();
313313
if (block.m_mtp_time) *block.m_mtp_time = index->GetMedianTimePast();
314+
if (block.m_in_active_chain) *block.m_in_active_chain = ChainActive()[index->nHeight] == index;
315+
if (block.m_next_block) FillBlock(ChainActive()[index->nHeight] == index ? ChainActive()[index->nHeight + 1] : nullptr, *block.m_next_block, lock);
314316
if (block.m_data) {
315317
REVERSE_LOCK(lock);
316318
if (!ReadBlockFromDisk(*block.m_data, index, Params().GetConsensus())) block.m_data->SetNull();
@@ -419,15 +421,6 @@ class ChainImpl : public Chain
419421
}
420422
return nullopt;
421423
}
422-
Optional<int> getBlockHeight(const uint256& hash) override
423-
{
424-
LOCK(::cs_main);
425-
CBlockIndex* block = LookupBlockIndex(hash);
426-
if (block && ::ChainActive().Contains(block)) {
427-
return block->nHeight;
428-
}
429-
return nullopt;
430-
}
431424
uint256 getBlockHash(int height) override
432425
{
433426
LOCK(::cs_main);
@@ -441,16 +434,6 @@ class ChainImpl : public Chain
441434
CBlockIndex* block = ::ChainActive()[height];
442435
return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0;
443436
}
444-
Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) override
445-
{
446-
LOCK(cs_main);
447-
CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time, height);
448-
if (block) {
449-
if (hash) *hash = block->GetBlockHash();
450-
return block->nHeight;
451-
}
452-
return nullopt;
453-
}
454437
CBlockLocator getTipLocator() override
455438
{
456439
LOCK(cs_main);
@@ -479,13 +462,6 @@ class ChainImpl : public Chain
479462
WAIT_LOCK(cs_main, lock);
480463
return FillBlock(ChainActive().FindEarliestAtLeast(min_time, min_height), block, lock);
481464
}
482-
bool findNextBlock(const uint256& block_hash, int block_height, const FoundBlock& next, bool* reorg) override {
483-
WAIT_LOCK(cs_main, lock);
484-
CBlockIndex* block = ChainActive()[block_height];
485-
if (block && block->GetBlockHash() != block_hash) block = nullptr;
486-
if (reorg) *reorg = !block;
487-
return FillBlock(block ? ChainActive()[block_height + 1] : nullptr, next, lock);
488-
}
489465
bool findAncestorByHeight(const uint256& block_hash, int ancestor_height, const FoundBlock& ancestor_out) override
490466
{
491467
WAIT_LOCK(cs_main, lock);

‎src/test/interfaces_tests.cpp‎

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,18 @@ BOOST_AUTO_TEST_CASE(findBlock)
4444
BOOST_CHECK(chain->findBlock(active[60]->GetBlockHash(), FoundBlock().mtpTime(mtp_time)));
4545
BOOST_CHECK_EQUAL(mtp_time, active[60]->GetMedianTimePast());
4646

47+
bool cur_active{false}, next_active{false};
48+
uint256 next_hash;
49+
BOOST_CHECK_EQUAL(active.Height(), 100);
50+
BOOST_CHECK(chain->findBlock(active[99]->GetBlockHash(), FoundBlock().inActiveChain(cur_active).nextBlock(FoundBlock().inActiveChain(next_active).hash(next_hash))));
51+
BOOST_CHECK(cur_active);
52+
BOOST_CHECK(next_active);
53+
BOOST_CHECK_EQUAL(next_hash, active[100]->GetBlockHash());
54+
cur_active = next_active = false;
55+
BOOST_CHECK(chain->findBlock(active[100]->GetBlockHash(), FoundBlock().inActiveChain(cur_active).nextBlock(FoundBlock().inActiveChain(next_active))));
56+
BOOST_CHECK(cur_active);
57+
BOOST_CHECK(!next_active);
58+
4759
BOOST_CHECK(!chain->findBlock({}, FoundBlock()));
4860
}
4961

@@ -59,21 +71,6 @@ BOOST_AUTO_TEST_CASE(findFirstBlockWithTimeAndHeight)
5971
BOOST_CHECK(!chain->findFirstBlockWithTimeAndHeight(/* min_time= */ active.Tip()->GetBlockTimeMax() + 1, /* min_height= */ 0));
6072
}
6173

62-
BOOST_AUTO_TEST_CASE(findNextBlock)
63-
{
64-
auto chain = interfaces::MakeChain(m_node);
65-
auto& active = ChainActive();
66-
bool reorg;
67-
uint256 hash;
68-
BOOST_CHECK(chain->findNextBlock(active[20]->GetBlockHash(), 20, FoundBlock().hash(hash), &reorg));
69-
BOOST_CHECK_EQUAL(hash, active[21]->GetBlockHash());
70-
BOOST_CHECK_EQUAL(reorg, false);
71-
BOOST_CHECK(!chain->findNextBlock(uint256(), 20, {}, &reorg));
72-
BOOST_CHECK_EQUAL(reorg, true);
73-
BOOST_CHECK(!chain->findNextBlock(active.Tip()->GetBlockHash(), active.Height(), {}, &reorg));
74-
BOOST_CHECK_EQUAL(reorg, false);
75-
}
76-
7774
BOOST_AUTO_TEST_CASE(findAncestorByHeight)
7875
{
7976
auto chain = interfaces::MakeChain(m_node);

‎src/wallet/wallet.cpp‎

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -946,11 +946,12 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx
946946
}
947947
// If wallet doesn't have a chain (e.g wallet-tool), don't bother to update txn.
948948
if (HaveChain()) {
949-
Optional<int> block_height = chain().getBlockHeight(wtx.m_confirm.hashBlock);
950-
if (block_height) {
949+
bool active;
950+
int height;
951+
if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().inActiveChain(active).height(height)) && active) {
951952
// Update cached block height variable since it not stored in the
952953
// serialized transaction.
953-
wtx.m_confirm.block_height = *block_height;
954+
wtx.m_confirm.block_height = height;
954955
} else if (wtx.isConflicted() || wtx.isConfirmed()) {
955956
// If tx block (or conflicting block) was reorged out of chain
956957
// while the wallet was shutdown, change tx status to UNCONFIRMED
@@ -1771,18 +1772,22 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
17711772
WalletLogPrintf("Still rescanning. At block %d. Progress=%f\n", block_height, progress_current);
17721773
}
17731774

1775+
// Read block data
17741776
CBlock block;
1775-
bool next_block;
1777+
chain().findBlock(block_hash, FoundBlock().data(block));
1778+
1779+
// Find next block separately from reading data above, because reading
1780+
// is slow and there might be a reorg while it is read.
1781+
bool block_still_active = false;
1782+
bool next_block = false;
17761783
uint256 next_block_hash;
1777-
bool reorg = false;
1778-
if (chain().findBlock(block_hash, FoundBlock().data(block)) && !block.IsNull()) {
1784+
chain().findBlock(block_hash, FoundBlock().inActiveChain(block_still_active).nextBlock(FoundBlock().inActiveChain(next_block).hash(next_block_hash)));
1785+
1786+
if (!block.IsNull()) {
17791787
LOCK(cs_wallet);
1780-
next_block = chain().findNextBlock(block_hash, block_height, FoundBlock().hash(next_block_hash), &reorg);
1781-
if (reorg) {
1788+
if (!block_still_active) {
17821789
// Abort scan if current block is no longer active, to prevent
17831790
// marking transactions as coming from the wrong block.
1784-
// TODO: This should return success instead of failure, see
1785-
// https://github.com/bitcoin/bitcoin/pull/14711#issuecomment-458342518
17861791
result.last_failed_block = block_hash;
17871792
result.status = ScanResult::FAILURE;
17881793
break;
@@ -1797,13 +1802,12 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
17971802
// could not scan block, keep scanning but record this block as the most recent failure
17981803
result.last_failed_block = block_hash;
17991804
result.status = ScanResult::FAILURE;
1800-
next_block = chain().findNextBlock(block_hash, block_height, FoundBlock().hash(next_block_hash), &reorg);
18011805
}
18021806
if (max_height && block_height >= *max_height) {
18031807
break;
18041808
}
18051809
{
1806-
if (!next_block || reorg) {
1810+
if (!next_block) {
18071811
// break successfully when rescan has reached the tip, or
18081812
// previous block is no longer on the chain due to a reorg
18091813
break;
@@ -4058,9 +4062,7 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::st
40584062
if (!time_first_key || time < *time_first_key) time_first_key = time;
40594063
}
40604064
if (time_first_key) {
4061-
if (Optional<int> first_block = chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, nullptr)) {
4062-
rescan_height = *first_block;
4063-
}
4065+
chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, FoundBlock().height(rescan_height));
40644066
}
40654067

40664068
{

0 commit comments

Comments
 (0)