diff --git a/src/avalanche/peermanager.h b/src/avalanche/peermanager.h --- a/src/avalanche/peermanager.h +++ b/src/avalanche/peermanager.h @@ -125,6 +125,7 @@ PeerSet peers; ProofPool validProofPool; + ProofPool conflictingProofPool; ProofPool orphanProofPool; using NodeSet = boost::multi_index_container< @@ -256,6 +257,7 @@ ProofRef getProof(const ProofId &proofid) const; bool isBoundToPeer(const ProofId &proofid) const; bool isOrphan(const ProofId &proofid) const; + bool isInConflictingPool(const ProofId &proofid) const; private: bool createPeer(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 @@ -217,7 +217,24 @@ // possible to pull back proofs with utxos that conflicted with these // invalid peers. for (const auto &pid : invalidPeers) { + auto it = peers.find(pid); + assert(it != peers.end()); + + const ProofRef peerProof = it->proof; + removePeer(pid); + + // If the peer had conflicting proofs, attempt to pull them back + for (const SignedStake &ss : peerProof->getStakes()) { + const ProofRef conflictingProof = + conflictingProofPool.getProof(ss.getStake().getUTXO()); + if (!conflictingProof) { + continue; + } + + conflictingProofPool.removeProof(conflictingProof); + registerProof(conflictingProof); + } } orphanProofPool.rescan(*this); @@ -235,6 +252,10 @@ return true; }); + if (!proof) { + proof = conflictingProofPool.getProof(proofid); + } + if (!proof) { proof = orphanProofPool.getProof(proofid); } @@ -251,14 +272,18 @@ return orphanProofPool.getProof(proofid) != nullptr; } +bool PeerManager::isInConflictingPool(const ProofId &proofid) const { + return conflictingProofPool.getProof(proofid) != nullptr; +} + bool PeerManager::createPeer(const ProofRef &proof) { assert(proof); switch (validProofPool.addProofIfNoConflict(proof)) { case ProofPool::AddProofStatus::REJECTED: - // The proof has conflicts, orphan the proof so it can be pulled - // back if the conflicting ones are invalidated. - orphanProofPool.addProofIfNoConflict(proof); + // The proof has conflicts, move it to the conflicting proof pool so + // it can be pulled back if the conflicting ones are invalidated. + conflictingProofPool.addProofIfNoConflict(proof); return false; case ProofPool::AddProofStatus::DUPLICATED: // If the proof was already in the pool, don't duplicate the peer. @@ -269,6 +294,8 @@ // No default case, so the compiler can warn about missing cases } + conflictingProofPool.removeProof(proof); + // New peer means new peerid! const PeerId peerid = nextPeerId++; 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 @@ -840,8 +840,7 @@ } BOOST_CHECK(!pm.registerProof(conflictingProof)); - // The conflicting proof is orphaned - BOOST_CHECK(pm.isOrphan(conflictingProof->getId())); + BOOST_CHECK(pm.isInConflictingPool(conflictingProof->getId())); { LOCK(cs_main); @@ -854,7 +853,7 @@ BOOST_CHECK(pm.isOrphan(proofToInvalidate->getId())); - BOOST_CHECK(!pm.isOrphan(conflictingProof->getId())); + BOOST_CHECK(!pm.isInConflictingPool(conflictingProof->getId())); BOOST_CHECK(pm.isBoundToPeer(conflictingProof->getId())); } @@ -973,4 +972,54 @@ BOOST_CHECK(pm.isOrphan(orphan10->getId())); } +BOOST_AUTO_TEST_CASE(preferred_conflicting_proof) { + avalanche::PeerManager pm; + + const CKey key = CKey::MakeCompressedKey(); + + const Amount amount(10 * COIN); + const uint32_t height = 100; + const bool is_coinbase = false; + CScript script = GetScriptForDestination(PKHash(key.GetPubKey())); + + const COutPoint conflictingOutpoint(TxId(GetRandHash()), 0); + { + LOCK(cs_main); + CCoinsViewCache &coins = ::ChainstateActive().CoinsTip(); + coins.AddCoin(conflictingOutpoint, + Coin(CTxOut(amount, script), height, is_coinbase), false); + } + + auto buildProofWithSequence = [&](uint64_t sequence) { + ProofBuilder pb(sequence, 0, key); + BOOST_CHECK( + pb.addUTXO(conflictingOutpoint, amount, height, is_coinbase, key)); + + return pb.build(); + }; + + auto proofSeq10 = buildProofWithSequence(10); + auto proofSeq20 = buildProofWithSequence(20); + auto proofSeq30 = buildProofWithSequence(30); + + BOOST_CHECK(pm.registerProof(proofSeq30)); + BOOST_CHECK(pm.isBoundToPeer(proofSeq30->getId())); + BOOST_CHECK(!pm.isInConflictingPool(proofSeq30->getId())); + + // proofSeq10 is a worst candidate than proofSeq30, so it goes to the + // conflicting pool. + BOOST_CHECK(!pm.registerProof(proofSeq10)); + BOOST_CHECK(pm.isBoundToPeer(proofSeq30->getId())); + BOOST_CHECK(!pm.isBoundToPeer(proofSeq10->getId())); + BOOST_CHECK(pm.isInConflictingPool(proofSeq10->getId())); + + // proofSeq20 is a worst candidate than proofSeq30 but a better one than + // proogSeq10. However it is not permitted to override a proof in the + // conflicting pool so proofSeq20 is rejectd + BOOST_CHECK(!pm.registerProof(proofSeq20)); + BOOST_CHECK(pm.isBoundToPeer(proofSeq30->getId())); + BOOST_CHECK(pm.isInConflictingPool(proofSeq10->getId())); + BOOST_CHECK(!pm.exists(proofSeq20->getId())); +} + BOOST_AUTO_TEST_SUITE_END()