Skip to content

Commit 0c9dea5

Browse files
ryanofskyryanofsky
committed
refactor: Get rid of more redundant chain methods
This just drops two interfaces::Chain methods replacing them with other calls, not changing behavior in any way. Motivation was - Need to get rid of findFirstBlockWithTimeAndHeight for #10102 - Followup from #16426 (comment) - phantomcircuit comments about findNextBlock test http://www.erisian.com.au/bitcoin-core-dev/log-2020-06-06.html#l-214
1 parent 85a6895 commit 0c9dea5

File tree

4 files changed

+33
-58
lines changed

4 files changed

+33
-58
lines changed

‎src/interfaces/chain.cpp‎

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<Rec
4646
if (block.m_time) *block.m_time = index->GetBlockTime();
4747
if (block.m_max_time) *block.m_max_time = index->GetBlockTimeMax();
4848
if (block.m_mtp_time) *block.m_mtp_time = index->GetMedianTimePast();
49+
if (block.m_in_active_chain) *block.m_in_active_chain = ::ChainActive()[index->nHeight] == index;
50+
if (block.m_next_block) FillBlock(ChainActive()[index->nHeight] == index ? ChainActive()[index->nHeight + 1] : nullptr, *block.m_next_block, lock);
4951
if (block.m_data) {
5052
REVERSE_LOCK(lock);
5153
if (!ReadBlockFromDisk(*block.m_data, index, Params().GetConsensus())) block.m_data->SetNull();
@@ -176,16 +178,6 @@ class ChainImpl : public Chain
176178
CBlockIndex* block = ::ChainActive()[height];
177179
return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0;
178180
}
179-
Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) override
180-
{
181-
LOCK(cs_main);
182-
CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time, height);
183-
if (block) {
184-
if (hash) *hash = block->GetBlockHash();
185-
return block->nHeight;
186-
}
187-
return nullopt;
188-
}
189181
CBlockLocator getTipLocator() override
190182
{
191183
LOCK(cs_main);
@@ -214,13 +206,6 @@ class ChainImpl : public Chain
214206
WAIT_LOCK(cs_main, lock);
215207
return FillBlock(ChainActive().FindEarliestAtLeast(min_time, min_height), block, lock);
216208
}
217-
bool findNextBlock(const uint256& block_hash, int block_height, const FoundBlock& next, bool* reorg) override {
218-
WAIT_LOCK(cs_main, lock);
219-
CBlockIndex* block = ChainActive()[block_height];
220-
if (block && block->GetBlockHash() != block_hash) block = nullptr;
221-
if (reorg) *reorg = !block;
222-
return FillBlock(block ? ChainActive()[block_height + 1] : nullptr, next, lock);
223-
}
224209
bool findAncestorByHeight(const uint256& block_hash, int ancestor_height, const FoundBlock& ancestor_out) override
225210
{
226211
WAIT_LOCK(cs_main, lock);

‎src/interfaces/chain.h‎

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,10 @@ class FoundBlock
4242
FoundBlock& time(int64_t& time) { m_time = &time; return *this; }
4343
FoundBlock& maxTime(int64_t& max_time) { m_max_time = &max_time; return *this; }
4444
FoundBlock& mtpTime(int64_t& mtp_time) { m_mtp_time = &mtp_time; return *this; }
45+
//! Return whether block is part of the current active (most-work) chain.
46+
FoundBlock& inActiveChain(bool& in_active_chain) { m_in_active_chain = &in_active_chain; return *this; }
47+
//! Return next block after this in the current active chain.
48+
FoundBlock& nextBlock(const FoundBlock& next_block) { m_next_block = &next_block; return *this; }
4549
//! Read block data from disk. If the block exists but doesn't have data
4650
//! (for example due to pruning), the CBlock variable will be set to null.
4751
FoundBlock& data(CBlock& data) { m_data = &data; return *this; }
@@ -52,6 +56,8 @@ class FoundBlock
5256
int64_t* m_max_time = nullptr;
5357
int64_t* m_mtp_time = nullptr;
5458
CBlock* m_data = nullptr;
59+
bool* m_in_active_chain = nullptr;
60+
const FoundBlock* m_next_block = nullptr;
5561
};
5662

5763
//! Interface giving clients (wallet processes, maybe other analysis tools in
@@ -77,7 +83,8 @@ class FoundBlock
7783
//!
7884
//! * The `guessVerificationProgress`, `getBlockHeight`, `getBlockHash`, etc
7985
//! methods can go away if rescan logic is moved on the node side, and wallet
80-
//! only register rescan request.
86+
//! just requests scans from the node
87+
//! (https://github.com/bitcoin/bitcoin/issues/11756)
8188
class Chain
8289
{
8390
public:
@@ -100,13 +107,6 @@ class Chain
100107
//! pruned), and contains transactions.
101108
virtual bool haveBlockOnDisk(int height) = 0;
102109

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

@@ -128,11 +128,6 @@ class Chain
128128
//! information.
129129
virtual bool findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, const FoundBlock& block={}) = 0;
130130

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

‎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: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -898,11 +898,12 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx
898898
}
899899
// If wallet doesn't have a chain (e.g wallet-tool), don't bother to update txn.
900900
if (HaveChain()) {
901-
Optional<int> block_height = chain().getBlockHeight(wtx.m_confirm.hashBlock);
902-
if (block_height) {
901+
bool active;
902+
int height;
903+
if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().inActiveChain(active).height(height)) && active) {
903904
// Update cached block height variable since it not stored in the
904905
// serialized transaction.
905-
wtx.m_confirm.block_height = *block_height;
906+
wtx.m_confirm.block_height = height;
906907
} else if (wtx.isConflicted() || wtx.isConfirmed()) {
907908
// If tx block (or conflicting block) was reorged out of chain
908909
// while the wallet was shutdown, change tx status to UNCONFIRMED
@@ -1706,13 +1707,12 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
17061707
}
17071708

17081709
CBlock block;
1709-
bool next_block;
1710+
bool next_block = false;
17101711
uint256 next_block_hash;
1711-
bool reorg = false;
1712-
if (chain().findBlock(block_hash, FoundBlock().data(block)) && !block.IsNull()) {
1712+
if (chain().findBlock(block_hash, FoundBlock().data(block).nextBlock(FoundBlock().inActiveChain(next_block))) && !block.IsNull()) {
17131713
LOCK(cs_wallet);
1714-
next_block = chain().findNextBlock(block_hash, block_height, FoundBlock().hash(next_block_hash), &reorg);
1715-
if (reorg) {
1714+
bool active;
1715+
if (!chain().findBlock(block_hash, FoundBlock().inActiveChain(active).nextBlock(FoundBlock().inActiveChain(next_block).hash(next_block_hash))) || !active) {
17161716
// Abort scan if current block is no longer active, to prevent
17171717
// marking transactions as coming from the wrong block.
17181718
// TODO: This should return success instead of failure, see
@@ -1731,13 +1731,13 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
17311731
// could not scan block, keep scanning but record this block as the most recent failure
17321732
result.last_failed_block = block_hash;
17331733
result.status = ScanResult::FAILURE;
1734-
next_block = chain().findNextBlock(block_hash, block_height, FoundBlock().hash(next_block_hash), &reorg);
1734+
chain().findBlock(block_hash, FoundBlock().nextBlock(FoundBlock().inActiveChain(next_block).hash(next_block_hash)));
17351735
}
17361736
if (max_height && block_height >= *max_height) {
17371737
break;
17381738
}
17391739
{
1740-
if (!next_block || reorg) {
1740+
if (!next_block) {
17411741
// break successfully when rescan has reached the tip, or
17421742
// previous block is no longer on the chain due to a reorg
17431743
break;
@@ -4006,9 +4006,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
40064006
if (!time_first_key || time < *time_first_key) time_first_key = time;
40074007
}
40084008
if (time_first_key) {
4009-
if (Optional<int> first_block = chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, nullptr)) {
4010-
rescan_height = *first_block;
4011-
}
4009+
chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, FoundBlock().height(rescan_height));
40124010
}
40134011

40144012
{

0 commit comments

Comments
 (0)