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.

7 files changed, 99 insertions, 36 deletions

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 059876bec8..a4d8946807 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -198,6 +198,7 @@ BITCOIN_TESTS =\
   test/fs_tests.cpp \
   test/getarg_tests.cpp \
   test/hash_tests.cpp \
+  test/interfaces_tests.cpp \
   test/key_io_tests.cpp \
   test/key_tests.cpp \
   test/limitedmap_tests.cpp \
diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
index 0b3cd08e22..cfaf79f709 100644
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -38,6 +38,21 @@
 namespace interfaces {
 namespace {
 
+bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<RecursiveMutex>& lock)
+{
+    if (!index) return false;
+    if (block.m_hash) *block.m_hash = index->GetBlockHash();
+    if (block.m_height) *block.m_height = index->nHeight;
+    if (block.m_time) *block.m_time = index->GetBlockTime();
+    if (block.m_max_time) *block.m_max_time = index->GetBlockTimeMax();
+    if (block.m_mtp_time) *block.m_mtp_time = index->GetMedianTimePast();
+    if (block.m_data) {
+        REVERSE_LOCK(lock);
+        if (!ReadBlockFromDisk(*block.m_data, index, Params().GetConsensus())) block.m_data->SetNull();
+    }
+    return true;
+}
+
 class LockImpl : public Chain::Lock, public UniqueLock<RecursiveMutex>
 {
     Optional<int> getHeight() override
@@ -247,26 +262,10 @@ public:
         std::unique_ptr<Chain::Lock> result = std::move(lock); // Temporary to avoid CWG 1579
         return result;
     }
-    bool findBlock(const uint256& hash, CBlock* block, int64_t* time, int64_t* time_max) override
+    bool findBlock(const uint256& hash, const FoundBlock& block) override
     {
-        CBlockIndex* index;
-        {
-            LOCK(cs_main);
-            index = LookupBlockIndex(hash);
-            if (!index) {
-                return false;
-            }
-            if (time) {
-                *time = index->GetBlockTime();
-            }
-            if (time_max) {
-                *time_max = index->GetBlockTimeMax();
-            }
-        }
-        if (block && !ReadBlockFromDisk(*block, index, Params().GetConsensus())) {
-            block->SetNull();
-        }
-        return true;
+        WAIT_LOCK(cs_main, lock);
+        return FillBlock(LookupBlockIndex(hash), block, lock);
     }
     void findCoins(std::map<COutPoint, Coin>& coins) override { return FindCoins(m_node, coins); }
     double guessVerificationProgress(const uint256& block_hash) override
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index e1bc9bbbf3..3778ab9a8b 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -30,6 +30,27 @@ namespace interfaces {
 class Handler;
 class Wallet;
 
+//! Helper for findBlock to selectively return pieces of block data.
+class FoundBlock
+{
+public:
+    FoundBlock& hash(uint256& hash) { m_hash = &hash; return *this; }
+    FoundBlock& height(int& height) { m_height = &height; return *this; }
+    FoundBlock& time(int64_t& time) { m_time = &time; return *this; }
+    FoundBlock& maxTime(int64_t& max_time) { m_max_time = &max_time; return *this; }
+    FoundBlock& mtpTime(int64_t& mtp_time) { m_mtp_time = &mtp_time; 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) { m_data = &data; return *this; }
+
+    uint256* m_hash = nullptr;
+    int* m_height = nullptr;
+    int64_t* m_time = nullptr;
+    int64_t* m_max_time = nullptr;
+    int64_t* m_mtp_time = nullptr;
+    CBlock* m_data = nullptr;
+};
+
 //! Interface giving clients (wallet processes, maybe other analysis tools in
 //! the future) ability to access to the chain state, receive notifications,
 //! estimate fees, and submit transactions.
@@ -127,14 +148,7 @@ public:
 
     //! Return whether node has the block and optionally return block metadata
     //! or contents.
-    //!
-    //! If a block pointer is provided to retrieve the block contents, and the
-    //! block exists but doesn't have data (for example due to pruning), the
-    //! block will be empty and all fields set to null.
-    virtual bool findBlock(const uint256& hash,
-        CBlock* block = nullptr,
-        int64_t* time = nullptr,
-        int64_t* max_time = nullptr) = 0;
+    virtual bool findBlock(const uint256& hash, const FoundBlock& block={}) = 0;
 
     //! Look up unspent output information. Returns coins in the mempool and in
     //! the current chain UTXO set. Iterates through all the keys in the map and
diff --git a/src/sync.h b/src/sync.h
index ead2cdc67b..0c6f0ef0a7 100644
--- a/src/sync.h
+++ b/src/sync.h
@@ -210,7 +210,7 @@ public:
      friend class reverse_lock;
 };
 
-#define REVERSE_LOCK(g) decltype(g)::reverse_lock PASTE2(revlock, __COUNTER__)(g, #g, __FILE__, __LINE__)
+#define REVERSE_LOCK(g) typename std::decay<decltype(g)>::type::reverse_lock PASTE2(revlock, __COUNTER__)(g, #g, __FILE__, __LINE__)
 
 template<typename MutexArg>
 using DebugLock = UniqueLock<typename std::remove_reference<typename std::remove_pointer<MutexArg>::type>::type>;
diff --git a/src/test/interfaces_tests.cpp b/src/test/interfaces_tests.cpp
new file mode 100644
index 0000000000..e3d1738c7f
--- /dev/null
+++ b/src/test/interfaces_tests.cpp
@@ -0,0 +1,47 @@
+// Copyright (c) 2020 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <interfaces/chain.h>
+#include <test/util/setup_common.h>
+#include <validation.h>
+
+#include <boost/test/unit_test.hpp>
+
+using interfaces::FoundBlock;
+
+BOOST_FIXTURE_TEST_SUITE(interfaces_tests, TestChain100Setup)
+
+BOOST_AUTO_TEST_CASE(findBlock)
+{
+    auto chain = interfaces::MakeChain(m_node);
+    auto& active = ChainActive();
+
+    uint256 hash;
+    BOOST_CHECK(chain->findBlock(active[10]->GetBlockHash(), FoundBlock().hash(hash)));
+    BOOST_CHECK_EQUAL(hash, active[10]->GetBlockHash());
+
+    int height = -1;
+    BOOST_CHECK(chain->findBlock(active[20]->GetBlockHash(), FoundBlock().height(height)));
+    BOOST_CHECK_EQUAL(height, active[20]->nHeight);
+
+    CBlock data;
+    BOOST_CHECK(chain->findBlock(active[30]->GetBlockHash(), FoundBlock().data(data)));
+    BOOST_CHECK_EQUAL(data.GetHash(), active[30]->GetBlockHash());
+
+    int64_t time = -1;
+    BOOST_CHECK(chain->findBlock(active[40]->GetBlockHash(), FoundBlock().time(time)));
+    BOOST_CHECK_EQUAL(time, active[40]->GetBlockTime());
+
+    int64_t max_time = -1;
+    BOOST_CHECK(chain->findBlock(active[50]->GetBlockHash(), FoundBlock().maxTime(max_time)));
+    BOOST_CHECK_EQUAL(max_time, active[50]->GetBlockTimeMax());
+
+    int64_t mtp_time = -1;
+    BOOST_CHECK(chain->findBlock(active[60]->GetBlockHash(), FoundBlock().mtpTime(mtp_time)));
+    BOOST_CHECK_EQUAL(mtp_time, active[60]->GetMedianTimePast());
+
+    BOOST_CHECK(!chain->findBlock({}, FoundBlock()));
+}
+
+BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 5d34e592db..1e6d37c462 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -37,6 +37,8 @@
 #include <univalue.h>
 
 
+using interfaces::FoundBlock;
+
 static const std::string WALLET_ENDPOINT_BASE = "/wallet/";
 
 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
         entry.pushKV("blockheight", wtx.m_confirm.block_height);
         entry.pushKV("blockindex", wtx.m_confirm.nIndex);
         int64_t block_time;
-        bool found_block = chain.findBlock(wtx.m_confirm.hashBlock, nullptr /* block */, &block_time);
-        CHECK_NONFATAL(found_block);
+        CHECK_NONFATAL(chain.findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(block_time)));
         entry.pushKV("blocktime", block_time);
     } else {
         entry.pushKV("trusted", wtx.IsTrusted(locked_chain));
@@ -1618,7 +1619,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
     UniValue removed(UniValue::VARR);
     while (include_removed && altheight && *altheight > *height) {
         CBlock block;
-        if (!pwallet->chain().findBlock(blockId, &block) || block.IsNull()) {
+        if (!pwallet->chain().findBlock(blockId, FoundBlock().data(block)) || block.IsNull()) {
             throw JSONRPCError(RPC_INTERNAL_ERROR, "Can't read block from disk");
         }
         for (const CTransactionRef& tx : block.vtx) {
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 98f308f927..c8641b03fb 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -22,6 +22,7 @@
 #include <script/script.h>
 #include <script/signingprovider.h>
 #include <util/bip32.h>
+#include <util/check.h>
 #include <util/error.h>
 #include <util/fees.h>
 #include <util/moneystr.h>
@@ -35,6 +36,8 @@
 
 #include <boost/algorithm/string/replace.hpp>
 
+using interfaces::FoundBlock;
+
 const std::map<uint64_t,std::string> WALLET_FLAG_CAVEATS{
     {WALLET_FLAG_AVOID_REUSE,
         "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
         ScanResult result = ScanForWalletTransactions(start_block, {} /* stop_block */, reserver, update);
         if (result.status == ScanResult::FAILURE) {
             int64_t time_max;
-            if (!chain().findBlock(result.last_failed_block, nullptr /* block */, nullptr /* time */, &time_max)) {
-                throw std::logic_error("ScanForWalletTransactions returned invalid block hash");
-            }
+            CHECK_NONFATAL(chain().findBlock(result.last_failed_block, FoundBlock().maxTime(time_max)));
             return time_max + TIMESTAMP_WINDOW + 1;
         }
     }
@@ -1671,7 +1672,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
         }
 
         CBlock block;
-        if (chain().findBlock(block_hash, &block) && !block.IsNull()) {
+        if (chain().findBlock(block_hash, FoundBlock().data(block)) && !block.IsNull()) {
             auto locked_chain = chain().lock();
             LOCK(cs_wallet);
             if (!locked_chain->getBlockHeight(block_hash)) {
@@ -3622,7 +3623,7 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
     unsigned int nTimeSmart = wtx.nTimeReceived;
     if (!wtx.isUnconfirmed() && !wtx.isAbandoned()) {
         int64_t blocktime;
-        if (chain().findBlock(wtx.m_confirm.hashBlock, nullptr /* block */, &blocktime)) {
+        if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(blocktime))) {
             int64_t latestNow = wtx.nTimeReceived;
             int64_t latestEntry = 0;