diff --git a/src/avalanche/peermanager.h b/src/avalanche/peermanager.h --- a/src/avalanche/peermanager.h +++ b/src/avalanche/peermanager.h @@ -269,9 +269,8 @@ uint64_t getFragmentation() const { return fragmentation; } ProofRef getProof(const ProofId &proofid) const; - - bool isOrphan(const ProofId &id) const; - ProofRef getOrphan(const ProofId &id) const; + bool isValid(const ProofId &proofid) const; + bool isOrphan(const ProofId &proofid) const; private: PeerSet::iterator fetchOrCreatePeer(const ProofRef &proof); diff --git a/src/avalanche/peermanager.cpp b/src/avalanche/peermanager.cpp --- a/src/avalanche/peermanager.cpp +++ b/src/avalanche/peermanager.cpp @@ -143,7 +143,7 @@ } bool PeerManager::registerProof(const ProofRef &proof) { - return !getProof(proof->getId()) && getPeerId(proof) != NO_PEER; + return !exists(proof->getId()) && getPeerId(proof) != NO_PEER; } NodeId PeerManager::selectNode() { @@ -217,9 +217,22 @@ return true; }); + if (!proof) { + proof = orphanProofs.getProof(proofid); + } + return proof; } +bool PeerManager::isValid(const ProofId &proofid) const { + auto &pview = peers.get(); + return pview.find(proofid) != pview.end(); +} + +bool PeerManager::isOrphan(const ProofId &proofid) const { + return orphanProofs.getProof(proofid) != nullptr; +} + PeerManager::PeerSet::iterator PeerManager::fetchOrCreatePeer(const ProofRef &proof) { assert(proof); @@ -537,17 +550,9 @@ return NO_PEER; } -bool PeerManager::isOrphan(const ProofId &id) const { - return orphanProofs.getProof(id) != nullptr; -} - -ProofRef PeerManager::getOrphan(const ProofId &id) const { - return orphanProofs.getProof(id); -} - void PeerManager::addUnbroadcastProof(const ProofId &proofid) { - // The proof should be known - if (getProof(proofid)) { + // The proof should be valid + if (isValid(proofid)) { m_unbroadcast_proofids.insert(proofid); } } 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 @@ -444,7 +444,7 @@ // Remove the peer, the nodes should be pending again BOOST_CHECK(pm.removePeer(peerid)); - BOOST_CHECK(!pm.getProof(proof->getId())); + BOOST_CHECK(!pm.exists(proof->getId())); for (int i = 0; i < 10; i++) { BOOST_CHECK(TestPeerManager::isNodePending(pm, i)); BOOST_CHECK(!TestPeerManager::nodeBelongToPeer(pm, i, peerid)); @@ -491,7 +491,7 @@ pm.updatedBlockTip(); BOOST_CHECK(pm.isOrphan(proofid)); - BOOST_CHECK(!pm.getProof(proofid)); + BOOST_CHECK(!pm.isValid(proofid)); for (int i = 0; i < 10; i++) { BOOST_CHECK(TestPeerManager::isNodePending(pm, i)); BOOST_CHECK(!TestPeerManager::nodeBelongToPeer(pm, i, peerid)); @@ -507,7 +507,7 @@ pm.updatedBlockTip(); BOOST_CHECK(!pm.isOrphan(proofid)); - BOOST_CHECK(pm.getProof(proofid)); + BOOST_CHECK(pm.isValid(proofid)); // The peerid has certainly been updated peerid = pm.getPeerId(proof); BOOST_CHECK_NE(peerid, NO_PEER); @@ -636,26 +636,28 @@ BOOST_CHECK(pm.getPeerId(proof2) == NO_PEER); BOOST_CHECK(pm.getPeerId(proof3) == NO_PEER); - // Good - BOOST_CHECK(!pm.isOrphan(proof1->getId())); - // MISSING_UTXO - BOOST_CHECK(pm.isOrphan(proof2->getId())); - // HEIGHT_MISMATCH - BOOST_CHECK(pm.isOrphan(proof3->getId())); + auto checkOrphan = [&](const ProofRef &proof, bool expectedOrphan) { + const ProofId &proofid = proof->getId(); + BOOST_CHECK(pm.exists(proofid)); + + BOOST_CHECK_EQUAL(pm.isOrphan(proofid), expectedOrphan); + BOOST_CHECK_EQUAL(pm.isValid(proofid), !expectedOrphan); - const auto isGoodPeer = [&pm](const ProofRef &p) { bool ret = false; pm.forEachPeer([&](const Peer &peer) { - if (p->getId() == peer.proof->getId()) { + if (proof->getId() == peer.proof->getId()) { ret = true; } }); - return ret; + BOOST_CHECK_EQUAL(ret, !expectedOrphan); }; - BOOST_CHECK(isGoodPeer(proof1)); - BOOST_CHECK(!isGoodPeer(proof2)); - BOOST_CHECK(!isGoodPeer(proof3)); + // Good + checkOrphan(proof1, false); + // MISSING_UTXO + checkOrphan(proof2, true); + // HEIGHT_MISMATCH + checkOrphan(proof3, true); // Add outpoint2, proof2 is no longer considered orphan { @@ -665,14 +667,11 @@ } pm.updatedBlockTip(); - BOOST_CHECK(!pm.isOrphan(proof2->getId())); - BOOST_CHECK(isGoodPeer(proof2)); + checkOrphan(proof2, false); // The status of proof1 and proof3 are unchanged - BOOST_CHECK(!pm.isOrphan(proof1->getId())); - BOOST_CHECK(isGoodPeer(proof1)); - BOOST_CHECK(pm.isOrphan(proof3->getId())); - BOOST_CHECK(!isGoodPeer(proof3)); + checkOrphan(proof1, false); + checkOrphan(proof3, true); // Spend outpoint1, proof1 becomes orphan { @@ -682,14 +681,11 @@ } pm.updatedBlockTip(); - BOOST_CHECK(pm.isOrphan(proof1->getId())); - BOOST_CHECK(!isGoodPeer(proof1)); + checkOrphan(proof1, true); // The status of proof2 and proof3 are unchanged - BOOST_CHECK(!pm.isOrphan(proof2->getId())); - BOOST_CHECK(isGoodPeer(proof2)); - BOOST_CHECK(pm.isOrphan(proof3->getId())); - BOOST_CHECK(!isGoodPeer(proof3)); + checkOrphan(proof2, false); + checkOrphan(proof3, true); // A reorg could make a previous HEIGHT_MISMATCH become valid { @@ -701,14 +697,11 @@ } pm.updatedBlockTip(); - BOOST_CHECK(!pm.isOrphan(proof3->getId())); - BOOST_CHECK(isGoodPeer(proof3)); + checkOrphan(proof3, false); // The status of proof 1 and proof2 are unchanged - BOOST_CHECK(pm.isOrphan(proof1->getId())); - BOOST_CHECK(!isGoodPeer(proof1)); - BOOST_CHECK(!pm.isOrphan(proof2->getId())); - BOOST_CHECK(isGoodPeer(proof2)); + checkOrphan(proof1, true); + checkOrphan(proof2, false); } BOOST_AUTO_TEST_CASE(dangling_node) { diff --git a/src/net_processing.cpp b/src/net_processing.cpp --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1106,9 +1106,9 @@ auto unbroadcasted_proofids = pm.getUnbroadcastProofs(); for (const auto &proofid : unbroadcasted_proofids) { - // Sanity check: all unbroadcast proofs should exist in the + // Sanity check: all unbroadcast proofs should be valid in the // peermanager - if (pm.exists(proofid)) { + if (pm.isValid(proofid)) { RelayProof(proofid, m_connman); } else { pm.removeUnbroadcastProof(proofid); @@ -1814,10 +1814,8 @@ static bool AlreadyHaveProof(const avalanche::ProofId &proofid) { assert(g_avalanche); - const bool hasProof = - g_avalanche->withPeerManager([&proofid](avalanche::PeerManager &pm) { - return pm.getProof(proofid) || pm.getOrphan(proofid); - }); + const bool hasProof = g_avalanche->withPeerManager( + [&proofid](avalanche::PeerManager &pm) { return pm.exists(proofid); }); LOCK(cs_rejectedProofs); return hasProof || rejectedProofs->contains(proofid); @@ -4463,7 +4461,7 @@ // invalid. In the latter case we should increase the ban score. // TODO improve the ban reason by printing the validation state if (!g_avalanche->withPeerManager([&](avalanche::PeerManager &pm) { - return pm.getOrphan(proofid); + return pm.isOrphan(proofid); })) { WITH_LOCK(cs_rejectedProofs, rejectedProofs->insert(proofid)); Misbehaving(nodeid, 100, "invalid-avaproof"); diff --git a/src/rpc/avalanche.cpp b/src/rpc/avalanche.cpp --- a/src/rpc/avalanche.cpp +++ b/src/rpc/avalanche.cpp @@ -615,12 +615,8 @@ bool isOrphan = false; auto proof = g_avalanche->withPeerManager([&](avalanche::PeerManager &pm) { - auto proof = pm.getProof(proofid); - if (!proof) { - proof = pm.getOrphan(proofid); - isOrphan = true; - } - return proof; + isOrphan = pm.isOrphan(proofid); + return pm.getProof(proofid); }); if (!proof) {