Skip to content

Commit bf30cd4

Browse files
ryanofskyryanofsky
committed
refactor: Add interfaces::FoundBlock class to selectively return block data
FoundBlock class allows interfaces::Chain::findBlock to return more block information without having lots of optional output parameters. FoundBlock class is also used by other chain methods in upcoming commits. There is mostly no change in behavior. Only exception is CWallet::RescanFromTime now throwing NonFatalCheckError instead of std::logic_error.
1 parent d52ba21 commit bf30cd4

File tree

7 files changed

+99
-36
lines changed

7 files changed

+99
-36
lines changed

‎src/Makefile.test.include‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,7 @@ BITCOIN_TESTS =\
198198
test/fs_tests.cpp \
199199
test/getarg_tests.cpp \
200200
test/hash_tests.cpp \
201+
test/interfaces_tests.cpp \
201202
test/key_io_tests.cpp \
202203
test/key_tests.cpp \
203204
test/limitedmap_tests.cpp \

‎src/interfaces/chain.cpp‎

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,21 @@
3838
namespace interfaces {
3939
namespace {
4040

41+
bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<RecursiveMutex>& lock)
42+
{
43+
if (!index) return false;
44+
if (block.m_hash) *block.m_hash = index->GetBlockHash();
45+
if (block.m_height) *block.m_height = index->nHeight;
46+
if (block.m_time) *block.m_time = index->GetBlockTime();
47+
if (block.m_max_time) *block.m_max_time = index->GetBlockTimeMax();
48+
if (block.m_mtp_time) *block.m_mtp_time = index->GetMedianTimePast();
49+
if (block.m_data) {
50+
REVERSE_LOCK(lock);
51+
if (!ReadBlockFromDisk(*block.m_data, index, Params().GetConsensus())) block.m_data->SetNull();
52+
}
53+
return true;
54+
}
55+
4156
class LockImpl : public Chain::Lock, public UniqueLock<RecursiveMutex>
4257
{
4358
Optional<int> getHeight() override
@@ -247,26 +262,10 @@ class ChainImpl : public Chain
247262
std::unique_ptr<Chain::Lock> result = std::move(lock); // Temporary to avoid CWG 1579
248263
return result;
249264
}
250-
bool findBlock(const uint256& hash, CBlock* block, int64_t* time, int64_t* time_max) override
265+
bool findBlock(const uint256& hash, const FoundBlock& block) override
251266
{
252-
CBlockIndex* index;
253-
{
254-
LOCK(cs_main);
255-
index = LookupBlockIndex(hash);
256-
if (!index) {
257-
return false;
258-
}
259-
if (time) {
260-
*time = index->GetBlockTime();
261-
}
262-
if (time_max) {
263-
*time_max = index->GetBlockTimeMax();
264-
}
265-
}
266-
if (block && !ReadBlockFromDisk(*block, index, Params().GetConsensus())) {
267-
block->SetNull();
268-
}
269-
return true;
267+
WAIT_LOCK(cs_main, lock);
268+
return FillBlock(LookupBlockIndex(hash), block, lock);
270269
}
271270
void findCoins(std::map<COutPoint, Coin>& coins) override { return FindCoins(m_node, coins); }
272271
double guessVerificationProgress(const uint256& block_hash) override

‎src/interfaces/chain.h‎

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,27 @@ namespace interfaces {
3030
class Handler;
3131
class Wallet;
3232

33+
//! Helper for findBlock to selectively return pieces of block data.
34+
class FoundBlock
35+
{
36+
public:
37+
FoundBlock& hash(uint256& hash) { m_hash = &hash; return *this; }
38+
FoundBlock& height(int& height) { m_height = &height; return *this; }
39+
FoundBlock& time(int64_t& time) { m_time = &time; return *this; }
40+
FoundBlock& maxTime(int64_t& max_time) { m_max_time = &max_time; return *this; }
41+
FoundBlock& mtpTime(int64_t& mtp_time) { m_mtp_time = &mtp_time; return *this; }
42+
//! Read block data from disk. If the block exists but doesn't have data
43+
//! (for example due to pruning), the CBlock variable will be set to null.
44+
FoundBlock& data(CBlock& data) { m_data = &data; return *this; }
45+
46+
uint256* m_hash = nullptr;
47+
int* m_height = nullptr;
48+
int64_t* m_time = nullptr;
49+
int64_t* m_max_time = nullptr;
50+
int64_t* m_mtp_time = nullptr;
51+
CBlock* m_data = nullptr;
52+
};
53+
3354
//! Interface giving clients (wallet processes, maybe other analysis tools in
3455
//! the future) ability to access to the chain state, receive notifications,
3556
//! estimate fees, and submit transactions.
@@ -127,14 +148,7 @@ class Chain
127148

128149
//! Return whether node has the block and optionally return block metadata
129150
//! or contents.
130-
//!
131-
//! If a block pointer is provided to retrieve the block contents, and the
132-
//! block exists but doesn't have data (for example due to pruning), the
133-
//! block will be empty and all fields set to null.
134-
virtual bool findBlock(const uint256& hash,
135-
CBlock* block = nullptr,
136-
int64_t* time = nullptr,
137-
int64_t* max_time = nullptr) = 0;
151+
virtual bool findBlock(const uint256& hash, const FoundBlock& block={}) = 0;
138152

139153
//! Look up unspent output information. Returns coins in the mempool and in
140154
//! the current chain UTXO set. Iterates through all the keys in the map and

‎src/sync.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ class SCOPED_LOCKABLE UniqueLock : public Base
210210
friend class reverse_lock;
211211
};
212212

213-
#define REVERSE_LOCK(g) decltype(g)::reverse_lock PASTE2(revlock, __COUNTER__)(g, #g, __FILE__, __LINE__)
213+
#define REVERSE_LOCK(g) typename std::decay<decltype(g)>::type::reverse_lock PASTE2(revlock, __COUNTER__)(g, #g, __FILE__, __LINE__)
214214

215215
template<typename MutexArg>
216216
using DebugLock = UniqueLock<typename std::remove_reference<typename std::remove_pointer<MutexArg>::type>::type>;

‎src/test/interfaces_tests.cpp‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
// Copyright (c) 2020 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <interfaces/chain.h>
6+
#include <test/util/setup_common.h>
7+
#include <validation.h>
8+
9+
#include <boost/test/unit_test.hpp>
10+
11+
using interfaces::FoundBlock;
12+
13+
BOOST_FIXTURE_TEST_SUITE(interfaces_tests, TestChain100Setup)
14+
15+
BOOST_AUTO_TEST_CASE(findBlock)
16+
{
17+
auto chain = interfaces::MakeChain(m_node);
18+
auto& active = ChainActive();
19+
20+
uint256 hash;
21+
BOOST_CHECK(chain->findBlock(active[10]->GetBlockHash(), FoundBlock().hash(hash)));
22+
BOOST_CHECK_EQUAL(hash, active[10]->GetBlockHash());
23+
24+
int height = -1;
25+
BOOST_CHECK(chain->findBlock(active[20]->GetBlockHash(), FoundBlock().height(height)));
26+
BOOST_CHECK_EQUAL(height, active[20]->nHeight);
27+
28+
CBlock data;
29+
BOOST_CHECK(chain->findBlock(active[30]->GetBlockHash(), FoundBlock().data(data)));
30+
BOOST_CHECK_EQUAL(data.GetHash(), active[30]->GetBlockHash());
31+
32+
int64_t time = -1;
33+
BOOST_CHECK(chain->findBlock(active[40]->GetBlockHash(), FoundBlock().time(time)));
34+
BOOST_CHECK_EQUAL(time, active[40]->GetBlockTime());
35+
36+
int64_t max_time = -1;
37+
BOOST_CHECK(chain->findBlock(active[50]->GetBlockHash(), FoundBlock().maxTime(max_time)));
38+
BOOST_CHECK_EQUAL(max_time, active[50]->GetBlockTimeMax());
39+
40+
int64_t mtp_time = -1;
41+
BOOST_CHECK(chain->findBlock(active[60]->GetBlockHash(), FoundBlock().mtpTime(mtp_time)));
42+
BOOST_CHECK_EQUAL(mtp_time, active[60]->GetMedianTimePast());
43+
44+
BOOST_CHECK(!chain->findBlock({}, FoundBlock()));
45+
}
46+
47+
BOOST_AUTO_TEST_SUITE_END()

‎src/wallet/rpcwallet.cpp‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@
3737
#include <univalue.h>
3838

3939

40+
using interfaces::FoundBlock;
41+
4042
static const std::string WALLET_ENDPOINT_BASE = "/wallet/";
4143

4244
static inline bool GetAvoidReuseFlag(const CWallet* const pwallet, const UniValue& param) {
@@ -149,8 +151,7 @@ static void WalletTxToJSON(interfaces::Chain& chain, interfaces::Chain::Lock& lo
149151
entry.pushKV("blockheight", wtx.m_confirm.block_height);
150152
entry.pushKV("blockindex", wtx.m_confirm.nIndex);
151153
int64_t block_time;
152-
bool found_block = chain.findBlock(wtx.m_confirm.hashBlock, nullptr /* block */, &block_time);
153-
CHECK_NONFATAL(found_block);
154+
CHECK_NONFATAL(chain.findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(block_time)));
154155
entry.pushKV("blocktime", block_time);
155156
} else {
156157
entry.pushKV("trusted", wtx.IsTrusted(locked_chain));
@@ -1618,7 +1619,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
16181619
UniValue removed(UniValue::VARR);
16191620
while (include_removed && altheight && *altheight > *height) {
16201621
CBlock block;
1621-
if (!pwallet->chain().findBlock(blockId, &block) || block.IsNull()) {
1622+
if (!pwallet->chain().findBlock(blockId, FoundBlock().data(block)) || block.IsNull()) {
16221623
throw JSONRPCError(RPC_INTERNAL_ERROR, "Can't read block from disk");
16231624
}
16241625
for (const CTransactionRef& tx : block.vtx) {

‎src/wallet/wallet.cpp‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include <script/script.h>
2323
#include <script/signingprovider.h>
2424
#include <util/bip32.h>
25+
#include <util/check.h>
2526
#include <util/error.h>
2627
#include <util/fees.h>
2728
#include <util/moneystr.h>
@@ -35,6 +36,8 @@
3536

3637
#include <boost/algorithm/string/replace.hpp>
3738

39+
using interfaces::FoundBlock;
40+
3841
const std::map<uint64_t,std::string> WALLET_FLAG_CAVEATS{
3942
{WALLET_FLAG_AVOID_REUSE,
4043
"You need to rescan the blockchain in order to correctly mark used "
@@ -1601,9 +1604,7 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
16011604
ScanResult result = ScanForWalletTransactions(start_block, {} /* stop_block */, reserver, update);
16021605
if (result.status == ScanResult::FAILURE) {
16031606
int64_t time_max;
1604-
if (!chain().findBlock(result.last_failed_block, nullptr /* block */, nullptr /* time */, &time_max)) {
1605-
throw std::logic_error("ScanForWalletTransactions returned invalid block hash");
1606-
}
1607+
CHECK_NONFATAL(chain().findBlock(result.last_failed_block, FoundBlock().maxTime(time_max)));
16071608
return time_max + TIMESTAMP_WINDOW + 1;
16081609
}
16091610
}
@@ -1671,7 +1672,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
16711672
}
16721673

16731674
CBlock block;
1674-
if (chain().findBlock(block_hash, &block) && !block.IsNull()) {
1675+
if (chain().findBlock(block_hash, FoundBlock().data(block)) && !block.IsNull()) {
16751676
auto locked_chain = chain().lock();
16761677
LOCK(cs_wallet);
16771678
if (!locked_chain->getBlockHeight(block_hash)) {
@@ -3622,7 +3623,7 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
36223623
unsigned int nTimeSmart = wtx.nTimeReceived;
36233624
if (!wtx.isUnconfirmed() && !wtx.isAbandoned()) {
36243625
int64_t blocktime;
3625-
if (chain().findBlock(wtx.m_confirm.hashBlock, nullptr /* block */, &blocktime)) {
3626+
if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(blocktime))) {
36263627
int64_t latestNow = wtx.nTimeReceived;
36273628
int64_t latestEntry = 0;
36283629

0 commit comments

Comments
 (0)