diff --git a/src/avalanche/proofbuilder.h b/src/avalanche/proofbuilder.h --- a/src/avalanche/proofbuilder.h +++ b/src/avalanche/proofbuilder.h @@ -29,7 +29,12 @@ SignedStake sign(const ProofId &proofid); }; - std::vector stakes; + struct StakeSignerComparator { + bool operator()(const StakeSigner &lhs, const StakeSigner &rhs) const { + return lhs.stake.getId() < rhs.stake.getId(); + } + }; + std::set stakes; public: ProofBuilder(uint64_t sequence_, int64_t expirationTime_, CPubKey master_) @@ -42,7 +47,7 @@ Proof build(); private: - ProofId getProofId(); + ProofId getProofId() const; friend struct TestProofBuilder; }; diff --git a/src/avalanche/proofbuilder.cpp b/src/avalanche/proofbuilder.cpp --- a/src/avalanche/proofbuilder.cpp +++ b/src/avalanche/proofbuilder.cpp @@ -25,10 +25,11 @@ return false; } - stakes.emplace_back( - Stake(std::move(utxo), amount, height, is_coinbase, key.GetPubKey()), - std::move(key)); - return true; + return stakes + .emplace(Stake(std::move(utxo), amount, height, is_coinbase, + key.GetPubKey()), + std::move(key)) + .second; } Proof ProofBuilder::build() { @@ -37,27 +38,20 @@ std::vector signedStakes; signedStakes.reserve(stakes.size()); - for (auto &s : stakes) { - signedStakes.push_back(s.sign(proofid)); + while (!stakes.empty()) { + auto handle = stakes.extract(stakes.begin()); + signedStakes.push_back(handle.value().sign(proofid)); } - stakes.clear(); return Proof(sequence, expirationTime, std::move(master), std::move(signedStakes)); } -ProofId ProofBuilder::getProofId() { +ProofId ProofBuilder::getProofId() const { CHashWriter ss(SER_GETHASH, 0); ss << sequence; ss << expirationTime; - // TODO This can be avoided by using a sorted container instead of a vector - // for the stakes. - std::sort(stakes.begin(), stakes.end(), - [](const StakeSigner &lhs, const StakeSigner &rhs) { - return lhs.stake.getId() < rhs.stake.getId(); - }); - WriteCompactSize(ss, stakes.size()); for (const auto &s : stakes) { ss << s.stake; 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 @@ -577,8 +577,14 @@ BOOST_CHECK(peer4 != NO_PEER && peer4 != peer1); // Duplicated input. - BOOST_CHECK_EQUAL(getPeerId({COutPoint(txid1, 3), COutPoint(txid1, 3)}), - NO_PEER); + { + ProofBuilder pb(0, 0, CPubKey()); + COutPoint o(txid1, 3); + pb.addUTXO(o, v, height, false, key); + PeerId peerid = pm.getPeerId(std::make_shared( + TestProofBuilder::buildDuplicatedStakes(pb))); + BOOST_CHECK_EQUAL(peerid, NO_PEER); + } // Multiple inputs, collision on first input. BOOST_CHECK_EQUAL(getPeerId({COutPoint(txid1, 0), COutPoint(txid2, 4)}), diff --git a/src/avalanche/test/proof_tests.cpp b/src/avalanche/test/proof_tests.cpp --- a/src/avalanche/test/proof_tests.cpp +++ b/src/avalanche/test/proof_tests.cpp @@ -434,8 +434,7 @@ { ProofBuilder pb(0, 0, pubkey); pb.addUTXO(pkh_outpoint, value, height, false, key); - pb.addUTXO(pkh_outpoint, value, height, false, key); - Proof p = pb.build(); + Proof p = TestProofBuilder::buildDuplicatedStakes(pb); ProofValidationState state; BOOST_CHECK(!p.verify(state, coins)); diff --git a/src/avalanche/test/util.h b/src/avalanche/test/util.h --- a/src/avalanche/test/util.h +++ b/src/avalanche/test/util.h @@ -22,6 +22,8 @@ struct TestProofBuilder { static ProofId getReverseOrderProofId(ProofBuilder &pb); static Proof buildWithReversedOrderStakes(ProofBuilder &pb); + static ProofId getDuplicatedStakeProofId(ProofBuilder &pb); + static Proof buildDuplicatedStakes(ProofBuilder &pb); }; } // namespace avalanche diff --git a/src/avalanche/test/util.cpp b/src/avalanche/test/util.cpp --- a/src/avalanche/test/util.cpp +++ b/src/avalanche/test/util.cpp @@ -50,16 +50,9 @@ ss << pb.sequence; ss << pb.expirationTime; - // Reverse the sorting order - std::sort(pb.stakes.begin(), pb.stakes.end(), - [](const ProofBuilder::StakeSigner &lhs, - const ProofBuilder::StakeSigner &rhs) { - return lhs.stake.getId() > rhs.stake.getId(); - }); - WriteCompactSize(ss, pb.stakes.size()); - for (const auto &s : pb.stakes) { - ss << s.stake; + for (auto it = pb.stakes.rbegin(); it != pb.stakes.rend(); it++) { + ss << it->stake; } CHashWriter ss2(SER_GETHASH, 0); @@ -75,11 +68,48 @@ std::vector signedStakes; signedStakes.reserve(pb.stakes.size()); + while (!pb.stakes.empty()) { + // We need a forward iterator, so pb.stakes.rbegin() is not an + // option. + auto handle = pb.stakes.extract(std::prev(pb.stakes.end())); + signedStakes.push_back(handle.value().sign(proofid)); + } + + return Proof(pb.sequence, pb.expirationTime, std::move(pb.master), + std::move(signedStakes)); +} + +ProofId TestProofBuilder::getDuplicatedStakeProofId(ProofBuilder &pb) { + CHashWriter ss(SER_GETHASH, 0); + ss << pb.sequence; + ss << pb.expirationTime; + + WriteCompactSize(ss, 2 * pb.stakes.size()); for (auto &s : pb.stakes) { - signedStakes.push_back(s.sign(proofid)); + ss << s.stake; + ss << s.stake; + } + + CHashWriter ss2(SER_GETHASH, 0); + ss2 << ss.GetHash(); + ss2 << pb.master; + + return ProofId(ss2.GetHash()); +} + +Proof TestProofBuilder::buildDuplicatedStakes(ProofBuilder &pb) { + const ProofId proofid = TestProofBuilder::getDuplicatedStakeProofId(pb); + + std::vector signedStakes; + signedStakes.reserve(2 * pb.stakes.size()); + + while (!pb.stakes.empty()) { + auto handle = pb.stakes.extract(pb.stakes.begin()); + SignedStake signedStake = handle.value().sign(proofid); + signedStakes.push_back(signedStake); + signedStakes.push_back(signedStake); } - pb.stakes.clear(); return Proof(pb.sequence, pb.expirationTime, std::move(pb.master), std::move(signedStakes)); } diff --git a/test/functional/abc_rpc_avalancheproof.py b/test/functional/abc_rpc_avalancheproof.py --- a/test/functional/abc_rpc_avalancheproof.py +++ b/test/functional/abc_rpc_avalancheproof.py @@ -234,10 +234,6 @@ create_coinbase_stakes(node, [blockhashes[0]], addrkey0.key, amount=str(dust_amount))) - duplicate_stake = node.buildavalancheproof( - proof_sequence, proof_expiration, proof_master, - create_coinbase_stakes(node, [blockhashes[0]] * 2, addrkey0.key)) - missing_stake = node.buildavalancheproof( proof_sequence, proof_expiration, proof_master, [{ 'txid': '0' * 64, @@ -249,6 +245,21 @@ }] ) + duplicate_stake = ("0b000000000000000c0000000000000021030b4c866585dd868" + "a9d62348a9cd008d6a312937048fff31670e7e920cfc7a74402" + "05c5f72f5d6da3085583e75ee79340eb4eff208c89988e7ed0e" + "fb30b87298fa30000000000f2052a0100000003000000210227" + "d85ba011276cf25b51df6a188b75e604b38770a462b2d0e9fb2" + "fc839ef5d3f86076def2e8bc3c40671c1a0eb505da5857a950a" + "0cf4625a80018cdd75ac62e61273ff8142f747de67e73f6368c" + "8648942b0ef6c065d72a81ad7438a23c11cca05c5f72f5d6da3" + "085583e75ee79340eb4eff208c89988e7ed0efb30b87298fa30" + "000000000f2052a0100000003000000210227d85ba011276cf2" + "5b51df6a188b75e604b38770a462b2d0e9fb2fc839ef5d3f860" + "76def2e8bc3c40671c1a0eb505da5857a950a0cf4625a80018c" + "dd75ac62e61273ff8142f747de67e73f6368c8648942b0ef6c0" + "65d72a81ad7438a23c11cca") + bad_sig = ("0b000000000000000c0000000000000021030b4c866585dd868a9d62348" "a9cd008d6a312937048fff31670e7e920cfc7a7440105c5f72f5d6da3085" "583e75ee79340eb4eff208c89988e7ed0efb30b87298fa30000000000f20"