diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -544,7 +544,6 @@ avalanche/avalanche.cpp avalanche/delegation.cpp avalanche/delegationbuilder.cpp - avalanche/orphanproofpool.cpp avalanche/peermanager.cpp avalanche/processor.cpp avalanche/proof.cpp diff --git a/src/avalanche/orphanproofpool.h b/src/avalanche/orphanproofpool.h deleted file mode 100644 --- a/src/avalanche/orphanproofpool.h +++ /dev/null @@ -1,90 +0,0 @@ -// Copyright (c) 2021 The Bitcoin developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. -#ifndef BITCOIN_AVALANCHE_ORPHANPROOFPOOL_H -#define BITCOIN_AVALANCHE_ORPHANPROOFPOOL_H - -#include - -#include -#include -#include - -#include - -namespace avalanche { - -class PeerManager; - -// Extracts a ProofId from a Proof -struct proofid_extractor { - using result_type = ProofId; - result_type operator()(const ProofRef &proof) const { - return proof->getId(); - } -}; - -struct by_sequence {}; -struct by_proofid {}; - -using ProofContainer = boost::multi_index_container< - ProofRef, - boost::multi_index::indexed_by< - // keep insertion order - boost::multi_index::sequenced>, - // indexed by proofid - boost::multi_index::hashed_unique, - proofid_extractor, - SaltedProofIdHasher>>>; - -/** - * OrphanProofPool stores orphan proofs waiting for their UTXOs to be - * discovered. - * The pool has a size limit. When the pool is full, the oldest proof is - * removed when a new one is added. - */ -class OrphanProofPool { - ProofContainer proofs; - - const size_t maxNumberOfStakes; - size_t nStakes = 0; - - /** - * Trim the proof pool to given max size. - * It the current size is <= max size this has no effect. - */ - void trimToMaximumSize(); - -public: - OrphanProofPool(size_t maxNumberOfStakes) - : maxNumberOfStakes(maxNumberOfStakes) {} - - /** - * Add a proof to the pool. - * The caller is responsible for checking the proof. - */ - bool addProof(const ProofRef &proof); - - /** Remove a proof from the pool. */ - bool removeProof(const ProofId &proofId); - - /** - * Get a pointer to a proof by id, or nullptr if the proof is not in the - * pool. - */ - ProofRef getProof(const ProofId &proofId) const; - - /** - * Rescan the pool to remove previously orphaned proofs that have become - * good or permanently bad. - */ - void rescan(PeerManager &peerManager); - - // For testing - size_t getNProofs() const; - size_t getNStakes() const; -}; - -} // namespace avalanche - -#endif // BITCOIN_AVALANCHE_ORPHANPROOFPOOL_H diff --git a/src/avalanche/orphanproofpool.cpp b/src/avalanche/orphanproofpool.cpp deleted file mode 100644 --- a/src/avalanche/orphanproofpool.cpp +++ /dev/null @@ -1,66 +0,0 @@ -// Copyright (c) 2021 The Bitcoin developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#include - -#include - -#include - -namespace avalanche { - -void OrphanProofPool::trimToMaximumSize() { - auto &proofs_by_sequence = proofs.get(); - auto it = proofs_by_sequence.begin(); - while (nStakes > maxNumberOfStakes) { - nStakes -= (*it)->getStakes().size(); - it = proofs_by_sequence.erase(it); - } -} - -bool OrphanProofPool::addProof(const ProofRef &proof) { - size_t nStakesProof = proof->getStakes().size(); - if (!proofs.push_back(proof).second) { - return false; - } - nStakes += nStakesProof; - trimToMaximumSize(); - return true; -} - -bool OrphanProofPool::removeProof(const ProofId &proofId) { - auto &proofs_by_id = proofs.get(); - auto it = proofs_by_id.find(proofId); - if (it == proofs_by_id.end()) { - return false; - } - nStakes -= (*it)->getStakes().size(); - proofs_by_id.erase(it); - return true; -} - -ProofRef OrphanProofPool::getProof(const ProofId &proofId) const { - auto &proofs_by_proofid = proofs.get(); - auto it = proofs_by_proofid.find(proofId); - return it == proofs_by_proofid.end() ? nullptr : *it; -} - -void OrphanProofPool::rescan(PeerManager &peerManager) { - ProofContainer last_gen_proofs = std::move(proofs); - proofs.clear(); - - for (auto &proof : last_gen_proofs.get()) { - peerManager.registerProof(proof); - } -} - -size_t OrphanProofPool::getNProofs() const { - return proofs.size(); -} - -size_t OrphanProofPool::getNStakes() const { - return nStakes; -} - -} // namespace avalanche diff --git a/src/avalanche/peermanager.h b/src/avalanche/peermanager.h --- a/src/avalanche/peermanager.h +++ b/src/avalanche/peermanager.h @@ -6,7 +6,6 @@ #define BITCOIN_AVALANCHE_PEERMANAGER_H #include -#include #include #include #include @@ -29,13 +28,6 @@ namespace avalanche { -/** - * Maximum number of stakes in the orphanProofs. - * Benchmarking on a consumer grade computer shows that 10000 stakes can be - * verified in less than 1 second. - */ -static constexpr size_t AVALANCHE_ORPHANPROOFPOOL_SIZE = 10000; - class Delegation; namespace { @@ -133,6 +125,7 @@ PeerSet peers; ProofPool validProofPool; + ProofPool orphanProofPool; using NodeSet = boost::multi_index_container< Node, @@ -165,11 +158,6 @@ static constexpr int SELECT_PEER_MAX_RETRY = 3; static constexpr int SELECT_NODE_MAX_RETRY = 3; - /** - * Tracks proof which for which the UTXO are unavailable. - */ - OrphanProofPool orphanProofs{AVALANCHE_ORPHANPROOFPOOL_SIZE}; - /** * Track proof ids to broadcast */ diff --git a/src/avalanche/peermanager.cpp b/src/avalanche/peermanager.cpp --- a/src/avalanche/peermanager.cpp +++ b/src/avalanche/peermanager.cpp @@ -161,7 +161,7 @@ cs_main, return proof->verify(state, ::ChainstateActive().CoinsTip())); if (!valid) { if (isOrphanState(state)) { - orphanProofs.addProof(proof); + orphanProofPool.addProof(proof); } // Reject invalid proof. @@ -220,10 +220,10 @@ removePeer(pid); } - orphanProofs.rescan(*this); + orphanProofPool.rescan(*this); for (auto &p : newOrphans) { - orphanProofs.addProof(p); + orphanProofPool.addProof(p); } } @@ -236,7 +236,7 @@ }); if (!proof) { - proof = orphanProofs.getProof(proofid); + proof = orphanProofPool.getProof(proofid); } return proof; @@ -248,7 +248,7 @@ } bool PeerManager::isOrphan(const ProofId &proofid) const { - return orphanProofs.getProof(proofid) != nullptr; + return orphanProofPool.getProof(proofid) != nullptr; } bool PeerManager::createPeer(const ProofRef &proof) { @@ -258,7 +258,7 @@ case ProofPool::AddProofStatus::REJECTED: // The proof has conflicts, orphan the proof so it can be pulled // back if the conflicting ones are invalidated. - orphanProofs.addProof(proof); + orphanProofPool.addProof(proof); return false; case ProofPool::AddProofStatus::DUPLICATED: // If the proof was already in the pool, don't duplicate the peer. diff --git a/src/avalanche/test/CMakeLists.txt b/src/avalanche/test/CMakeLists.txt --- a/src/avalanche/test/CMakeLists.txt +++ b/src/avalanche/test/CMakeLists.txt @@ -14,7 +14,6 @@ TESTS delegation_tests.cpp - orphanproofpool_tests.cpp peermanager_tests.cpp processor_tests.cpp proof_tests.cpp diff --git a/src/avalanche/test/orphanproofpool_tests.cpp b/src/avalanche/test/orphanproofpool_tests.cpp deleted file mode 100644 --- a/src/avalanche/test/orphanproofpool_tests.cpp +++ /dev/null @@ -1,163 +0,0 @@ -// Copyright (c) 2021 The Bitcoin developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#include -#include - -#include - -#include - -#include - -#include -#include - -using namespace avalanche; - -BOOST_FIXTURE_TEST_SUITE(orphanproofpool_tests, TestingSetup) - -/** Make a proof with stakes using random txids */ -static ProofRef makeProof(const size_t nStakes) { - const Amount v = 5 * COIN; - const int height = 1234; - ProofBuilder pb(0, 0, CKey::MakeCompressedKey()); - for (size_t i = 0; i < nStakes; i++) { - TxId txid(GetRandHash()); - BOOST_CHECK(pb.addUTXO(COutPoint(txid, 0), v, height, false, - CKey::MakeCompressedKey())); - } - return pb.build(); -} - -BOOST_AUTO_TEST_CASE(pool_starts_empty) { - OrphanProofPool pool{10}; - BOOST_CHECK_EQUAL(pool.getNProofs(), 0); - BOOST_CHECK_EQUAL(pool.getNStakes(), 0); -} - -BOOST_AUTO_TEST_CASE(fail_to_add_same_proof_twice) { - OrphanProofPool pool{10}; - auto p = makeProof(1); - BOOST_CHECK(!pool.getProof(p->getId())); - - BOOST_CHECK(pool.addProof(p)); - BOOST_CHECK_EQUAL(pool.getNStakes(), 1); - BOOST_CHECK_EQUAL(pool.getNProofs(), 1); - BOOST_CHECK(pool.getProof(p->getId())); - - BOOST_CHECK(!pool.addProof(p)); - BOOST_CHECK_EQUAL(pool.getNStakes(), 1); - BOOST_CHECK_EQUAL(pool.getNProofs(), 1); - BOOST_CHECK(pool.getProof(p->getId())); -} - -BOOST_AUTO_TEST_CASE(check_eviction_behavior) { - { - // Fill the pool - OrphanProofPool pool{7}; - auto first = makeProof(4); - pool.addProof(first); - pool.addProof(makeProof(2)); - pool.addProof(makeProof(1)); - BOOST_CHECK_EQUAL(pool.getNStakes(), 7); - BOOST_CHECK_EQUAL(pool.getNProofs(), 3); - BOOST_CHECK(pool.getProof(first->getId())); - } - - { - OrphanProofPool pool{6}; - auto first = makeProof(4); - pool.addProof(first); - pool.addProof(makeProof(2)); - BOOST_CHECK_EQUAL(pool.getNStakes(), 6); - BOOST_CHECK_EQUAL(pool.getNProofs(), 2); - - // The oldest proof has to be dropped - pool.addProof(makeProof(1)); - BOOST_CHECK_EQUAL(pool.getNStakes(), 3); - BOOST_CHECK_EQUAL(pool.getNProofs(), 2); - BOOST_CHECK(!pool.getProof(first->getId())); - } - - { - OrphanProofPool pool{15}; - auto first = makeProof(1); - pool.addProof(first); - auto second = makeProof(2); - pool.addProof(second); - pool.addProof(makeProof(4)); - pool.addProof(makeProof(8)); - BOOST_CHECK_EQUAL(pool.getNStakes(), 15); - BOOST_CHECK_EQUAL(pool.getNProofs(), 4); - - // Multiple proofs are dropped if needed - pool.addProof(makeProof(2)); - BOOST_CHECK_EQUAL(pool.getNStakes(), 14); - BOOST_CHECK_EQUAL(pool.getNProofs(), 3); - BOOST_CHECK(!pool.getProof(first->getId())); - BOOST_CHECK(!pool.getProof(second->getId())); - } -} - -BOOST_AUTO_TEST_CASE(remove_proofs) { - OrphanProofPool pool{1337}; - std::array aProofIds; - - // Add 10 proofs - for (size_t i = 0; i < 10; i++) { - auto p = makeProof(i + 1); - aProofIds[i] = p->getId(); - BOOST_CHECK(pool.addProof(p)); - } - BOOST_CHECK_EQUAL(pool.getNProofs(), 10); - BOOST_CHECK_EQUAL(pool.getNStakes(), 55); - - // Remove a proof in the middle - BOOST_CHECK(pool.removeProof(aProofIds[5])); - BOOST_CHECK_EQUAL(pool.getNProofs(), 9); - BOOST_CHECK_EQUAL(pool.getNStakes(), 49); - - // Remove a proof at the front - BOOST_CHECK(pool.removeProof(aProofIds[0])); - BOOST_CHECK_EQUAL(pool.getNProofs(), 8); - BOOST_CHECK_EQUAL(pool.getNStakes(), 48); - - // Remove a proof at the back - BOOST_CHECK(pool.removeProof(aProofIds[9])); - BOOST_CHECK_EQUAL(pool.getNProofs(), 7); - BOOST_CHECK_EQUAL(pool.getNStakes(), 38); - - // Fail to remove a proof that is longer in the pool - BOOST_CHECK(!pool.removeProof(aProofIds[5])); - BOOST_CHECK_EQUAL(pool.getNProofs(), 7); - BOOST_CHECK_EQUAL(pool.getNStakes(), 38); - - // Fail to remove a proof that was never in the pool - auto p = makeProof(11); - BOOST_CHECK(!pool.getProof(p->getId())); - BOOST_CHECK(!pool.removeProof(p->getId())); - BOOST_CHECK_EQUAL(pool.getNProofs(), 7); - BOOST_CHECK_EQUAL(pool.getNStakes(), 38); -} - -BOOST_AUTO_TEST_CASE(add_proof_larger_than_pool) { - OrphanProofPool pool{AVALANCHE_MAX_PROOF_STAKES - 1}; - - // Add a couple of small proofs - BOOST_CHECK(pool.addProof(makeProof(1))); - BOOST_CHECK_EQUAL(pool.getNStakes(), 1); - BOOST_CHECK_EQUAL(pool.getNProofs(), 1); - - BOOST_CHECK(pool.addProof(makeProof(2))); - BOOST_CHECK_EQUAL(pool.getNStakes(), 3); - BOOST_CHECK_EQUAL(pool.getNProofs(), 2); - - // Adding a proof larger than the pool causes the pool to be emptied - BOOST_CHECK(pool.addProof(makeProof(AVALANCHE_MAX_PROOF_STAKES))); - BOOST_CHECK_EQUAL(pool.getNStakes(), 0); - BOOST_CHECK_EQUAL(pool.getNProofs(), 0); -} - -BOOST_AUTO_TEST_SUITE_END() diff --git a/src/avalanche/test/peermanager_tests.cpp b/src/avalanche/test/peermanager_tests.cpp --- a/src/avalanche/test/peermanager_tests.cpp +++ b/src/avalanche/test/peermanager_tests.cpp @@ -942,4 +942,35 @@ } } +BOOST_AUTO_TEST_CASE(conflicting_orphans) { + avalanche::PeerManager pm; + + const CKey key = CKey::MakeCompressedKey(); + + const Amount amount(10 * COIN); + const uint32_t height = 100; + const bool is_coinbase = false; + + const COutPoint conflictingOutpoint(TxId(GetRandHash()), 0); + + auto buildProofWithSequence = [&](uint64_t sequence) { + ProofBuilder pb(sequence, 0, key); + BOOST_CHECK( + pb.addUTXO(conflictingOutpoint, amount, height, is_coinbase, key)); + + return pb.build(); + }; + + auto orphan10 = buildProofWithSequence(10); + auto orphan20 = buildProofWithSequence(20); + + BOOST_CHECK(!pm.registerProof(orphan10)); + BOOST_CHECK(pm.isOrphan(orphan10->getId())); + + BOOST_CHECK(!pm.registerProof(orphan20)); + BOOST_CHECK(!pm.isOrphan(orphan20->getId())); + BOOST_CHECK(!pm.exists(orphan20->getId())); + BOOST_CHECK(pm.isOrphan(orphan10->getId())); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/test/functional/abc_rpc_avalancheproof.py b/test/functional/abc_rpc_avalancheproof.py --- a/test/functional/abc_rpc_avalancheproof.py +++ b/test/functional/abc_rpc_avalancheproof.py @@ -387,6 +387,9 @@ assert_raises_rpc_error(-8, "The proof has conflicting utxo with an existing proof", node.sendavalancheproof, conflicting_utxo) + # Clear the proof pool + self.restart_node(0) + # Good proof assert node.verifyavalancheproof(proof) diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh --- a/test/lint/lint-circular-dependencies.sh +++ b/test/lint/lint-circular-dependencies.sh @@ -32,7 +32,6 @@ "checkpoints -> validation -> checkpoints" "pow/aserti32d -> validation -> pow/aserti32d" "pow/aserti32d -> validation -> pow/pow -> pow/aserti32d" - "avalanche/orphanproofpool -> avalanche/peermanager -> avalanche/orphanproofpool" "avalanche/peermanager -> avalanche/proofpool -> avalanche/peermanager" )