diff --git a/src/avalanche/peermanager.h b/src/avalanche/peermanager.h --- a/src/avalanche/peermanager.h +++ b/src/avalanche/peermanager.h @@ -36,6 +36,10 @@ class Delegation; +namespace { + struct TestPeerManager; +} + struct Slot { private: uint64_t start; @@ -93,6 +97,17 @@ struct next_request_time {}; +struct PendingNode { + ProofId proofid; + NodeId nodeid; + + PendingNode(ProofId proofid_, NodeId nodeid_) + : proofid(proofid_), nodeid(nodeid_){}; +}; + +struct by_proofid; +struct by_nodeid; + namespace bmi = boost::multi_index; class PeerManager { @@ -131,6 +146,20 @@ NodeSet nodes; + using PendingNodeSet = boost::multi_index_container< + PendingNode, + bmi::indexed_by< + // index by proofid + bmi::hashed_non_unique< + bmi::tag, + bmi::member, + SaltedProofIdHasher>, + // index by nodeid + bmi::hashed_unique< + bmi::tag, + bmi::member>>>; + PendingNodeSet pendingNodes; + static constexpr int SELECT_PEER_MAX_RETRY = 3; static constexpr int SELECT_NODE_MAX_RETRY = 3; @@ -249,6 +278,8 @@ bool addOrUpdateNode(const PeerSet::iterator &it, NodeId nodeid); bool addNodeToPeer(const PeerSet::iterator &it); bool removeNodeFromPeer(const PeerSet::iterator &it, uint32_t count = 1); + + 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 @@ -9,6 +9,7 @@ #include #include // For ChainstateActive() +#include #include namespace avalanche { @@ -17,6 +18,11 @@ auto &pview = peers.get(); auto it = pview.find(proofid); if (it == pview.end()) { + // If the node exists, it is actually updating its proof to an unknown + // one. In this case we need to remove it so it is not both active and + // pending at the same time. + removeNode(nodeid); + pendingNodes.emplace(proofid, nodeid); return false; } @@ -47,6 +53,9 @@ bool success = addNodeToPeer(it); assert(success); + // If the added node was in the pending set, remove it + pendingNodes.get().erase(nodeid); + return true; } @@ -76,6 +85,8 @@ const PeerId peerid = it->peerid; nodes.erase(it); + pendingNodes.get().erase(nodeid); + // Keep the track of the reference count. bool success = removeNodeFromPeer(peers.find(peerid)); assert(success); @@ -273,6 +284,22 @@ auto inserted = peers.emplace(peerid, proof); assert(inserted.second); + // If there are nodes waiting for this proof, add them + auto &pendingNodesView = pendingNodes.get(); + auto range = pendingNodesView.equal_range(proofid); + + // We want to update the nodes then remove them from the pending set. That + // will invalidate the range iterators, so we need to save the node ids + // first before we can loop over them. + std::vector nodeids; + nodeids.reserve(std::distance(range.first, range.second)); + std::transform(range.first, range.second, std::back_inserter(nodeids), + [](const PendingNode &n) { return n.nodeid; }); + + for (const NodeId &nodeid : nodeids) { + addOrUpdateNode(inserted.first, nodeid); + } + return inserted.first; } @@ -285,10 +312,17 @@ // Remove all nodes from this peer. removeNodeFromPeer(it, it->node_count); + auto &nview = nodes.get(); + + // Add the nodes to the pending set + auto range = nview.equal_range(peerid); + for (auto &nit = range.first; nit != range.second; ++nit) { + pendingNodes.emplace(it->getProofId(), nit->nodeid); + }; + // Remove nodes associated with this peer, unless their timeout is still // active. This ensure that we don't overquery them in case they are // subsequently added to another peer. - auto &nview = nodes.get(); nview.erase(nview.lower_bound(boost::make_tuple(peerid, TimePoint())), nview.upper_bound(boost::make_tuple( peerid, std::chrono::steady_clock::now()))); 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 @@ -15,6 +15,24 @@ using namespace avalanche; +namespace avalanche { +namespace { + struct TestPeerManager { + static bool nodeBelongToPeer(const PeerManager &pm, NodeId nodeid, + PeerId peerid) { + return pm.forNode(nodeid, [&](const Node &node) { + return node.peerid == peerid; + }); + } + + static bool isNodePending(const PeerManager &pm, NodeId nodeid) { + auto &pendingNodesView = pm.pendingNodes.get(); + return pendingNodesView.find(nodeid) != pendingNodesView.end(); + } + }; +} // namespace +} // namespace avalanche + BOOST_FIXTURE_TEST_SUITE(peermanager_tests, TestingSetup) BOOST_AUTO_TEST_CASE(select_peer_linear) { @@ -366,6 +384,144 @@ } } +BOOST_AUTO_TEST_CASE(node_binding) { + avalanche::PeerManager pm; + + auto proof = getRandomProofPtr(MIN_VALID_PROOF_SCORE); + const ProofId &proofid = proof->getId(); + + // Add a bunch of nodes with no associated peer + for (int i = 0; i < 10; i++) { + BOOST_CHECK(!pm.addNode(i, proofid)); + BOOST_CHECK(TestPeerManager::isNodePending(pm, i)); + } + + // Now create the peer and check all the nodes are bound + const PeerId peerid = pm.getPeerId(proof); + BOOST_CHECK_NE(peerid, NO_PEER); + for (int i = 0; i < 10; i++) { + BOOST_CHECK(!TestPeerManager::isNodePending(pm, i)); + BOOST_CHECK(TestPeerManager::nodeBelongToPeer(pm, i, peerid)); + } + BOOST_CHECK(pm.verify()); + + // Disconnect some nodes + for (int i = 0; i < 5; i++) { + BOOST_CHECK(pm.removeNode(i)); + BOOST_CHECK(!TestPeerManager::isNodePending(pm, i)); + BOOST_CHECK(!TestPeerManager::nodeBelongToPeer(pm, i, peerid)); + } + + // Add nodes when the peer already exists + for (int i = 0; i < 5; i++) { + BOOST_CHECK(pm.addNode(i, proofid)); + BOOST_CHECK(!TestPeerManager::isNodePending(pm, i)); + BOOST_CHECK(TestPeerManager::nodeBelongToPeer(pm, i, peerid)); + } + + auto alt_proof = getRandomProofPtr(MIN_VALID_PROOF_SCORE); + const ProofId &alt_proofid = alt_proof->getId(); + + // Update some nodes from a known proof to an unknown proof + for (int i = 0; i < 5; i++) { + BOOST_CHECK(!pm.addNode(i, alt_proofid)); + BOOST_CHECK(TestPeerManager::isNodePending(pm, i)); + BOOST_CHECK(!TestPeerManager::nodeBelongToPeer(pm, i, peerid)); + } + + auto alt2_proof = getRandomProofPtr(MIN_VALID_PROOF_SCORE); + const ProofId &alt2_proofid = alt2_proof->getId(); + + // Update some nodes from an unknown proof to another unknown proof + for (int i = 0; i < 5; i++) { + BOOST_CHECK(!pm.addNode(i, alt2_proofid)); + BOOST_CHECK(TestPeerManager::isNodePending(pm, i)); + } + + // Update some nodes from an unknown proof to a known proof + for (int i = 0; i < 5; i++) { + BOOST_CHECK(pm.addNode(i, proofid)); + BOOST_CHECK(!TestPeerManager::isNodePending(pm, i)); + BOOST_CHECK(TestPeerManager::nodeBelongToPeer(pm, i, peerid)); + } + + // Remove the peer, the nodes should be pending again + BOOST_CHECK(pm.removePeer(peerid)); + BOOST_CHECK(!pm.getProof(proof->getId())); + for (int i = 0; i < 10; i++) { + BOOST_CHECK(TestPeerManager::isNodePending(pm, i)); + BOOST_CHECK(!TestPeerManager::nodeBelongToPeer(pm, i, peerid)); + } + BOOST_CHECK(pm.verify()); +} + +BOOST_AUTO_TEST_CASE(node_binding_reorg) { + avalanche::PeerManager pm; + + ProofBuilder pb(0, 0, CPubKey()); + CKey key; + key.MakeNewKey(true); + const CScript script = GetScriptForDestination(PKHash(key.GetPubKey())); + COutPoint utxo(TxId(GetRandHash()), 0); + Amount amount = 1 * COIN; + const int height = 1234; + pb.addUTXO(utxo, amount, height, false, key); + auto proof = std::make_shared(pb.build()); + const ProofId &proofid = proof->getId(); + + { + LOCK(cs_main); + CCoinsViewCache &coins = ::ChainstateActive().CoinsTip(); + coins.AddCoin(utxo, Coin(CTxOut(amount, script), height, false), false); + } + + PeerId peerid = pm.getPeerId(proof); + BOOST_CHECK_NE(peerid, NO_PEER); + BOOST_CHECK(pm.verify()); + + // Add nodes to our peer + for (int i = 0; i < 10; i++) { + BOOST_CHECK(pm.addNode(i, proofid)); + BOOST_CHECK(!TestPeerManager::isNodePending(pm, i)); + BOOST_CHECK(TestPeerManager::nodeBelongToPeer(pm, i, peerid)); + } + + // Orphan the proof + { + LOCK(cs_main); + CCoinsViewCache &coins = ::ChainstateActive().CoinsTip(); + coins.SpendCoin(utxo); + } + + pm.updatedBlockTip(); + BOOST_CHECK(pm.isOrphan(proofid)); + BOOST_CHECK(!pm.getProof(proofid)); + for (int i = 0; i < 10; i++) { + BOOST_CHECK(TestPeerManager::isNodePending(pm, i)); + BOOST_CHECK(!TestPeerManager::nodeBelongToPeer(pm, i, peerid)); + } + BOOST_CHECK(pm.verify()); + + // Make the proof great again + { + LOCK(cs_main); + CCoinsViewCache &coins = ::ChainstateActive().CoinsTip(); + coins.AddCoin(utxo, Coin(CTxOut(amount, script), height, false), false); + } + + pm.updatedBlockTip(); + BOOST_CHECK(!pm.isOrphan(proofid)); + BOOST_CHECK(pm.getProof(proofid)); + // The peerid has certainly been updated + peerid = pm.getPeerId(proof); + BOOST_CHECK_NE(peerid, NO_PEER); + for (int i = 0; i < 10; i++) { + BOOST_CHECK(!TestPeerManager::isNodePending(pm, i)); + BOOST_CHECK(TestPeerManager::nodeBelongToPeer(pm, i, peerid)); + } + BOOST_CHECK(pm.verify()); +} + BOOST_AUTO_TEST_CASE(proof_conflict) { CKey key; key.MakeNewKey(true);