diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -65,6 +65,17 @@ m_mtp_time = &mtp_time; return *this; } + //! Return whether block is in the active (most-work) chain. + FoundBlock &inActiveChain(bool &in_active_chain) { + m_in_active_chain = &in_active_chain; + return *this; + } + //! Return next block in the active chain if current block is in the active + //! chain. + FoundBlock &nextBlock(const FoundBlock &next_block) { + m_next_block = &next_block; + return *this; + } //! Read block data from disk. If the block exists but doesn't have data //! (for example due to pruning), the CBlock variable will be set to null. FoundBlock &data(CBlock &data) { @@ -77,6 +88,8 @@ int64_t *m_time = nullptr; int64_t *m_max_time = nullptr; int64_t *m_mtp_time = nullptr; + bool *m_in_active_chain = nullptr; + const FoundBlock *m_next_block = nullptr; CBlock *m_data = nullptr; }; @@ -102,9 +115,9 @@ //! wallet cache it, fee estimation being driven by node mempool, wallet //! should be the consumer. //! -//! * The `guessVerificationProgress`, `getBlockHeight`, `getBlockHash`, etc -//! methods can go away if rescan logic is moved on the node side, and wallet -//! only register rescan request. +//! * `guessVerificationProgress` and similar methods can go away if rescan +//! logic moves out of the wallet, and the wallet just requests scans from the +//! node (https://github.com/bitcoin/bitcoin/issues/11756) class Chain { public: virtual ~Chain() {} @@ -114,11 +127,6 @@ //! contain any blocks) virtual std::optional getHeight() = 0; - //! Get block height above genesis block. Returns 0 for genesis block, - //! 1 for following block, and so on. Returns std::nullopt for a block not - //! included in the current chain. - virtual std::optional getBlockHeight(const BlockHash &hash) = 0; - //! Get block hash. Height must be valid or this function will abort. virtual BlockHash getBlockHash(int height) = 0; @@ -126,16 +134,6 @@ //! pruned), and contains transactions. virtual bool haveBlockOnDisk(int height) = 0; - //! Return height of the first block in the chain with timestamp equal - //! or greater than the given time and height equal or greater than the - //! given height, or std::nullopt if there is no block with a high enough - //! timestamp and height. Also return the block hash as an optional output - //! parameter (to avoid the cost of a second lookup in case this information - //! is needed.) - virtual std::optional - findFirstBlockWithTimeAndHeight(int64_t time, int height, - BlockHash *hash) = 0; - //! Get locator for the current chain tip. virtual CBlockLocator getTipLocator() = 0; @@ -163,13 +161,6 @@ findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, const FoundBlock &block = {}) = 0; - //! Find next block if block is part of current chain. Also flag if - //! there was a reorg and the specified block hash is no longer in the - //! current chain, and optionally return block information. - virtual bool findNextBlock(const BlockHash &block_hash, int block_height, - const FoundBlock &next = {}, - bool *reorg = nullptr) = 0; - //! Find ancestor of block at specified height and optionally return //! ancestor information. virtual bool findAncestorByHeight(const BlockHash &block_hash, diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -56,6 +56,15 @@ if (block.m_mtp_time) { *block.m_mtp_time = index->GetMedianTimePast(); } + if (block.m_in_active_chain) { + *block.m_in_active_chain = ChainActive()[index->nHeight] == index; + } + if (block.m_next_block) { + FillBlock(ChainActive()[index->nHeight] == index + ? ChainActive()[index->nHeight + 1] + : nullptr, + *block.m_next_block, lock); + } if (block.m_data) { REVERSE_LOCK(lock); if (!ReadBlockFromDisk(*block.m_data, index, @@ -175,14 +184,6 @@ } return std::nullopt; } - std::optional getBlockHeight(const BlockHash &hash) override { - LOCK(::cs_main); - CBlockIndex *block = g_chainman.m_blockman.LookupBlockIndex(hash); - if (block && ::ChainActive().Contains(block)) { - return block->nHeight; - } - return std::nullopt; - } BlockHash getBlockHash(int height) override { LOCK(::cs_main); CBlockIndex *block = ::ChainActive()[height]; @@ -194,20 +195,6 @@ CBlockIndex *block = ::ChainActive()[height]; return block && (block->nStatus.hasData() != 0) && block->nTx > 0; } - std::optional - findFirstBlockWithTimeAndHeight(int64_t time, int height, - BlockHash *hash) override { - LOCK(cs_main); - CBlockIndex *block = - ::ChainActive().FindEarliestAtLeast(time, height); - if (block) { - if (hash) { - *hash = block->GetBlockHash(); - } - return block->nHeight; - } - return std::nullopt; - } CBlockLocator getTipLocator() override { LOCK(cs_main); return ::ChainActive().GetLocator(); @@ -240,19 +227,6 @@ ChainActive().FindEarliestAtLeast(min_time, min_height), block, lock); } - bool findNextBlock(const BlockHash &block_hash, int block_height, - const FoundBlock &next, bool *reorg) override { - WAIT_LOCK(cs_main, lock); - CBlockIndex *block = ChainActive()[block_height]; - if (block && block->GetBlockHash() != block_hash) { - block = nullptr; - } - if (reorg) { - *reorg = !block; - } - return FillBlock(block ? ChainActive()[block_height + 1] : nullptr, - next, lock); - } bool findAncestorByHeight(const BlockHash &block_hash, int ancestor_height, const FoundBlock &ancestor_out) override { diff --git a/src/test/interfaces_tests.cpp b/src/test/interfaces_tests.cpp --- a/src/test/interfaces_tests.cpp +++ b/src/test/interfaces_tests.cpp @@ -50,6 +50,27 @@ FoundBlock().mtpTime(mtp_time))); BOOST_CHECK_EQUAL(mtp_time, active[60]->GetMedianTimePast()); + bool cur_active{false}, next_active{false}; + BlockHash next_hash; + BOOST_CHECK_EQUAL(active.Height(), 100); + BOOST_CHECK(chain->findBlock( + active[99]->GetBlockHash(), + FoundBlock() + .inActiveChain(cur_active) + .nextBlock( + FoundBlock().inActiveChain(next_active).hash(next_hash)))); + BOOST_CHECK(cur_active); + BOOST_CHECK(next_active); + BOOST_CHECK_EQUAL(next_hash, active[100]->GetBlockHash()); + cur_active = next_active = false; + BOOST_CHECK(chain->findBlock( + active[100]->GetBlockHash(), + FoundBlock() + .inActiveChain(cur_active) + .nextBlock(FoundBlock().inActiveChain(next_active)))); + BOOST_CHECK(cur_active); + BOOST_CHECK(!next_active); + BOOST_CHECK(!chain->findBlock(BlockHash(), FoundBlock())); } @@ -68,22 +89,6 @@ /* min_height= */ 0)); } -BOOST_AUTO_TEST_CASE(findNextBlock) { - auto chain = interfaces::MakeChain(m_node, Params()); - auto &active = ChainActive(); - bool reorg; - BlockHash hash; - BOOST_CHECK(chain->findNextBlock(active[20]->GetBlockHash(), 20, - FoundBlock().hash(hash), &reorg)); - BOOST_CHECK_EQUAL(hash, active[21]->GetBlockHash()); - BOOST_CHECK_EQUAL(reorg, false); - BOOST_CHECK(!chain->findNextBlock(BlockHash(), 20, {}, &reorg)); - BOOST_CHECK_EQUAL(reorg, true); - BOOST_CHECK(!chain->findNextBlock(active.Tip()->GetBlockHash(), - active.Height(), {}, &reorg)); - BOOST_CHECK_EQUAL(reorg, false); -} - BOOST_AUTO_TEST_CASE(findAncestorByHeight) { auto chain = interfaces::MakeChain(m_node, Params()); auto &active = ChainActive(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1056,12 +1056,15 @@ // If wallet doesn't have a chain (e.g wallet-tool), don't bother to update // txn. if (HaveChain()) { - std::optional block_height = - chain().getBlockHeight(wtx.m_confirm.hashBlock); - if (block_height) { + bool active; + int height; + if (chain().findBlock( + wtx.m_confirm.hashBlock, + FoundBlock().inActiveChain(active).height(height)) && + active) { // Update cached block height variable since it not stored in the // serialized transaction. - wtx.m_confirm.block_height = *block_height; + wtx.m_confirm.block_height = height; } else if (wtx.isConflicted() || wtx.isConfirmed()) { // If tx block (or conflicting block) was reorged out of chain // while the wallet was shutdown, change tx status to UNCONFIRMED @@ -1968,21 +1971,27 @@ block_height, progress_current); } + // Read block data CBlock block; - bool next_block; + chain().findBlock(block_hash, FoundBlock().data(block)); + + // Find next block separately from reading data above, because reading + // is slow and there might be a reorg while it is read. + bool block_still_active = false; + bool next_block = false; BlockHash next_block_hash; - bool reorg = false; - if (chain().findBlock(block_hash, FoundBlock().data(block)) && - !block.IsNull()) { + chain().findBlock(block_hash, + FoundBlock() + .inActiveChain(block_still_active) + .nextBlock(FoundBlock() + .inActiveChain(next_block) + .hash(next_block_hash))); + + if (!block.IsNull()) { LOCK(cs_wallet); - next_block = chain().findNextBlock( - block_hash, block_height, FoundBlock().hash(next_block_hash), - &reorg); - if (reorg) { + if (!block_still_active) { // Abort scan if current block is no longer active, to prevent // marking transactions as coming from the wrong block. - // TODO: This should return success instead of failure, see - // https://github.com/bitcoin/bitcoin/pull/14711#issuecomment-458342518 result.last_failed_block = block_hash; result.status = ScanResult::FAILURE; break; @@ -2006,15 +2015,12 @@ // the most recent failure result.last_failed_block = block_hash; result.status = ScanResult::FAILURE; - next_block = chain().findNextBlock( - block_hash, block_height, FoundBlock().hash(next_block_hash), - &reorg); } if (max_height && block_height >= *max_height) { break; } { - if (!next_block || reorg) { + if (!next_block) { // break successfully when rescan has reached the tip, or // previous block is no longer on the chain due to a reorg break; @@ -4530,12 +4536,9 @@ } } if (time_first_key) { - if (std::optional first_block = - chain.findFirstBlockWithTimeAndHeight( - *time_first_key - TIMESTAMP_WINDOW, rescan_height, - nullptr)) { - rescan_height = *first_block; - } + chain.findFirstBlockWithTimeAndHeight( + *time_first_key - TIMESTAMP_WINDOW, rescan_height, + FoundBlock().height(rescan_height)); } {