Page MenuHomePhabricator
Files F14864801

D11630.id.diff
No OneTemporary
Actions

D11630.id.diff
View Options

diff --git a/src/avalanche/processor.h b/src/avalanche/processor.h
--- a/src/avalanche/processor.h
+++ b/src/avalanche/processor.h
@@ -195,7 +195,7 @@
}
bool addBlockToReconcile(const CBlockIndex *pindex);
- void addProofToReconcile(const ProofRef &proof);
+ bool addProofToReconcile(const ProofRef &proof);
bool isAccepted(const CBlockIndex *pindex) const;
bool isAccepted(const ProofRef &proof) const;
int getConfidence(const CBlockIndex *pindex) const;
@@ -260,6 +260,8 @@
bool isWorthPolling(const CBlockIndex *pindex) const
EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+ bool isWorthPolling(const ProofRef &proof) const
+ EXCLUSIVE_LOCKS_REQUIRED(cs_peerManager);
friend struct ::avalanche::AvalancheTest;
};
diff --git a/src/avalanche/processor.cpp b/src/avalanche/processor.cpp
--- a/src/avalanche/processor.cpp
+++ b/src/avalanche/processor.cpp
@@ -317,13 +317,12 @@
}
bool Processor::addBlockToReconcile(const CBlockIndex *pindex) {
- bool isAccepted;
-
if (!pindex) {
// isWorthPolling expects this to be non-null, so bail early.
return false;
}
+ bool isAccepted;
{
LOCK(cs_main);
if (!isWorthPolling(pindex)) {
@@ -339,15 +338,25 @@
.second;
}
-void Processor::addProofToReconcile(const ProofRef &proof) {
- // TODO We don't want to accept an infinite number of conflicting proofs.
- // They should be some rules to make them expensive and/or limited by
- // design.
- const bool isAccepted = WITH_LOCK(
- cs_peerManager, return peerManager->isBoundToPeer(proof->getId()));
+bool Processor::addProofToReconcile(const ProofRef &proof) {
+ if (!proof) {
+ // isWorthPolling expects this to be non-null, so bail early.
+ return false;
+ }
+
+ bool isAccepted;
+ {
+ LOCK(cs_peerManager);
+ if (!isWorthPolling(proof)) {
+ return false;
+ }
+
+ isAccepted = peerManager->isBoundToPeer(proof->getId());
+ }
- proofVoteRecords.getWriteView()->insert(
- std::make_pair(proof, VoteRecord(isAccepted)));
+ return proofVoteRecords.getWriteView()
+ ->insert(std::make_pair(proof, VoteRecord(isAccepted)))
+ .second;
}
bool Processor::isAccepted(const CBlockIndex *pindex) const {
@@ -514,10 +523,17 @@
if (invs[i].IsMsgProof()) {
const ProofId proofid(votes[i].GetHash());
- const ProofRef proof = WITH_LOCK(
- cs_peerManager, return peerManager->getProof(proofid));
- if (!proof) {
- continue;
+ ProofRef proof;
+ {
+ LOCK(cs_peerManager);
+ proof = peerManager->getProof(proofid);
+ if (!proof) {
+ continue;
+ }
+
+ if (!isWorthPolling(proof)) {
+ continue;
+ }
}
responseProof.insert(std::make_pair(proof, votes[i]));
@@ -828,6 +844,20 @@
std::vector<CInv> Processor::getInvsForNextPoll(bool forPoll) {
std::vector<CInv> invs;
+ // Use NO_THREAD_SAFETY_ANALYSIS to avoid false positive due to
+ // isWorthPolling requiring a different lock depending of the prototype.
+ auto removeItemsNotWorthPolling =
+ [&](auto &itemVoteRecords) NO_THREAD_SAFETY_ANALYSIS {
+ auto w = itemVoteRecords.getWriteView();
+ for (auto it = w->begin(); it != w->end();) {
+ if (!isWorthPolling(it->first)) {
+ it = w->erase(it);
+ } else {
+ ++it;
+ }
+ }
+ };
+
auto extractVoteRecordsToInvs = [&](const auto &itemVoteRecordRange,
auto buildInvFromVoteItem) {
for (const auto &[item, voteRecord] : itemVoteRecordRange) {
@@ -850,6 +880,9 @@
return invs.size() >= AVALANCHE_MAX_ELEMENT_POLL;
};
+ // First remove all proofs that are not worth polling.
+ WITH_LOCK(cs_peerManager, removeItemsNotWorthPolling(proofVoteRecords));
+
if (extractVoteRecordsToInvs(proofVoteRecords.getReadView(),
[](const ProofRef &proof) {
return CInv(MSG_AVA_PROOF, proof->getId());
@@ -859,18 +892,7 @@
}
// First remove all blocks that are not worth polling.
- {
- LOCK(cs_main);
- auto w = blockVoteRecords.getWriteView();
- for (auto it = w->begin(); it != w->end();) {
- const CBlockIndex *pindex = it->first;
- if (!isWorthPolling(pindex)) {
- w->erase(it++);
- } else {
- ++it;
- }
- }
- }
+ WITH_LOCK(cs_main, removeItemsNotWorthPolling(blockVoteRecords));
auto r = blockVoteRecords.getReadView();
extractVoteRecordsToInvs(reverse_iterate(r), [](const CBlockIndex *pindex) {
@@ -911,4 +933,14 @@
return true;
}
+bool Processor::isWorthPolling(const ProofRef &proof) const {
+ AssertLockHeld(cs_peerManager);
+
+ const ProofId &proofid = proof->getId();
+
+ // No point polling orphans or discarded proofs
+ return peerManager->isBoundToPeer(proofid) ||
+ peerManager->isInConflictingPool(proofid);
+}
+
} // namespace avalanche
diff --git a/src/avalanche/test/processor_tests.cpp b/src/avalanche/test/processor_tests.cpp
--- a/src/avalanche/test/processor_tests.cpp
+++ b/src/avalanche/test/processor_tests.cpp
@@ -27,6 +27,7 @@
#include <boost/test/unit_test.hpp>
#include <functional>
+#include <type_traits>
#include <vector>
using namespace avalanche;
@@ -259,6 +260,10 @@
return votes;
}
+
+ void invalidateItem(CBlockIndex *pindex) {
+ pindex->nStatus = pindex->nStatus.withFailed();
+ }
};
struct ProofProvider {
@@ -295,8 +300,7 @@
}
bool addToReconcile(const ProofRef &proof) {
- fixture->m_processor->addProofToReconcile(proof);
- return true;
+ return fixture->m_processor->addProofToReconcile(proof);
}
std::vector<Vote> buildVotesForItems(uint32_t error,
@@ -314,6 +318,13 @@
return votes;
}
+
+ void invalidateItem(const ProofRef &proof) {
+ fixture->m_processor->withPeerManager([&](avalanche::PeerManager &pm) {
+ pm.rejectProof(proof->getId(),
+ avalanche::PeerManager::RejectionMode::INVALIDATE);
+ });
+ }
};
} // namespace
@@ -341,39 +352,33 @@
}
}
-BOOST_AUTO_TEST_CASE(block_reconcile_twice) {
- CBlock block = CreateAndProcessBlock({}, CScript());
- const BlockHash blockHash = block.GetHash();
- CBlockIndex *pindex;
- {
- LOCK(cs_main);
- pindex = g_chainman.m_blockman.LookupBlockIndex(blockHash);
- }
+BOOST_AUTO_TEST_CASE_TEMPLATE(item_reconcile_twice, P, VoteItemProviders) {
+ P provider(this);
- // Adding the block twice does nothing.
- BOOST_CHECK(m_processor->addBlockToReconcile(pindex));
- BOOST_CHECK(!m_processor->addBlockToReconcile(pindex));
- BOOST_CHECK(m_processor->isAccepted(pindex));
+ auto item = provider.buildVoteItem();
+
+ // Adding the item twice does nothing.
+ BOOST_CHECK(provider.addToReconcile(item));
+ BOOST_CHECK(!provider.addToReconcile(item));
+ BOOST_CHECK(m_processor->isAccepted(item));
}
-BOOST_AUTO_TEST_CASE(block_null) {
+BOOST_AUTO_TEST_CASE_TEMPLATE(item_null, P, VoteItemProviders) {
+ P provider(this);
+
// Check that null case is handled on the public interface
BOOST_CHECK(!m_processor->isAccepted(nullptr));
BOOST_CHECK_EQUAL(m_processor->getConfidence(nullptr), -1);
- BOOST_CHECK(!m_processor->addBlockToReconcile(nullptr));
+ auto item = decltype(provider.buildVoteItem())();
+ BOOST_CHECK(item == nullptr);
+ BOOST_CHECK(!provider.addToReconcile(item));
- // Check that adding blocks to vote on doesn't change the outcome. A
+ // Check that adding item to vote on doesn't change the outcome. A
// comparator is used under the hood, and this is skipped if there are no
// vote records.
- CBlock block = CreateAndProcessBlock({}, CScript());
- const BlockHash blockHash = block.GetHash();
- CBlockIndex *pindex;
- {
- LOCK(cs_main);
- pindex = g_chainman.m_blockman.LookupBlockIndex(blockHash);
- }
- BOOST_CHECK(m_processor->addBlockToReconcile(pindex));
+ item = provider.buildVoteItem();
+ BOOST_CHECK(provider.addToReconcile(item));
BOOST_CHECK(!m_processor->isAccepted(nullptr));
BOOST_CHECK_EQUAL(m_processor->getConfidence(nullptr), -1);
@@ -745,40 +750,44 @@
BOOST_CHECK_EQUAL(getSuitableNodeToQuery(), avanodeid);
}
-BOOST_AUTO_TEST_CASE(dont_poll_invalid_block) {
- std::vector<BlockUpdate> updates;
+BOOST_AUTO_TEST_CASE_TEMPLATE(dont_poll_invalid_item, P, VoteItemProviders) {
+ P provider(this);
+ auto &updates = provider.updates;
+ const uint32_t invType = provider.invType;
- CBlock blockA = CreateAndProcessBlock({}, CScript());
- CBlock blockB = CreateAndProcessBlock({}, CScript());
- const BlockHash blockHashA = blockA.GetHash();
- const BlockHash blockHashB = blockB.GetHash();
- const CBlockIndex *pindexA;
- CBlockIndex *pindexB;
- {
- LOCK(cs_main);
- pindexA = g_chainman.m_blockman.LookupBlockIndex(blockHashA);
- pindexB = g_chainman.m_blockman.LookupBlockIndex(blockHashB);
- }
+ auto itemA = provider.buildVoteItem();
+ auto itemB = provider.buildVoteItem();
auto avanodes = ConnectNodes();
- // Register the blocks and check they are added to the list of elements to
+ // Build votes to get proper ordering
+ std::vector<Vote> votes = provider.buildVotesForItems(0, {itemA, itemB});
+
+ // Register the items and check they are added to the list of elements to
// poll.
- BOOST_CHECK(m_processor->addBlockToReconcile(pindexA));
- BOOST_CHECK(m_processor->addBlockToReconcile(pindexB));
+ BOOST_CHECK(provider.addToReconcile(itemA));
+ BOOST_CHECK(provider.addToReconcile(itemB));
auto invs = getInvsForNextPoll();
BOOST_CHECK_EQUAL(invs.size(), 2);
- BOOST_CHECK_EQUAL(invs[0].type, MSG_BLOCK);
- BOOST_CHECK(invs[0].hash == blockHashB);
- BOOST_CHECK_EQUAL(invs[1].type, MSG_BLOCK);
- BOOST_CHECK(invs[1].hash == blockHashA);
-
- // When a block is marked invalid, stop polling.
- pindexB->nStatus = pindexB->nStatus.withFailed();
- Response resp{getRound(), 0, {Vote(0, blockHashA)}};
+ for (size_t i = 0; i < invs.size(); i++) {
+ BOOST_CHECK_EQUAL(invs[i].type, invType);
+ BOOST_CHECK(invs[i].hash == votes[i].GetHash());
+ }
+
+ // When an item is marked invalid, stop polling.
+ provider.invalidateItem(itemB);
+
+ Response goodResp{getRound(), 0, {Vote(0, provider.getVoteItemId(itemA))}};
runEventLoop();
- BOOST_CHECK(registerVotes(avanodes[0]->GetId(), resp, updates));
+ BOOST_CHECK(provider.registerVotes(avanodes[0]->GetId(), goodResp));
BOOST_CHECK_EQUAL(updates.size(), 0);
+
+ // Votes including itemB are rejected
+ Response badResp{getRound(), 0, votes};
+ runEventLoop();
+ std::string error;
+ BOOST_CHECK(!provider.registerVotes(avanodes[1]->GetId(), badResp, error));
+ BOOST_CHECK_EQUAL(error, "invalid-ava-response-size");
}
BOOST_TEST_DECORATOR(*boost::unit_test::timeout(60))
@@ -1055,7 +1064,10 @@
auto addProofToReconcile = [&](uint32_t proofScore) {
auto proof = buildRandomProof(proofScore);
- m_processor->addProofToReconcile(proof);
+ m_processor->withPeerManager([&](avalanche::PeerManager &pm) {
+ BOOST_CHECK(pm.registerProof(proof));
+ });
+ BOOST_CHECK(m_processor->addProofToReconcile(proof));
return proof;
};
@@ -1100,31 +1112,83 @@
}
BOOST_AUTO_TEST_CASE(proof_record) {
+ gArgs.ForceSetArg("-enableavalancheproofreplacement", "1");
+ gArgs.ForceSetArg("-avalancheconflictingproofcooldown", "0");
+
BOOST_CHECK(!m_processor->isAccepted(nullptr));
BOOST_CHECK_EQUAL(m_processor->getConfidence(nullptr), -1);
- auto proofA = GetProof();
- auto proofB = GetProof();
+ const CKey key = CKey::MakeCompressedKey();
- BOOST_CHECK(!m_processor->isAccepted(proofA));
- BOOST_CHECK(!m_processor->isAccepted(proofB));
- BOOST_CHECK_EQUAL(m_processor->getConfidence(proofA), -1);
- BOOST_CHECK_EQUAL(m_processor->getConfidence(proofB), -1);
+ const COutPoint conflictingOutpoint{TxId(GetRandHash()), 0};
+ {
+ CScript script = GetScriptForDestination(PKHash(key.GetPubKey()));
- m_processor->addProofToReconcile(proofA);
- BOOST_CHECK(!m_processor->isAccepted(proofA));
- BOOST_CHECK(!m_processor->isAccepted(proofB));
- BOOST_CHECK_EQUAL(m_processor->getConfidence(proofA), 0);
- BOOST_CHECK_EQUAL(m_processor->getConfidence(proofB), -1);
+ LOCK(cs_main);
+ CCoinsViewCache &coins = ::ChainstateActive().CoinsTip();
+ coins.AddCoin(conflictingOutpoint,
+ Coin(CTxOut(10 * COIN, script), 10, false), false);
+ }
+ const COutPoint missingOutpoint{TxId(GetRandHash()), 0};
+
+ auto buildProof = [&](const COutPoint &outpoint, uint64_t sequence) {
+ ProofBuilder pb(sequence, 0, key);
+ BOOST_CHECK(pb.addUTXO(outpoint, 10 * COIN, 10, false, key));
+ return pb.build();
+ };
+
+ auto conflictingProof = buildProof(conflictingOutpoint, 1);
+ auto validProof = buildProof(conflictingOutpoint, 2);
+ auto orphanProof = buildProof(missingOutpoint, 3);
+
+ BOOST_CHECK(!m_processor->isAccepted(conflictingProof));
+ BOOST_CHECK(!m_processor->isAccepted(validProof));
+ BOOST_CHECK(!m_processor->isAccepted(orphanProof));
+ BOOST_CHECK_EQUAL(m_processor->getConfidence(conflictingProof), -1);
+ BOOST_CHECK_EQUAL(m_processor->getConfidence(validProof), -1);
+ BOOST_CHECK_EQUAL(m_processor->getConfidence(orphanProof), -1);
+
+ // Reconciling proofs that don't exist will fail
+ BOOST_CHECK(!m_processor->addProofToReconcile(conflictingProof));
+ BOOST_CHECK(!m_processor->addProofToReconcile(validProof));
+ BOOST_CHECK(!m_processor->addProofToReconcile(orphanProof));
m_processor->withPeerManager([&](avalanche::PeerManager &pm) {
- BOOST_CHECK(pm.registerProof(proofB));
+ BOOST_CHECK(pm.registerProof(conflictingProof));
+ BOOST_CHECK(pm.registerProof(validProof));
+ BOOST_CHECK(!pm.registerProof(orphanProof));
+
+ BOOST_CHECK(pm.isBoundToPeer(validProof->getId()));
+ BOOST_CHECK(pm.isInConflictingPool(conflictingProof->getId()));
+ BOOST_CHECK(pm.isOrphan(orphanProof->getId()));
});
- m_processor->addProofToReconcile(proofB);
- BOOST_CHECK(!m_processor->isAccepted(proofA));
- BOOST_CHECK(m_processor->isAccepted(proofB));
- BOOST_CHECK_EQUAL(m_processor->getConfidence(proofA), 0);
- BOOST_CHECK_EQUAL(m_processor->getConfidence(proofB), 0);
+
+ BOOST_CHECK(m_processor->addProofToReconcile(conflictingProof));
+ BOOST_CHECK(!m_processor->isAccepted(conflictingProof));
+ BOOST_CHECK(!m_processor->isAccepted(validProof));
+ BOOST_CHECK(!m_processor->isAccepted(orphanProof));
+ BOOST_CHECK_EQUAL(m_processor->getConfidence(conflictingProof), 0);
+ BOOST_CHECK_EQUAL(m_processor->getConfidence(validProof), -1);
+ BOOST_CHECK_EQUAL(m_processor->getConfidence(orphanProof), -1);
+
+ BOOST_CHECK(m_processor->addProofToReconcile(validProof));
+ BOOST_CHECK(!m_processor->isAccepted(conflictingProof));
+ BOOST_CHECK(m_processor->isAccepted(validProof));
+ BOOST_CHECK(!m_processor->isAccepted(orphanProof));
+ BOOST_CHECK_EQUAL(m_processor->getConfidence(conflictingProof), 0);
+ BOOST_CHECK_EQUAL(m_processor->getConfidence(validProof), 0);
+ BOOST_CHECK_EQUAL(m_processor->getConfidence(orphanProof), -1);
+
+ BOOST_CHECK(!m_processor->addProofToReconcile(orphanProof));
+ BOOST_CHECK(!m_processor->isAccepted(conflictingProof));
+ BOOST_CHECK(m_processor->isAccepted(validProof));
+ BOOST_CHECK(!m_processor->isAccepted(orphanProof));
+ BOOST_CHECK_EQUAL(m_processor->getConfidence(conflictingProof), 0);
+ BOOST_CHECK_EQUAL(m_processor->getConfidence(validProof), 0);
+ BOOST_CHECK_EQUAL(m_processor->getConfidence(orphanProof), -1);
+
+ gArgs.ClearForcedArg("-avalancheconflictingproofcooldown");
+ gArgs.ClearForcedArg("-enableavalancheproofreplacement");
}
BOOST_AUTO_TEST_CASE(quorum_detection) {

File Metadata

Mime Type
text/plain
Expires
Tue, May 20, 22:26 (18 h, 34 m)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
5865519
Default Alt Text
D11630.id.diff (16 KB)

Event Timeline