always generate a valid block in CreateNewBlock(); refactor temporary…

… payloads handling (#26)

src/miner.cpp

@@ -317,6 +317,9 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda
     // Keep track of entries that failed inclusion, to avoid duplicate work
     CTxMemPool::setEntries failedTx;
 
+    // A list of failed PoP transactions to delete after we finish assembling the block
+    CTxMemPool::setEntries failedPopTx;
+
     // Start by adding all descendants of previously added txs to mapModifiedTx
     // and modifying them for their already included ancestors
     UpdatePackagesForAdded(inBlock, mapModifiedTx);
@@ -419,6 +422,19 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda
             continue;
         }
 
+        if (VeriBlock::isPopTx(*iter->GetSharedTx())) {
+            // contextual PoP validation
+            assert(ancestors.size() == 1);
+            TxValidationState txstate;
+            if (nPopTx < config.max_pop_tx_amount &&
+                !VeriBlock::getService<VeriBlock::PopService>().addTemporaryPayloads(iter->GetSharedTx(), *::ChainActive().Tip(), chainparams.GetConsensus(), txstate)) {
+
+                failedTx.insert(iter);
+                failedPopTx.insert(iter);
+                continue;
+            }
+        }
+
         // This transaction will make it in; reset the failed counter.
         nConsecutiveFailed = 0;
 
@@ -439,6 +455,11 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda
         // Update transactions that depend on each of these
         nDescendantsUpdated += UpdatePackagesForAdded(ancestors, mapModifiedTx);
     }
+
+    // delete invalid PoP transactions
+    for (auto & tx: failedPopTx) {
+        mempool.removeRecursive(tx->GetTx(), MemPoolRemovalReason::BLOCK); // FIXME: a more appropriate removal reason
+    }
 }
 
 template void BlockAssembler::addPackageTxs<ancestor_score>(int &nPackagesSelected, int &nDescendantsUpdated);

src/vbk/pop_service.hpp

@@ -29,6 +29,8 @@ using BlockBytes = std::vector<uint8_t>;
 struct PopService {
     virtual ~PopService() = default;
 
+    virtual bool addTemporaryPayloads(const CTransactionRef& tx, const CBlockIndex& pindexPrev, const Consensus::Params& params, TxValidationState& state) = 0;
+
     virtual void clearTemporaryPayloads() = 0;
 
     virtual void savePopTxToDatabase(const CBlock& block, const int& nHeight) = 0;

src/vbk/pop_service/pop_service_impl.cpp

@@ -61,16 +61,6 @@ inline std::string heightToHash(uint32_t height)
     return arith_uint256(height).GetHex();
 }
 
-void clearTemporaryPayloadsImpl(VeriBlock::PopService& pop, uint32_t begin, uint32_t end)
-{
-    for (uint32_t height = end - 1; height >= begin; --height) {
-        try {
-            pop.removePayloads(heightToHash(height), height);
-        } catch (const std::exception& /*ignore*/) {
-        }
-    }
-}
-
 } // namespace
 
 
@@ -101,9 +91,18 @@ PopServiceImpl::PopServiceImpl(bool altautoconfig)
         setConfig();
     }
 
+    assert(getReservedBlockIndexBegin(config) <= getReservedBlockIndexEnd(config) && "oh no, programming error");
+    temporaryPayloadsIndex = getReservedBlockIndexEnd(config);
     clearTemporaryPayloads();
 }
 
+void initTemporaryPayloadsMock(PopServiceImpl& pop)
+{
+    auto& config = VeriBlock::getService<VeriBlock::Config>();
+
+    pop.temporaryPayloadsIndex = getReservedBlockIndexBegin(config);
+}
+
 void PopServiceImpl::addPayloads(std::string blockHash, const int& nHeight, const Publications& publications)
 {
     std::lock_guard<std::mutex> lock(mutex);
@@ -575,18 +574,52 @@ void PopServiceImpl::setConfig()
     }
 }
 
+bool PopServiceImpl::addTemporaryPayloads(const CTransactionRef& tx, const CBlockIndex& pindexPrev, const Consensus::Params& params, TxValidationState& state)
+{
+    return addTemporaryPayloadsImpl(*this, tx, pindexPrev, params, state);
+}
+
+bool addTemporaryPayloadsImpl(PopServiceImpl& pop, const CTransactionRef& tx, const CBlockIndex& pindexPrev, const Consensus::Params& params, TxValidationState& state)
+{
+    if (pop.temporaryPayloadsIndex >= getReservedBlockIndexEnd(VeriBlock::getService<VeriBlock::Config>())) {
+        return state.Invalid(TxValidationResult::TX_BAD_POP_DATA, "pop-block-num-pop-tx", "too many pop transactions in a block");
+    }
+
+    //TODO: need locking
+
+    bool isValid = txPopValidation(pop, tx, pindexPrev, params, state, pop.temporaryPayloadsIndex);
+    if (isValid) {
+        pop.temporaryPayloadsIndex++;
+    }
+
+    return isValid;
+}
+
 void PopServiceImpl::clearTemporaryPayloads()
+{
+    clearTemporaryPayloadsImpl(*this);
+}
+
+void clearTemporaryPayloadsImpl(PopServiceImpl& pop)
 {
     // since we have no transactional interface (yet), we agreed to do the following instead:
     // 1. during block pop validation, execute addPayloads with heights from reserved block heights - this changes the state of alt-service and does all validations.
     // 2. since it changes state, after validation we want to rollback. execute removePayloads on all reserved heights to do that
     // 3. if node was shutdown during block pop validation, also clear all payloads within given height range
     // range is [uint32_t::max() - pop_tx_amount_per_block...uint32_t::max); total size is config.pop_tx_amount
 
-    auto& config = getService<Config>();
-    clearTemporaryPayloadsImpl(*this, getReservedBlockIndexBegin(config), getReservedBlockIndexEnd(config));
-}
+    const auto begin = getReservedBlockIndexBegin(getService<Config>());
+    const auto end = pop.temporaryPayloadsIndex;
 
+    for (uint32_t height = end - 1; height >= begin; --height) {
+        try {
+            pop.removePayloads(heightToHash(height), height);
+        } catch (const std::exception& /*ignore*/) {
+        }
+    }
+
+    pop.temporaryPayloadsIndex = begin;
+}
 
 bool txPopValidation(PopServiceImpl& pop, const CTransactionRef& tx, const CBlockIndex& pindexPrev, const Consensus::Params& params, TxValidationState& state, uint32_t heightIndex) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
@@ -667,13 +700,6 @@ bool blockPopValidationImpl(PopServiceImpl& pop, const CBlock& block, const CBlo
     const auto& config = getService<Config>();
     size_t numOfPopTxes = 0;
 
-    // block index here works as a database transaction ID
-    // if given tx is invalid, we need to rollback the alt-service state
-    // and to know which exactly - we use BlockIndex
-    const auto blockIndexBegin = getReservedBlockIndexBegin(config);
-    auto blockIndexIt = blockIndexBegin;
-    const auto blockIndexEnd = getReservedBlockIndexEnd(config);
-
     LOCK(mempool.cs);
     AssertLockHeld(mempool.cs);
     AssertLockHeld(cs_main);
@@ -684,23 +710,22 @@ bool blockPopValidationImpl(PopServiceImpl& pop, const CBlock& block, const CBlo
         }
 
         if (++numOfPopTxes > config.max_pop_tx_amount) {
-            pop.clearTemporaryPayloads();
+            clearTemporaryPayloadsImpl(pop);
             return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "pop-block-num-pop-tx", "too many pop transactions in a block");
         }
 
         TxValidationState txstate;
-        assert(blockIndexBegin <= blockIndexEnd && "oh no, programming error");
 
-        if (!txPopValidation(pop, tx, pindexPrev, params, txstate, blockIndexIt++)) {
-            clearTemporaryPayloadsImpl(pop, getReservedBlockIndexBegin(config), blockIndexIt);
+        if (!addTemporaryPayloadsImpl(pop, tx, pindexPrev, params, txstate)) {
+            clearTemporaryPayloadsImpl(pop);
             mempool.removeRecursive(*tx, MemPoolRemovalReason::BLOCK);
             return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, txstate.GetRejectReason(), txstate.GetDebugMessage());
         }
     }
 
     // because this is validation, we need to clear current temporary payloads
-    // actual addPayloads call is performed in savePopTxToDatabase
-    clearTemporaryPayloadsImpl(pop, getReservedBlockIndexBegin(config), blockIndexIt);
+    // actual addPayloads call is performed in ConnectTip
+    clearTemporaryPayloadsImpl(pop);
 
     return true;
 }

src/vbk/pop_service/pop_service_impl.hpp

@@ -24,10 +24,16 @@ class PopServiceImpl : public PopService
     std::shared_ptr<VeriBlock::GrpcPopService::Stub> grpcPopService;
 
 public:
+
+    // FIXME: have to make it public so that it could be accessed in mocks
+    // the index of the last temporary payloads applied to the alt-integration blockchain view
+    uint32_t temporaryPayloadsIndex;
+
     PopServiceImpl(bool altautoconfig = false);
 
     ~PopServiceImpl() override = default;
 
+    bool addTemporaryPayloads(const CTransactionRef& tx, const CBlockIndex& pindexPrev, const Consensus::Params& params, TxValidationState& state) override;
     void clearTemporaryPayloads() override;
 
     void savePopTxToDatabase(const CBlock& block, const int& nHeight) override;
@@ -66,5 +72,10 @@ bool blockPopValidationImpl(PopServiceImpl& pop, const CBlock& block, const CBlo
 
 bool txPopValidation(PopServiceImpl& pop, const CTransactionRef& tx, const CBlockIndex& pindexPrev, const Consensus::Params& params, TxValidationState& state, uint32_t heightIndex);
 
+// FIXME: an ugly crutch for tests
+bool addTemporaryPayloadsImpl(PopServiceImpl& pop, const CTransactionRef& tx, const CBlockIndex& pindexPrev, const Consensus::Params& params, TxValidationState& state);
+void clearTemporaryPayloadsImpl(PopServiceImpl& pop);
+void initTemporaryPayloadsMock(PopServiceImpl& pop);
+
 } // namespace VeriBlock
 #endif //BITCOIN_SRC_VBK_POP_SERVICE_POP_SERVICE_IMPL_HPP

src/vbk/test/unit/block_validation_tests.cpp

@@ -57,13 +57,20 @@ struct BlockValidationFixture : public TestChain100Setup {
         Fake(OverloadedMethod(pop_impl_mock, addPayloads, void(std::string, const int&, const VeriBlock::Publications&)));
         Fake(OverloadedMethod(pop_impl_mock, removePayloads, void(std::string, const int&)));
         Fake(Method(pop_impl_mock, updateContext));
-        Fake(Method(pop_impl_mock, clearTemporaryPayloads));
 
         When(Method(pop_service_mock, checkVTBinternally)).AlwaysReturn(true);
         When(Method(pop_service_mock, checkATVinternally)).AlwaysReturn(true);
         When(Method(pop_service_mock, blockPopValidation)).AlwaysDo([&](const CBlock& block, const CBlockIndex& pindexPrev, const Consensus::Params& params, BlockValidationState& state) -> bool {
             return VeriBlock::blockPopValidationImpl(pop_impl_mock.get(), block, pindexPrev, params, state);
         });
+
+        When(Method(pop_impl_mock, addTemporaryPayloads)).AlwaysDo([&](const CTransactionRef& tx, const CBlockIndex& pindexPrev, const Consensus::Params& params, TxValidationState& state) {
+                return VeriBlock::addTemporaryPayloadsImpl(pop_impl_mock.get(), tx, pindexPrev, params, state);
+            });
+        When(Method(pop_impl_mock, clearTemporaryPayloads)).AlwaysDo([&]() {
+                VeriBlock::clearTemporaryPayloadsImpl(pop_impl_mock.get());
+            });
+        VeriBlock::initTemporaryPayloadsMock(pop_impl_mock.get());
     };
 
     std::shared_ptr<CDataStream> stream;

src/vbk/test/unit/pop_service_tests.cpp

@@ -52,7 +52,25 @@ struct PopServiceFixture : public TestChain100Setup {
                 }
                 return true;
             });
+
         When(Method(pop_service_impl_mock, determineATVPlausibilityWithBTCRules)).AlwaysReturn(true);
+
+        When(Method(pop_service_impl_mock, addTemporaryPayloads)).AlwaysDo([&](const CTransactionRef& tx, const CBlockIndex& pindexPrev, const Consensus::Params& params, TxValidationState& state) {
+                return VeriBlock::addTemporaryPayloadsImpl(pop_service_impl_mock.get(), tx, pindexPrev, params, state);
+            });
+        When(Method(pop_service_impl_mock, clearTemporaryPayloads)).AlwaysDo([&]() {
+                VeriBlock::clearTemporaryPayloadsImpl(pop_service_impl_mock.get());
+            });
+        VeriBlock::initTemporaryPayloadsMock(pop_service_impl_mock.get());
+    }
+
+    void verifyNoAddRemovePayloads()
+    {
+        Verify_Method(OverloadedMethod(pop_service_impl_mock, addPayloads, void(std::string, const int&, const VeriBlock::Publications&))).Never();
+        Verify_Method(OverloadedMethod(pop_service_impl_mock, addPayloads, void(const CBlockIndex&, const CBlock&))).Never();
+
+        Verify_Method(OverloadedMethod(pop_service_impl_mock, removePayloads, void(std::string, const int&))).Never();
+        Verify_Method(OverloadedMethod(pop_service_impl_mock, removePayloads, void(const CBlockIndex&))).Never();
     }
 };
 
@@ -75,7 +93,6 @@ BOOST_FIXTURE_TEST_CASE(blockPopValidation_test, PopServiceFixture)
             setPublicationData(publicationData, stream, config.index.unwrap());
         });
 
-
     BlockValidationState state;
     {
         LOCK(cs_main);
@@ -110,7 +127,8 @@ BOOST_FIXTURE_TEST_CASE(blockPopValidation_test_wrong_index, PopServiceFixture)
         BOOST_CHECK(!blockPopValidationImpl(pop_service_impl_mock.get(), block, *ChainActive().Tip()->pprev, Params().GetConsensus(), state));
         BOOST_CHECK_EQUAL(state.GetRejectReason(), "pop-tx-altchain-id");
     }
-    Verify_Method(OverloadedMethod(pop_service_impl_mock, removePayloads, void(std::string, const int&))).Once();
+
+    verifyNoAddRemovePayloads();
 }
 
 BOOST_FIXTURE_TEST_CASE(blockPopValidation_test_endorsed_block_not_known_orphan_block, PopServiceFixture)
@@ -137,7 +155,7 @@ BOOST_FIXTURE_TEST_CASE(blockPopValidation_test_endorsed_block_not_known_orphan_
         BOOST_CHECK(!blockPopValidationImpl(pop_service_impl_mock.get(), block, *ChainActive().Tip()->pprev, Params().GetConsensus(), state));
         BOOST_CHECK_EQUAL(state.GetRejectReason(), "pop-tx-endorsed-block-not-known-orphan-block");
     }
-    Verify_Method(OverloadedMethod(pop_service_impl_mock, removePayloads, void(std::string, const int&))).Once();
+    verifyNoAddRemovePayloads();
 }
 
 BOOST_FIXTURE_TEST_CASE(blockPopValidation_test_endorsed_block_not_from_chain, PopServiceFixture)
@@ -174,7 +192,7 @@ BOOST_FIXTURE_TEST_CASE(blockPopValidation_test_endorsed_block_not_from_chain, P
         BOOST_CHECK(!blockPopValidationImpl(pop_service_impl_mock.get(), block, *ChainActive().Tip()->pprev, Params().GetConsensus(), state));
         BOOST_CHECK_EQUAL(state.GetRejectReason(), "pop-tx-endorsed-block-not-from-this-chain");
     }
-    Verify_Method(OverloadedMethod(pop_service_impl_mock, removePayloads, void(std::string, const int&))).Once();
+    verifyNoAddRemovePayloads();
 }
 
 BOOST_FIXTURE_TEST_CASE(blockPopValidation_test_wrong_settlement_interval, PopServiceFixture)
@@ -203,7 +221,7 @@ BOOST_FIXTURE_TEST_CASE(blockPopValidation_test_wrong_settlement_interval, PopSe
         BOOST_CHECK(!blockPopValidationImpl(pop_service_impl_mock.get(), block, *ChainActive().Tip()->pprev, Params().GetConsensus(), state));
         BOOST_CHECK_EQUAL(state.GetRejectReason(), "pop-tx-endorsed-block-too-old");
     }
-    Verify_Method(OverloadedMethod(pop_service_impl_mock, removePayloads, void(std::string, const int&))).Once();
+    verifyNoAddRemovePayloads();
 }
 
 BOOST_FIXTURE_TEST_CASE(blockPopValidation_test_wrong_addPayloads, PopServiceFixture)
@@ -233,6 +251,11 @@ BOOST_FIXTURE_TEST_CASE(blockPopValidation_test_wrong_addPayloads, PopServiceFix
         BOOST_CHECK(!blockPopValidationImpl(pop_service_impl_mock.get(), block, *ChainActive().Tip()->pprev, Params().GetConsensus(), state));
         BOOST_CHECK_EQUAL(state.GetRejectReason(), "pop-tx-add-payloads-failed");
     }
-    Verify_Method(OverloadedMethod(pop_service_impl_mock, removePayloads, void(std::string, const int&))).Once();
+
+    Verify_Method(OverloadedMethod(pop_service_impl_mock, addPayloads, void(std::string, const int&, const VeriBlock::Publications&))).Once();
+    Verify_Method(OverloadedMethod(pop_service_impl_mock, addPayloads, void(const CBlockIndex&, const CBlock&))).Never();
+
+    Verify_Method(OverloadedMethod(pop_service_impl_mock, removePayloads, void(std::string, const int&))).Never();
+    Verify_Method(OverloadedMethod(pop_service_impl_mock, removePayloads, void(const CBlockIndex&))).Never();
 }
 BOOST_AUTO_TEST_SUITE_END()

src/vbk/test/unit/updated_mempool_tests.cpp

@@ -288,8 +288,18 @@ BOOST_FIXTURE_TEST_CASE(check_CreateNewBlock_with_blockPopValidation_fail, Testi
    Fake(Method(pop_service_mock, clearTemporaryPayloads));
    When(Method(pop_service_mock, determineATVPlausibilityWithBTCRules)).AlwaysReturn(true);
 
-   When(Method(pop_service_mock, blockPopValidation)).AlwaysDo([](const CBlock& block, const CBlockIndex& pindexPrev, const Consensus::Params& params, BlockValidationState& state) -> bool { return VeriBlock::blockPopValidationImpl((VeriBlock::PopServiceImpl&)VeriBlock::getService<VeriBlock::PopService>(), block, pindexPrev, params, state); });
-   When(Method(pop_service_mock, parsePopTx)).AlwaysDo([](const CTransactionRef&, ScriptError* serror, VeriBlock::Publications*, VeriBlock::Context*, VeriBlock::PopTxType* type) -> bool {
+   fakeit::Mock<VeriBlock::PopServiceImpl> pop_impl_mock;
+
+   When(Method(pop_service_mock, blockPopValidation)).AlwaysDo(
+       [&](const CBlock& block, const CBlockIndex& pindexPrev, const Consensus::Params& params, BlockValidationState& state) {
+            return VeriBlock::blockPopValidationImpl(pop_impl_mock.get(), block, pindexPrev, params, state);
+        });
+    When(Method(pop_service_mock, addTemporaryPayloads)).AlwaysDo(
+        [&](const CTransactionRef& tx, const CBlockIndex& pindexPrev, const Consensus::Params& params, TxValidationState& state) {
+            return VeriBlock::addTemporaryPayloadsImpl(pop_impl_mock.get(), tx, pindexPrev, params, state);
+        });
+
+   When(Method(pop_impl_mock, parsePopTx)).AlwaysDo([](const CTransactionRef&, ScriptError* serror, VeriBlock::Publications*, VeriBlock::Context*, VeriBlock::PopTxType* type) -> bool {
        if (type != nullptr) {
            *type = VeriBlock::PopTxType::CONTEXT;
        }
@@ -298,8 +308,19 @@ BOOST_FIXTURE_TEST_CASE(check_CreateNewBlock_with_blockPopValidation_fail, Testi
        }
        return true; });
 
+    When(Method(pop_impl_mock, addTemporaryPayloads)).AlwaysDo([&](const CTransactionRef& tx, const CBlockIndex& pindexPrev, const Consensus::Params& params, TxValidationState& state) {
+            return VeriBlock::addTemporaryPayloadsImpl(pop_impl_mock.get(), tx, pindexPrev, params, state);
+        });
+    When(Method(pop_impl_mock, clearTemporaryPayloads)).AlwaysDo([&]() {
+                VeriBlock::clearTemporaryPayloadsImpl(pop_impl_mock.get());
+            });
+    VeriBlock::initTemporaryPayloadsMock(pop_impl_mock.get());
+
+    Fake(OverloadedMethod(pop_impl_mock, removePayloads, void(std::string, const int&)));
+
    // Simulate that we have 8 invalid popTxs
-   When(Method(pop_service_mock, updateContext)).Throw(8_Times(VeriBlock::PopServiceException("fail"))).AlwaysDo([](const std::vector<std::vector<uint8_t>>& veriBlockBlocks, const std::vector<std::vector<uint8_t>>& bitcoinBlocks) {});
+   When(Method(pop_impl_mock, updateContext)).Throw(8_Times(VeriBlock::PopServiceException("fail"))).AlwaysDo(
+       [](const std::vector<std::vector<uint8_t>>& veriBlockBlocks, const std::vector<std::vector<uint8_t>>& bitcoinBlocks) {});
 
    const size_t popTxCount = 10;
 
@@ -313,21 +334,11 @@ BOOST_FIXTURE_TEST_CASE(check_CreateNewBlock_with_blockPopValidation_fail, Testi
    BlockAssembler blkAssembler(Params());
    CScript scriptPubKey = CScript() << OP_CHECKSIG;
 
-   // repeatedly attempt to create a new block until either it
-   // succeeds or we make a suspiciously large number of attempts
    // intentionally generate the correct block mutiple times to make sure it's repeatable
-   bool success = false;
    for(size_t i = 0; i < popTxCount*2; i++) {
-       try {
-           std::unique_ptr<CBlockTemplate> pblockTemplate = blkAssembler.CreateNewBlock(scriptPubKey);
-
-           BOOST_TEST(pblockTemplate->block.vtx.size() == 3);
-
-           success = true;
-       }
-       catch (const std::runtime_error&) {}
+        std::unique_ptr<CBlockTemplate> pblockTemplate = blkAssembler.CreateNewBlock(scriptPubKey);
+        BOOST_TEST(pblockTemplate->block.vtx.size() == 3);
    }
-   BOOST_TEST(success);
 }
 
 BOOST_AUTO_TEST_SUITE_END()