diff --git a/src/avalanche/peermanager.h b/src/avalanche/peermanager.h
--- a/src/avalanche/peermanager.h
+++ b/src/avalanche/peermanager.h
@@ -290,7 +290,7 @@
bool addNodeToPeer(const PeerSet::iterator &it);
bool removeNodeFromPeer(const PeerSet::iterator &it, uint32_t count = 1);
- bool addProofToPool(const ProofRef &proof);
+ bool addProofToPool(const ProofRef &proof, const bool allowPeerOverride);
friend struct ::avalanche::TestPeerManager;
};
diff --git a/src/avalanche/peermanager.cpp b/src/avalanche/peermanager.cpp
--- a/src/avalanche/peermanager.cpp
+++ b/src/avalanche/peermanager.cpp
@@ -147,43 +147,89 @@
state.GetResult() == ProofValidationResult::HEIGHT_MISMATCH;
}
-bool PeerManager::addProofToPool(const ProofRef &proof) {
+bool PeerManager::addProofToPool(const ProofRef &proof,
+ const bool allowPeerOverride) {
std::set<ProofRef> shiftedProofs;
if (!pool.addProof(proof, shiftedProofs)) {
return false;
}
- if (!gArgs.GetBoolArg("-enableavalancheproofreplacement",
- AVALANCHE_DEFAULT_PROOF_REPLACEMENT_ENABLED)) {
- // The proof is attached with no conflict
- if (shiftedProofs.empty()) {
- return true;
- }
-
- // For now, if there is a conflict, just cleanup the mess.
- pool.removeProof(proof);
+ // No conflict, bail early
+ if (shiftedProofs.empty()) {
+ return true;
+ }
- // Restore the previous state. Since we removed the conflicting proof,
- // this can never fail.
+ if (allowPeerOverride) {
+ // If the proof is successfully added to the pool, deal with conflicts.
+ // The proof might have replaced others, so we need to remove their
+ // peers as needed.
for (const ProofRef &p : shiftedProofs) {
- std::set<ProofRef> inconsistent;
- const bool attached = pool.addProof(p, inconsistent);
- assert(attached);
- assert(inconsistent.empty());
+ removePeer(p);
}
- return false;
+ return true;
}
- // If the proof is successfully added to the pool, deal with conflicts.
- // The proof might have replaced others, so we need to remove their peers
- // as needed.
+ // We don't want an this proof to override any valid peer, even if the proof
+ // has better properties.
+ bool hasShiftedPeers = false;
+ std::unordered_set<ProofRef> proofsToRemove;
+ std::unordered_set<ProofRef> proofsToAdd;
for (const ProofRef &p : shiftedProofs) {
- removePeer(p);
- orphanProofs.addProof(p);
+ // If there is no peer associated with this proof, no need to solve the
+ // conflict.
+ if (!isValid(p->getId())) {
+ continue;
+ }
+
+ // At least one peer was shifted
+ hasShiftedPeers = true;
+
+ for (const SignedStake &ss : p->getStakes()) {
+ // If our peer is no longer in the pool and the set is full, we need
+ // to remove the last proof from the set (the least valuable one),
+ // and try to attach our peer again. We repeat for each utxo until
+ // completion as there might be several utxos conflicting.
+ pool.forUtxo(ss.getStake().getUTXO(), [&](const auto &proofs) {
+ if (proofs.size() < pool.getMaxProofsPerUtxo()) {
+ // No conflict to solve
+ return true;
+ }
+
+ if (proofs.find(p) != proofs.end()) {
+ // Peer not removed
+ return true;
+ }
+
+ // This proof was removed and had a peer attached, we need to
+ // restore it
+ proofsToAdd.emplace(p);
+
+ proofsToRemove.emplace(*proofs.rbegin());
+ return false;
+ });
+ }
}
- return true;
+ // If the proof we are trying to add is in the list of the proofs we need to
+ // remove for restoring the peers, then we can be sure it's enough to remove
+ // only this one. Otherwise, we need to detach them all.
+ if (proofsToRemove.count(proof) > 0) {
+ proofsToRemove = {proof};
+ }
+
+ for (const auto &p : proofsToRemove) {
+ const bool removed = pool.removeProof(p);
+ assert(removed);
+ }
+
+ for (const auto &p : proofsToAdd) {
+ std::set<ProofRef> dummy;
+ const bool added = pool.addProof(p, dummy);
+ assert(added);
+ }
+
+ return !hasShiftedPeers;
}
bool PeerManager::registerProof(const ProofRef &proof) {
@@ -209,7 +255,10 @@
return false;
}
- if (!addProofToPool(proof)) {
+ if (!addProofToPool(
+ proof,
+ gArgs.GetBoolArg("-enableavalancheproofreplacement",
+ AVALANCHE_DEFAULT_PROOF_REPLACEMENT_ENABLED))) {
// Rejected due to conflicting proofs
orphanProofs.addProof(proof);
return false;
diff --git a/src/avalanche/proofpool.h b/src/avalanche/proofpool.h
--- a/src/avalanche/proofpool.h
+++ b/src/avalanche/proofpool.h
@@ -38,6 +38,8 @@
bool addProof(const ProofRef &proof, std::set<ProofRef> &shifted);
bool removeProof(const ProofRef &proof);
+ size_t getMaxProofsPerUtxo() { return maxProofsPerUtxo; }
+
template <typename Callable>
bool forUtxo(const COutPoint &outpoint, Callable fun) const {
auto it = utxos.find(outpoint);
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
@@ -40,6 +40,23 @@
auto it = pview.find(proof->getId());
return it == pview.end() ? NO_PEER : it->peerid;
}
+
+ static bool addProofToPool(PeerManager &pm, const ProofRef &proof,
+ const bool allowPeerOverride) {
+ return pm.addProofToPool(proof, allowPeerOverride);
+ }
+
+ static std::vector<ProofRef>
+ getConflictingProofs(const PeerManager &pm, const COutPoint &utxo) {
+ std::vector<ProofRef> conflictingProofs;
+
+ pm.pool.forUtxo(utxo, [&](const auto &proofs) {
+ conflictingProofs.assign(proofs.begin(), proofs.end());
+ return true;
+ });
+
+ return conflictingProofs;
+ }
};
} // namespace
} // namespace avalanche
@@ -952,4 +969,120 @@
gArgs.ClearForcedArg("-enableavalancheproofreplacement");
}
+BOOST_AUTO_TEST_CASE(add_proof_no_override) {
+ avalanche::PeerManager pm{2};
+
+ 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()));
+
+ auto addCoin = [&](const Amount &amount) {
+ LOCK(cs_main);
+ const COutPoint outpoint(TxId(GetRandHash()), 0);
+ CCoinsViewCache &coins = ::ChainstateActive().CoinsTip();
+ coins.AddCoin(outpoint,
+ Coin(CTxOut(amount, script), height, is_coinbase), false);
+ return outpoint;
+ };
+
+ // This will be the conflicting utxos for all the following proofs
+ auto conflictingOutpoint1 = addCoin(amount);
+ auto conflictingOutpoint2 = addCoin(amount);
+ auto conflictingOutpoint3 = addCoin(amount);
+
+ auto buildProofWithSequence = [&](uint64_t sequence,
+ const std::vector<COutPoint> &outpoints) {
+ ProofBuilder pb(sequence, GetRandInt(std::numeric_limits<int>::max()),
+ key);
+ for (const COutPoint &o : outpoints) {
+ BOOST_CHECK(pb.addUTXO(o, amount, height, is_coinbase, key));
+ }
+
+ return pb.build();
+ };
+
+ // Build and register a base proof which should never be overridden through
+ // the test
+ auto proof_base = buildProofWithSequence(
+ 10, {conflictingOutpoint1, conflictingOutpoint2, conflictingOutpoint3});
+ BOOST_CHECK(pm.registerProof(proof_base));
+ BOOST_CHECK(pm.isValid(proof_base->getId()));
+
+ auto addProofToPoolNoPeerOverride = [&](const ProofRef &proof) {
+ return TestPeerManager::addProofToPool(pm, proof,
+ false /* allowPeerOverride */);
+ };
+
+ auto checkUtxo =
+ [&](const COutPoint &utxo,
+ const std::vector<ProofRef> &expectedConflictingProofs) {
+ auto conflictingProofs =
+ TestPeerManager::getConflictingProofs(pm, utxo);
+ BOOST_CHECK_EQUAL_COLLECTIONS(conflictingProofs.begin(),
+ conflictingProofs.end(),
+ expectedConflictingProofs.begin(),
+ expectedConflictingProofs.end());
+ };
+
+ // Adding the proof again will just work
+ for (size_t i = 0; i < 10; i++) {
+ BOOST_CHECK(addProofToPoolNoPeerOverride(proof_base));
+ BOOST_CHECK(pm.isValid(proof_base->getId()));
+ checkUtxo(conflictingOutpoint1, {proof_base});
+ checkUtxo(conflictingOutpoint2, {proof_base});
+ checkUtxo(conflictingOutpoint3, {proof_base});
+ }
+
+ auto conflictingSeq8 =
+ buildProofWithSequence(8, {conflictingOutpoint1, conflictingOutpoint2});
+ auto conflictingSeq9 =
+ buildProofWithSequence(9, {conflictingOutpoint1, conflictingOutpoint3});
+ auto conflictingSeq11 = buildProofWithSequence(
+ 11, {conflictingOutpoint1, conflictingOutpoint2});
+ auto conflictingSeq12 = buildProofWithSequence(
+ 12, {conflictingOutpoint1, conflictingOutpoint3});
+
+ // This insert did not impact any peer
+ BOOST_CHECK(addProofToPoolNoPeerOverride(conflictingSeq8));
+ BOOST_CHECK(pm.isValid(proof_base->getId()));
+ checkUtxo(conflictingOutpoint1, {proof_base, conflictingSeq8});
+ checkUtxo(conflictingOutpoint2, {proof_base, conflictingSeq8});
+ checkUtxo(conflictingOutpoint3, {proof_base});
+
+ BOOST_CHECK(addProofToPoolNoPeerOverride(conflictingSeq9));
+ BOOST_CHECK(pm.isValid(proof_base->getId()));
+ checkUtxo(conflictingOutpoint1, {proof_base, conflictingSeq9});
+ checkUtxo(conflictingOutpoint2, {proof_base});
+ checkUtxo(conflictingOutpoint3, {proof_base, conflictingSeq9});
+
+ // This insert shifted a peer
+ BOOST_CHECK(!addProofToPoolNoPeerOverride(conflictingSeq11));
+ BOOST_CHECK(pm.isValid(proof_base->getId()));
+ checkUtxo(conflictingOutpoint1, {conflictingSeq11, proof_base});
+ checkUtxo(conflictingOutpoint2, {conflictingSeq11, proof_base});
+ checkUtxo(conflictingOutpoint3, {proof_base});
+
+ BOOST_CHECK(!addProofToPoolNoPeerOverride(conflictingSeq12));
+ BOOST_CHECK(pm.isValid(proof_base->getId()));
+ checkUtxo(conflictingOutpoint1, {conflictingSeq12, proof_base});
+ checkUtxo(conflictingOutpoint2, {proof_base});
+ checkUtxo(conflictingOutpoint3, {conflictingSeq12, proof_base});
+
+ BOOST_CHECK(!addProofToPoolNoPeerOverride(conflictingSeq11));
+ BOOST_CHECK(pm.isValid(proof_base->getId()));
+ checkUtxo(conflictingOutpoint1, {conflictingSeq12, proof_base});
+ checkUtxo(conflictingOutpoint2, {proof_base});
+ checkUtxo(conflictingOutpoint3, {conflictingSeq12, proof_base});
+
+ BOOST_CHECK(!addProofToPoolNoPeerOverride(conflictingSeq9));
+ BOOST_CHECK(pm.isValid(proof_base->getId()));
+ checkUtxo(conflictingOutpoint1, {conflictingSeq12, proof_base});
+ checkUtxo(conflictingOutpoint2, {proof_base});
+ checkUtxo(conflictingOutpoint3, {conflictingSeq12, proof_base});
+}
+
BOOST_AUTO_TEST_SUITE_END()