Merge #1875: BugFix: tx sync parsing problem fixed + extra refactoring.

9d8b8a2 BugFix: CMutableTransaction following the same sapling nVersion rules as CTransaction. (furszy)
1ca1b89 PeerLogicValidation missing destructor declaration warning fix. (furszy)
093a766 Introduce convenience type CTransactionRef (furszy)
8345a87 Refactor: make the read function simpler (gnuser)

Pull request description:

  PR solving the current master's syncing issue (syncing process not passing through block 5840), solved in 9d8b8a2.

  Essentially, when we merged #1815, we moved from using the default transaction constructor and the ser/unser template methods (`SerializationOp`) to be using, inside the templated serialization puzzle, the deserializing constructor (`CTransaction(deserialize_type, Stream& s)`) which internally creates a `CMutableTransaction` which wasn't having the same sapling tx version guard as `CTransaction` ser/unser method. So, in other words, it was trying to parse the shielded transaction data from an old version two transaction (yes, we already have version two transaction in our network.. first one is in block 5840).

  Plus, i took the mischief of not only including the bugfix, have added:

  * stream::read function readability improvement coming from bitcoin#11221.
  * a pretty straightforward adaptation of upstream's b4e4ba4  (missing last commit not included in #1815).
  * a `PeerLogicValidation` class compiler warning fix 1ca1b89

ACKs for top commit:
  random-zebra:
    ACK 9d8b8a2
  Fuzzbawls:
    ACK 9d8b8a2

Tree-SHA512: ffb60001160d177d20da3d9198f73910921ec521e6cae1ccbf9f6359dc96e3e4fdbbcaabb85331ce2a8b839c07ab34b3981c69d83aa6990aa79e134588539f48

src/main.h

@@ -439,6 +439,7 @@ class PeerLogicValidation : public CValidationInterface {
 
 public:
     PeerLogicValidation(CConnman* connmanIn);
+    ~PeerLogicValidation() = default;
 
     virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload);
     virtual void BlockChecked(const CBlock& block, const CValidationState& state);

src/primitives/block.h

@@ -82,7 +82,7 @@ class CBlock : public CBlockHeader
 {
 public:
     // network and disk
-    std::vector<std::shared_ptr<const CTransaction>> vtx;
+    std::vector<CTransactionRef> vtx;
 
     // ppcoin: block signature - signed by one of the coin base txout[N]'s owner
     std::vector<unsigned char> vchBlockSig;

src/primitives/transaction.h

@@ -393,7 +393,7 @@ struct CMutableTransaction
         READWRITE(vout);
         READWRITE(nLockTime);
 
-        if (nVersion == CTransaction::SAPLING_VERSION) {
+        if (g_IsSaplingActive && nVersion == CTransaction::SAPLING_VERSION) {
             READWRITE(*const_cast<Optional<SaplingTxData>*>(&sapData));
         }
     }
@@ -411,4 +411,10 @@ struct CMutableTransaction
     std::string ToString() const;
 };
 
+typedef std::shared_ptr<const CTransaction> CTransactionRef;
+static inline CTransactionRef MakeTransactionRef() { return std::make_shared<const CTransaction>(); }
+template <typename Tx> static inline CTransactionRef MakeTransactionRef(Tx&& txIn) { return std::make_shared<const CTransaction>(std::forward<Tx>(txIn)); }
+static inline CTransactionRef MakeTransactionRef(const CTransactionRef& txIn) { return txIn; }
+static inline CTransactionRef MakeTransactionRef(CTransactionRef&& txIn) { return std::move(txIn); }
+
 #endif // BITCOIN_PRIMITIVES_TRANSACTION_H

src/streams.h

@@ -224,16 +224,15 @@ class CBaseDataStream
 
         // Read from the beginning of the buffer
         unsigned int nReadPosNext = nReadPos + nSize;
-        if (nReadPosNext >= vch.size()) {
-            if (nReadPosNext > vch.size()) {
-                throw std::ios_base::failure("CBaseDataStream::read() : end of data");
-            }
-            memcpy(pch, &vch[nReadPos], nSize);
+        if (nReadPosNext > vch.size()) {
+            throw std::ios_base::failure("CDataStream::read(): end of data");
+        }
+        memcpy(pch, &vch[nReadPos], nSize);
+        if (nReadPosNext == vch.size()) {
             nReadPos = 0;
             vch.clear();
             return;
         }
-        memcpy(pch, &vch[nReadPos], nSize);
         nReadPos = nReadPosNext;
     }
 

src/test/mempool_tests.cpp

@@ -443,7 +443,7 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest)
     pool.addUnchecked(tx5.GetHash(), entry.Fee(1000LL).FromTx(tx5, &pool));
     pool.addUnchecked(tx7.GetHash(), entry.Fee(9000LL).FromTx(tx7, &pool));
 
-    std::vector<std::shared_ptr<const CTransaction>> vtx;
+    std::vector<CTransactionRef> vtx;
     std::list<CTransaction> conflicts;
     SetMockTime(42);
     SetMockTime(42 + CTxMemPool::ROLLING_FEE_HALFLIFE);

src/test/merkle_tests.cpp

@@ -15,7 +15,7 @@ static uint256 BlockBuildMerkleTree(const CBlock& block, bool* fMutated, std::ve
 {
     vMerkleTree.clear();
     vMerkleTree.reserve(block.vtx.size() * 2 + 16); // Safe upper bound for the number of total nodes.
-    for (std::vector<std::shared_ptr<const CTransaction>>::const_iterator it(block.vtx.begin()); it != block.vtx.end(); ++it)
+    for (std::vector<CTransactionRef>::const_iterator it(block.vtx.begin()); it != block.vtx.end(); ++it)
         vMerkleTree.push_back((*it)->GetHash());
     int j = 0;
     bool mutated = false;

src/test/policyestimator_tests.cpp

@@ -45,7 +45,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
     CFeeRate baseRate(basefee, ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION));
 
     // Create a fake block
-    std::vector<std::shared_ptr<const CTransaction>> block;
+    std::vector<CTransactionRef> block;
     int blocknum = 0;
 
     // Loop through 200 blocks

src/txmempool.cpp

@@ -542,7 +542,7 @@ void CTxMemPool::removeConflicts(const CTransaction& tx, std::list<CTransaction>
 /**
  * Called when a block is connected. Removes from mempool and updates the miner fee estimator.
  */
-void CTxMemPool::removeForBlock(const std::vector<std::shared_ptr<const CTransaction>>& vtx, unsigned int nBlockHeight, std::list<CTransaction>& conflicts, bool fCurrentEstimate)
+void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight, std::list<CTransaction>& conflicts, bool fCurrentEstimate)
 {
     LOCK(cs);
     std::vector<CTxMemPoolEntry> entries;

src/txmempool.h

@@ -478,7 +478,7 @@ class CTxMemPool
     void remove(const CTransaction& tx, std::list<CTransaction>& removed, bool fRecursive = false);
     void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags);
     void removeConflicts(const CTransaction& tx, std::list<CTransaction>& removed);
-    void removeForBlock(const std::vector<std::shared_ptr<const CTransaction>>& vtx, unsigned int nBlockHeight, std::list<CTransaction>& conflicts, bool fCurrentEstimate = true);
+    void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight, std::list<CTransaction>& conflicts, bool fCurrentEstimate = true);
     void clear();
     void _clear();  // lock-free
     void queryHashes(std::vector<uint256>& vtxid);