Solve an incorrect MakeTransactionRef usage for ATMP call inside the wallet and unit tests

src/chain.cpp

@@ -64,6 +64,54 @@ const CBlockIndex* CChain::FindFork(const CBlockIndex* pindex) const
     return pindex;
 }
 
+/** Turn the lowest '1' bit in the binary representation of a number into a '0'. */
+int static inline InvertLowestOne(int n) { return n & (n - 1); }
+
+/** Compute what height to jump back to with the CBlockIndex::pskip pointer. */
+int static inline GetSkipHeight(int height)
+{
+    if (height < 2)
+        return 0;
+    // Determine which height to jump back to. Any number strictly lower than height is acceptable,
+    // but the following expression seems to perform well in simulations (max 110 steps to go back
+    // up to 2**18 blocks).
+    return (height & 1) ? InvertLowestOne(InvertLowestOne(height - 1)) + 1 : InvertLowestOne(height);
+}
+
+CBlockIndex* CBlockIndex::GetAncestor(int height)
+{
+    if (height > nHeight || height < 0)
+        return NULL;
+
+    CBlockIndex* pindexWalk = this;
+    int heightWalk = nHeight;
+    while (heightWalk > height) {
+        int heightSkip = GetSkipHeight(heightWalk);
+        int heightSkipPrev = GetSkipHeight(heightWalk - 1);
+        if (heightSkip == height ||
+            (heightSkip > height && !(heightSkipPrev < heightSkip - 2 && heightSkipPrev >= height))) {
+            // Only follow pskip if pprev->pskip isn't better than pskip->pprev.
+            pindexWalk = pindexWalk->pskip;
+            heightWalk = heightSkip;
+        } else {
+            pindexWalk = pindexWalk->pprev;
+            heightWalk--;
+        }
+    }
+    return pindexWalk;
+}
+
+const CBlockIndex* CBlockIndex::GetAncestor(int height) const
+{
+    return const_cast<CBlockIndex*>(this)->GetAncestor(height);
+}
+
+void CBlockIndex::BuildSkip()
+{
+    if (pprev)
+        pskip = pprev->GetAncestor(GetSkipHeight(nHeight));
+}
+
 CBlockIndex::CBlockIndex(const CBlock& block):
         nVersion{block.nVersion},
         hashMerkleRoot{block.hashMerkleRoot},

src/test/librust/sapling_rpc_wallet_tests.cpp

@@ -415,7 +415,7 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_taddr_to_sapling)
     BOOST_CHECK_EQUAL(0, chainActive.Height());
     CBlock block;
     block.hashPrevBlock = chainActive.Tip()->GetBlockHash();
-    block.vtx.emplace_back(MakeTransactionRef(wtx));
+    block.vtx.emplace_back(wtx.tx);
     block.hashMerkleRoot = BlockMerkleRoot(block);
     auto blockHash = block.GetHash();
     CBlockIndex fakeIndex {block};

src/test/librust/sapling_wallet_tests.cpp

@@ -54,7 +54,7 @@ SaplingOutPoint CreateValidBlock(CWallet& wallet,
     auto saplingNotes = SetSaplingNoteData(wtx);
     wallet.LoadToWallet(wtx);
 
-    block.vtx.emplace_back(MakeTransactionRef(wtx));
+    block.vtx.emplace_back(wtx.tx);
     wallet.IncrementNoteWitnesses(&index, &block, saplingTree);
 
     return saplingNotes[0];
@@ -218,7 +218,7 @@ BOOST_AUTO_TEST_CASE(GetConflictedSaplingNotes) {
     BOOST_CHECK_EQUAL(0, chainActive.Height());
     CBlock block;
     block.hashPrevBlock = chainActive[0]->GetBlockHash();
-    block.vtx.emplace_back(MakeTransactionRef(wtx));
+    block.vtx.emplace_back(wtx.tx);
     block.hashMerkleRoot = BlockMerkleRoot(block);
     const auto& blockHash = block.GetHash();
     CBlockIndex fakeIndex {block};
@@ -341,7 +341,7 @@ BOOST_AUTO_TEST_CASE(SaplingNullifierIsSpent) {
     BOOST_CHECK_EQUAL(0, chainActive.Height());
     CBlock block;
     block.hashPrevBlock = chainActive[0]->GetBlockHash();
-    block.vtx.emplace_back(MakeTransactionRef(wtx));
+    block.vtx.emplace_back(wtx.tx);
     block.hashMerkleRoot = BlockMerkleRoot(block);
     const auto& blockHash = block.GetHash();
     CBlockIndex fakeIndex {block};
@@ -403,7 +403,7 @@ BOOST_AUTO_TEST_CASE(NavigateFromSaplingNullifierToNote) {
     BOOST_CHECK_EQUAL(0, chainActive.Height());
     CBlock block;
     block.hashPrevBlock = chainActive[0]->GetBlockHash();
-    block.vtx.emplace_back(MakeTransactionRef(wtx));
+    block.vtx.emplace_back(wtx.tx);
     block.hashMerkleRoot = BlockMerkleRoot(block);
     const auto& blockHash = block.GetHash();
     CBlockIndex fakeIndex {block};
@@ -499,7 +499,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) {
     BOOST_CHECK_EQUAL(0, chainActive.Height());
     CBlock block;
     block.hashPrevBlock = chainActive[0]->GetBlockHash();
-    block.vtx.emplace_back(MakeTransactionRef(wtx));
+    block.vtx.emplace_back(wtx.tx);
     block.hashMerkleRoot = BlockMerkleRoot(block);
     const auto& blockHash = block.GetHash();
     CBlockIndex fakeIndex {block};
@@ -573,7 +573,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) {
     // Fake-mine this tx into the next block
     BOOST_CHECK_EQUAL(0, chainActive.Height());
     CBlock block2;
-    block2.vtx.emplace_back(MakeTransactionRef(wtx2));
+    block2.vtx.emplace_back(wtx2.tx);
     block.hashMerkleRoot = BlockMerkleRoot(block);
     block2.hashPrevBlock = blockHash;
     auto blockHash2 = block2.GetHash();
@@ -631,7 +631,7 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesEmptyChain) {
     BOOST_CHECK(!(bool) saplingWitnesses[0]);
 
     CBlock block;
-    block.vtx.emplace_back(MakeTransactionRef(wtx));
+    block.vtx.emplace_back(wtx.tx);
     CBlockIndex index(block);
     SaplingMerkleTree saplingTree;
     wallet.IncrementNoteWitnesses(&index, &block, saplingTree);
@@ -691,7 +691,7 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesChainTip) {
         // Second block
         CBlock block2;
         block2.hashPrevBlock = block1.GetHash();
-        block2.vtx.emplace_back(MakeTransactionRef(wtx));
+        block2.vtx.emplace_back(wtx.tx);
         CBlockIndex index2(block2);
         index2.nHeight = 2;
         SaplingMerkleTree saplingTree2 {saplingTree};
@@ -953,7 +953,7 @@ BOOST_AUTO_TEST_CASE(UpdatedSaplingNoteData) {
     // Fake-mine the transaction
     BOOST_CHECK_EQUAL(0, chainActive.Height());
     CBlock block;
-    block.vtx.emplace_back(MakeTransactionRef(wtx));
+    block.vtx.emplace_back(wtx.tx);
     block.hashMerkleRoot = BlockMerkleRoot(block);
     const auto& blockHash = block.GetHash();
     CBlockIndex fakeIndex {block};
@@ -1069,7 +1069,7 @@ BOOST_AUTO_TEST_CASE(MarkAffectedSaplingTransactionsDirty) {
     BOOST_CHECK_EQUAL(0, chainActive.Height());
     SaplingMerkleTree saplingTree;
     CBlock block;
-    block.vtx.emplace_back(MakeTransactionRef(wtx));
+    block.vtx.emplace_back(wtx.tx);
     block.hashMerkleRoot = BlockMerkleRoot(block);
     const auto& blockHash = block.GetHash();
     CBlockIndex fakeIndex {block};

src/validation.cpp

@@ -3349,54 +3349,6 @@ bool AcceptBlock(const CBlock& block, CValidationState& state, CBlockIndex** ppi
     return true;
 }
 
-/** Turn the lowest '1' bit in the binary representation of a number into a '0'. */
-int static inline InvertLowestOne(int n) { return n & (n - 1); }
-
-/** Compute what height to jump back to with the CBlockIndex::pskip pointer. */
-int static inline GetSkipHeight(int height)
-{
-    if (height < 2)
-        return 0;
-    // Determine which height to jump back to. Any number strictly lower than height is acceptable,
-    // but the following expression seems to perform well in simulations (max 110 steps to go back
-    // up to 2**18 blocks).
-    return (height & 1) ? InvertLowestOne(InvertLowestOne(height - 1)) + 1 : InvertLowestOne(height);
-}
-
-CBlockIndex* CBlockIndex::GetAncestor(int height)
-{
-    if (height > nHeight || height < 0)
-        return NULL;
-
-    CBlockIndex* pindexWalk = this;
-    int heightWalk = nHeight;
-    while (heightWalk > height) {
-        int heightSkip = GetSkipHeight(heightWalk);
-        int heightSkipPrev = GetSkipHeight(heightWalk - 1);
-        if (heightSkip == height ||
-            (heightSkip > height && !(heightSkipPrev < heightSkip - 2 && heightSkipPrev >= height))) {
-            // Only follow pskip if pprev->pskip isn't better than pskip->pprev.
-            pindexWalk = pindexWalk->pskip;
-            heightWalk = heightSkip;
-        } else {
-            pindexWalk = pindexWalk->pprev;
-            heightWalk--;
-        }
-    }
-    return pindexWalk;
-}
-
-const CBlockIndex* CBlockIndex::GetAncestor(int height) const
-{
-    return const_cast<CBlockIndex*>(this)->GetAncestor(height);
-}
-
-void CBlockIndex::BuildSkip()
-{
-    if (pprev)
-        pskip = pprev->GetAncestor(GetSkipHeight(nHeight));
-}
-
 bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const std::shared_ptr<const CBlock> pblock, CDiskBlockPos* dbp, bool* fAccepted)
 {
     AssertLockNotHeld(cs_main);

src/wallet/test/wallet_shielded_balances_tests.cpp

@@ -295,7 +295,7 @@ FakeBlock SimpleFakeMine(CWalletTx& wtx, SaplingMerkleTree& currentTree)
 {
     FakeBlock fakeBlock;
     fakeBlock.block.nVersion = 8;
-    fakeBlock.block.vtx.emplace_back(MakeTransactionRef(wtx));
+    fakeBlock.block.vtx.emplace_back(wtx.tx);
     fakeBlock.block.hashMerkleRoot = BlockMerkleRoot(fakeBlock.block);
     for (const OutputDescription& out : wtx.tx->sapData->vShieldedOutput) {
         currentTree.append(out.cmu);

src/wallet/test/wallet_tests.cpp

@@ -435,7 +435,7 @@ BOOST_AUTO_TEST_CASE(cached_balances_tests)
     );
 
     // GetUnconfirmedBalance requires tx in mempool.
-    fakeMempoolInsertion(MakeTransactionRef(wtxCredit));
+    fakeMempoolInsertion(wtxCredit.tx);
     BOOST_CHECK_EQUAL(wallet.GetUnconfirmedBalance(), nCredit);
 
     // 2) Confirm tx and verify

src/wallet/wallet.cpp

@@ -22,7 +22,6 @@
 #include "zpivchain.h"
 
 #include <boost/algorithm/string/replace.hpp>
-#include <boost/thread.hpp>
 
 CWallet* pwalletMain = nullptr;
 /**
@@ -1823,7 +1822,8 @@ void CWallet::ReacceptWalletTransactions(bool fFirstLoad)
         CWalletTx& wtx = *(item.second);
 
         LOCK(mempool.cs);
-        bool fSuccess = wtx.AcceptToMemoryPool(false);
+        CValidationState state;
+        bool fSuccess = wtx.AcceptToMemoryPool(state, false);
         if (!fSuccess && fFirstLoad && GetTime() - wtx.GetTxTime() > 12*60*60) {
             //First load of wallet, failed to accept to mempool, and older than 12 hours... not likely to ever
             //make it in to mempool
@@ -3142,7 +3142,7 @@ CWallet::CommitResult CWallet::CommitTransaction(CWalletTx& wtxNew, CReserveKey*
 
         // Try ATMP. This must not fail. The transaction has already been signed and recorded.
         CValidationState state;
-        if (!AcceptToMemoryPool(mempool, state, MakeTransactionRef(std::move(wtxNew)), true, nullptr, false, true, false)) {
+        if (!wtxNew.AcceptToMemoryPool(state, true, true, false)) {
             res.state = state;
             // Abandon the transaction
             if (AbandonTransaction(res.hashTx)) {
@@ -4275,10 +4275,9 @@ bool CMerkleTx::IsInMainChainImmature() const
 }
 
 
-bool CMerkleTx::AcceptToMemoryPool(bool fLimitFree, bool fRejectInsaneFee, bool ignoreFees)
+bool CMerkleTx::AcceptToMemoryPool(CValidationState& state, bool fLimitFree, bool fRejectInsaneFee, bool ignoreFees)
 {
-    CValidationState state;
-    bool fAccepted = ::AcceptToMemoryPool(mempool, state, MakeTransactionRef(*this), fLimitFree, nullptr, false, fRejectInsaneFee, ignoreFees);
+    bool fAccepted = ::AcceptToMemoryPool(mempool, state, tx, fLimitFree, nullptr, false, fRejectInsaneFee, ignoreFees);
     if (!fAccepted)
         LogPrintf("%s : %s\n", __func__, state.GetRejectReason());
     return fAccepted;

src/wallet/wallet.h

@@ -931,7 +931,7 @@ class CMerkleTx
     bool IsInMainChain() const;
     bool IsInMainChainImmature() const;
     int GetBlocksToMaturity() const;
-    bool AcceptToMemoryPool(bool fLimitFree = true, bool fRejectInsaneFee = true, bool ignoreFees = false);
+    bool AcceptToMemoryPool(CValidationState& state, bool fLimitFree = true, bool fRejectInsaneFee = true, bool ignoreFees = false);
     bool hashUnset() const { return (hashBlock.IsNull() || hashBlock == ABANDON_HASH); }
     bool isAbandoned() const { return (hashBlock == ABANDON_HASH); }
     void setAbandoned() { hashBlock = ABANDON_HASH; }