From 8c6211357b101b9f961d32d3da729fd17a40f472 Mon Sep 17 00:00:00 2001 From: Dmitriy Khaustov aka xDimon Date: Wed, 21 Sep 2022 16:20:49 +0800 Subject: [PATCH 1/7] feature: reputation Signed-off-by: Dmitriy Khaustov aka xDimon --- core/consensus/grandpa/grandpa_context.hpp | 3 + core/consensus/grandpa/impl/grandpa_impl.cpp | 294 +++++++++++++++++- core/consensus/grandpa/impl/grandpa_impl.hpp | 43 ++- .../grandpa/impl/voting_round_impl.cpp | 49 ++- core/injector/CMakeLists.txt | 2 +- core/injector/application_injector.cpp | 9 +- core/network/impl/CMakeLists.txt | 6 +- core/network/impl/peer_manager_impl.cpp | 27 +- core/network/impl/peer_manager_impl.hpp | 6 +- .../impl/protocols/protocol_factory.cpp | 8 +- .../impl/protocols/protocol_factory.hpp | 9 +- .../impl/protocols/protocol_req_collation.cpp | 4 +- .../impl/protocols/sync_protocol_impl.cpp | 12 +- .../impl/protocols/sync_protocol_impl.hpp | 13 +- core/network/impl/rating_repository_impl.cpp | 87 ------ core/network/impl/rating_repository_impl.hpp | 52 ---- .../impl/reputation_repository_impl.cpp | 77 +++++ .../impl/reputation_repository_impl.hpp | 46 +++ core/network/impl/stream_engine.hpp | 12 +- core/network/rating_repository.hpp | 78 ----- core/network/reputation_change.hpp | 106 +++++++ core/network/reputation_repository.hpp | 54 ++++ test/core/network/stream_engine_test.cpp | 10 +- .../core/network/rating_repository_mock.hpp | 43 --- .../network/reputation_repository_mock.hpp | 32 ++ 25 files changed, 732 insertions(+), 350 deletions(-) delete mode 100644 core/network/impl/rating_repository_impl.cpp delete mode 100644 core/network/impl/rating_repository_impl.hpp create mode 100644 core/network/impl/reputation_repository_impl.cpp create mode 100644 core/network/impl/reputation_repository_impl.hpp delete mode 100644 core/network/rating_repository.hpp create mode 100644 core/network/reputation_change.hpp create mode 100644 core/network/reputation_repository.hpp delete mode 100644 test/mock/core/network/rating_repository_mock.hpp create mode 100644 test/mock/core/network/reputation_repository_mock.hpp diff --git a/core/consensus/grandpa/grandpa_context.hpp b/core/consensus/grandpa/grandpa_context.hpp index e8cf5be8a0..4445490477 100644 --- a/core/consensus/grandpa/grandpa_context.hpp +++ b/core/consensus/grandpa/grandpa_context.hpp @@ -42,6 +42,9 @@ namespace kagome::consensus::grandpa { std::optional commit{}; std::set> missing_blocks{}; + size_t checked_signature_counter = 0; + size_t invalid_signature_counter = 0; + size_t unknown_voter_counter = 0; static void set(std::shared_ptr context) { auto &opt = instance(); diff --git a/core/consensus/grandpa/impl/grandpa_impl.cpp b/core/consensus/grandpa/impl/grandpa_impl.cpp index f3f8e249db..7de5c31ef7 100644 --- a/core/consensus/grandpa/impl/grandpa_impl.cpp +++ b/core/consensus/grandpa/impl/grandpa_impl.cpp @@ -12,12 +12,18 @@ #include "consensus/grandpa/impl/voting_round_impl.hpp" #include "consensus/grandpa/vote_graph/vote_graph_impl.hpp" #include "network/helpers/peer_id_formatter.hpp" +#include "network/reputation_repository.hpp" #include "scale/scale.hpp" namespace { constexpr auto highestGrandpaRoundMetricName = "kagome_finality_grandpa_round"; -} + + template + auto toMilliseconds(D &&duration) { + return std::chrono::duration_cast(duration); + } +} // namespace namespace kagome::consensus::grandpa { @@ -34,7 +40,8 @@ namespace kagome::consensus::grandpa { std::shared_ptr authority_manager, std::shared_ptr synchronizer, std::shared_ptr peer_manager, - std::shared_ptr block_tree) + std::shared_ptr block_tree, + std::shared_ptr reputation_repository) : environment_{std::move(environment)}, crypto_provider_{std::move(crypto_provider)}, grandpa_api_{std::move(grandpa_api)}, @@ -44,7 +51,8 @@ namespace kagome::consensus::grandpa { authority_manager_(std::move(authority_manager)), synchronizer_(std::move(synchronizer)), peer_manager_(std::move(peer_manager)), - block_tree_(std::move(block_tree)) { + block_tree_(std::move(block_tree)), + reputation_repository_(std::move(reputation_repository)) { BOOST_ASSERT(environment_ != nullptr); BOOST_ASSERT(crypto_provider_ != nullptr); BOOST_ASSERT(grandpa_api_ != nullptr); @@ -54,6 +62,7 @@ namespace kagome::consensus::grandpa { BOOST_ASSERT(synchronizer_ != nullptr); BOOST_ASSERT(peer_manager_ != nullptr); BOOST_ASSERT(block_tree_ != nullptr); + BOOST_ASSERT(reputation_repository_ != nullptr); BOOST_ASSERT(app_state_manager != nullptr); @@ -158,7 +167,7 @@ namespace kagome::consensus::grandpa { GrandpaConfig config{.voters = std::move(voters), .round_number = round_state.round_number, - .duration = round_time_factor_, + .duration = kRoundTimeFactor, .id = keypair_ ? std::make_optional(keypair_->public_key) : std::nullopt}; @@ -219,7 +228,7 @@ namespace kagome::consensus::grandpa { GrandpaConfig config{.voters = std::move(voters), .round_number = new_round_number, - .duration = round_time_factor_, + .duration = kRoundTimeFactor, .id = keypair_ ? std::make_optional(keypair_->public_key) : std::nullopt}; @@ -365,15 +374,63 @@ namespace kagome::consensus::grandpa { } } + bool reputation_changed = false; + if (info.has_value() and info->get().set_id.has_value() + and info->get().round_number.has_value()) { + const auto prev_set_id = info->get().set_id.value(); + const auto prev_round_number = info->get().round_number.value(); + + // bad order of set id + if (msg.voter_set_id < prev_set_id) { + reputation_repository_->change( + peer_id, network::reputation::cost::INVALID_VIEW_CHANGE); + reputation_changed = true; + } + + // bad order of round number + if (msg.voter_set_id == prev_set_id + and msg.round_number < prev_round_number) { + reputation_repository_->change( + peer_id, network::reputation::cost::INVALID_VIEW_CHANGE); + reputation_changed = true; + } + } + peer_manager_->updatePeerState(peer_id, msg); + if (not reputation_changed) { + reputation_repository_->change( + peer_id, network::reputation::benefit::NEIGHBOR_MESSAGE); + } + // If peer has the same voter set id if (msg.voter_set_id == current_round_->voterSetId()) { // Check if needed to catch-up peer, then do that if (msg.round_number >= current_round_->roundNumber() + kCatchUpThreshold) { - std::ignore = environment_->onCatchUpRequested( + auto res = environment_->onCatchUpRequested( peer_id, msg.voter_set_id, msg.round_number - 1); + if (res.has_value()) { + pending_catchup_request_.emplace( + peer_id, + network::CatchUpRequest{msg.round_number - 1, msg.voter_set_id}); + catchup_request_timer_handle_ = scheduler_->scheduleWithHandle( + [wp = weak_from_this()] { + auto self = wp.lock(); + if (not self) { + return; + } + if (self->pending_catchup_request_.has_value()) { + const auto &peer_id = + std::get<0>(self->pending_catchup_request_.value()); + self->reputation_repository_->change( + peer_id, + network::reputation::cost::CATCH_UP_REQUEST_TIMEOUT); + self->pending_catchup_request_.reset(); + } + }, + toMilliseconds(kCatchupRequestTimeout)); + } } return; @@ -415,6 +472,63 @@ namespace kagome::consensus::grandpa { void GrandpaImpl::onCatchUpRequest(const libp2p::peer::PeerId &peer_id, const network::CatchUpRequest &msg) { + auto info_opt = peer_manager_->getPeerState(peer_id); + if (not info_opt.has_value() or not info_opt->get().set_id.has_value() + or not info_opt->get().round_number.has_value()) { + SL_DEBUG(logger_, + "Catch-up request to round #{} received from {} was rejected: " + "we are not have our view about remote peer", + msg.round_number, + peer_id); + reputation_repository_->change( + peer_id, network::reputation::cost::OUT_OF_SCOPE_MESSAGE); + return; + } + const auto &info = info_opt->get(); + + // Check if request is corresponding our view about remote peer by set id + if (msg.voter_set_id != info.set_id.value()) { + SL_DEBUG(logger_, + "Catch-up request to round #{} received from {} was rejected: " + "it is not corresponding our view about remote peer ", + msg.round_number, + peer_id, + current_round_->voterSetId(), + msg.voter_set_id); + + // NOTE: When we're close to a set change there is potentially a + // race where the peer sent us the request before it observed that + // we had transitioned to a new set. In this case we charge a lower + // cost. + if (msg.voter_set_id == info.set_id.value() + and msg.round_number + < info.round_number.value() + kCatchUpThreshold) { + reputation_repository_->change( + peer_id, network::reputation::cost::HONEST_OUT_OF_SCOPE_CATCH_UP); + return; + } + + reputation_repository_->change( + peer_id, network::reputation::cost::OUT_OF_SCOPE_MESSAGE); + return; + } + + // Check if request is corresponding our view about remote peer by round + // number + if (msg.round_number <= info.round_number.value()) { + SL_DEBUG(logger_, + "Catch-up request to round #{} received from {} was rejected: " + "it is not corresponding our view about remote peer ", + msg.round_number, + peer_id, + current_round_->voterSetId(), + msg.voter_set_id); + + reputation_repository_->change( + peer_id, network::reputation::cost::OUT_OF_SCOPE_MESSAGE); + return; + } + // It is also impolite to send a catch-up request to a peer in a new // different Set ID. if (msg.voter_set_id != current_round_->voterSetId()) { @@ -437,6 +551,9 @@ namespace kagome::consensus::grandpa { msg.round_number, peer_id, current_round_->roundNumber()); + + reputation_repository_->change( + peer_id, network::reputation::cost::OUT_OF_SCOPE_MESSAGE); return; } @@ -465,10 +582,52 @@ namespace kagome::consensus::grandpa { msg.round_number, peer_id); round->doCatchUpResponse(peer_id); + + reputation_repository_->change(peer_id, + network::reputation::cost::CATCH_UP_REPLY); } void GrandpaImpl::onCatchUpResponse(const libp2p::peer::PeerId &peer_id, const network::CatchUpResponse &msg) { + GrandpaContext::Guard cg; + + auto ctx = GrandpaContext::get().value(); + if (not ctx->peer_id.has_value()) { + if (not pending_catchup_request_.has_value()) { + reputation_repository_->change( + peer_id, network::reputation::cost::MALFORMED_CATCH_UP); + return; + } + + const auto &[remote_peer_id, catchup_request] = + pending_catchup_request_.value(); + + if (peer_id != remote_peer_id) { + reputation_repository_->change( + peer_id, network::reputation::cost::OUT_OF_SCOPE_MESSAGE); + return; + } + + if (msg.voter_set_id != catchup_request.voter_set_id + or msg.round_number != catchup_request.round_number) { + reputation_repository_->change( + peer_id, network::reputation::cost::MALFORMED_CATCH_UP); + return; + } + + if (msg.prevote_justification.empty() + or msg.precommit_justification.empty()) { + reputation_repository_->change( + peer_id, network::reputation::cost::MALFORMED_CATCH_UP); + return; + } + + auto cleanup = gsl::finally([this] { + catchup_request_timer_handle_.cancel(); + pending_catchup_request_.reset(); + }); + } + BOOST_ASSERT(current_round_ != nullptr); // Ignore message of peer whose round in different voter set if (msg.voter_set_id != current_round_->voterSetId()) { @@ -499,8 +658,6 @@ namespace kagome::consensus::grandpa { msg.round_number, peer_id); - GrandpaContext::Guard cg; - if (msg.round_number > current_round_->roundNumber()) { MovableRoundState round_state{ .round_number = msg.round_number, @@ -541,7 +698,20 @@ namespace kagome::consensus::grandpa { if (not round->completable() and not round->finalizedBlock().has_value()) { - auto ctx = GrandpaContext::get().value(); + // Met unknown voter - cost reputation + if (ctx->unknown_voter_counter > 0) { + reputation_repository_->change( + peer_id, + network::reputation::cost::UNKNOWN_VOTER + * ctx->unknown_voter_counter); + } + // Met invalid signature - cost reputation + if (ctx->invalid_signature_counter > 0) { + reputation_repository_->change( + peer_id, + network::reputation::cost::BAD_CATCHUP_RESPONSE + * ctx->checked_signature_counter); + } // Check if missed block are detected and if this is first attempt // (considering by definition peer id in context) if (not ctx->missing_blocks.empty()) { @@ -583,7 +753,20 @@ namespace kagome::consensus::grandpa { // Check if catch-up round is not completable if (not current_round_->completable()) { - auto ctx = GrandpaContext::get().value(); + // Met unknown voter - cost reputation + if (ctx->unknown_voter_counter > 0) { + reputation_repository_->change( + peer_id, + network::reputation::cost::UNKNOWN_VOTER + * ctx->unknown_voter_counter); + } + // Met invalid signature - cost reputation + if (ctx->invalid_signature_counter > 0) { + reputation_repository_->change( + peer_id, + network::reputation::cost::BAD_CATCHUP_RESPONSE + * ctx->checked_signature_counter); + } // Check if missed block are detected and if this is first attempt // (considering by definition peer id in context) if (not ctx->missing_blocks.empty()) { @@ -598,10 +781,33 @@ namespace kagome::consensus::grandpa { } tryExecuteNextRound(current_round_); + + reputation_repository_->change( + peer_id, network::reputation::benefit::BASIC_VALIDATED_CATCH_UP); } void GrandpaImpl::onVoteMessage(const libp2p::peer::PeerId &peer_id, const VoteMessage &msg) { + auto info = peer_manager_->getPeerState(peer_id); + if (not info.has_value() or not info->get().set_id.has_value() + or not info->get().round_number.has_value()) { + SL_DEBUG( + logger_, + "{} signed by {} with set_id={} in round={} has received from {} " + "and we are not have our view about remote peer", + msg.vote.is() ? "Prevote" + : msg.vote.is() ? "Precommit" + : "PrimaryPropose", + msg.id(), + msg.counter, + msg.round_number, + peer_id, + current_round_->voterSetId()); + reputation_repository_->change( + peer_id, network::reputation::cost::OUT_OF_SCOPE_MESSAGE); + return; + } + // If a peer is at a given voter set, it is impolite to send messages from // an earlier voter set. if (msg.counter < current_round_->voterSetId()) { @@ -617,6 +823,8 @@ namespace kagome::consensus::grandpa { msg.round_number, peer_id, current_round_->voterSetId()); + reputation_repository_->change(peer_id, + network::reputation::cost::PAST_REJECTION); return; } @@ -634,9 +842,19 @@ namespace kagome::consensus::grandpa { msg.round_number, peer_id, current_round_->voterSetId()); + reputation_repository_->change(peer_id, + network::reputation::cost::FUTURE_MESSAGE); return; } + if (msg.round_number > current_round_->roundNumber() + 1) { + reputation_repository_->change(peer_id, + network::reputation::cost::FUTURE_MESSAGE); + } else if (msg.round_number < current_round_->roundNumber() - 1) { + reputation_repository_->change(peer_id, + network::reputation::cost::PAST_REJECTION); + } + // If the current peer is at round r, it is impolite to receive messages // about r-2 or earlier if (msg.round_number + 2 < current_round_->roundNumber()) { @@ -724,15 +942,34 @@ namespace kagome::consensus::grandpa { is_precommits_changed = true; } }); + + auto ctx = GrandpaContext::get().value(); + + // Met invalid signature - cost reputation + if (ctx->invalid_signature_counter > 0) { + reputation_repository_->change(peer_id, + network::reputation::cost::BAD_SIGNATURE + * ctx->checked_signature_counter); + } + + // Met unknown voter - cost reputation + if (ctx->unknown_voter_counter > 0) { + reputation_repository_->change(peer_id, + network::reputation::cost::UNKNOWN_VOTER + * ctx->unknown_voter_counter); + } + if (is_prevotes_changed or is_precommits_changed) { target_round->update( VotingRound::IsPreviousRoundChanged{false}, VotingRound::IsPrevotesChanged{is_prevotes_changed}, VotingRound::IsPrecommitsChanged{is_precommits_changed}); + + reputation_repository_->change( + peer_id, network::reputation::benefit::ROUND_MESSAGE); } if (not target_round->finalizedBlock().has_value()) { - auto ctx = GrandpaContext::get().value(); // Check if missed block are detected and if this is first attempt // (considering by definition peer id in context) if (not ctx->missing_blocks.empty()) { @@ -748,6 +985,12 @@ namespace kagome::consensus::grandpa { void GrandpaImpl::onCommitMessage(const libp2p::peer::PeerId &peer_id, const network::FullCommitMessage &msg) { + // TODO check if height of commit less then previous one + // if (new_commit_height < last_commit_height) { + // reputation_repository_->change( + // peer_id, network::reputation::cost::INVALID_VIEW_CHANGE); + // } + // It is especially impolite to send commits which are invalid, or from // a different Set ID than the receiving peer has indicated if (msg.set_id != current_round_->voterSetId()) { @@ -760,12 +1003,18 @@ namespace kagome::consensus::grandpa { BlockInfo(msg.message.target_number, msg.message.target_hash), peer_id, current_round_->voterSetId()); + + reputation_repository_->change( + peer_id, + msg.set_id < current_round_->voterSetId() + ? network::reputation::cost::PAST_REJECTION + : network::reputation::cost::FUTURE_MESSAGE); return; } // It is impolite to send commits which are earlier than the last commit // sent - if (msg.round + kKeepRecentRounds < current_round_->voterSetId()) { + if (msg.round + kKeepRecentRounds < current_round_->roundNumber()) { SL_DEBUG( logger_, "Commit with set_id={} in round={} for block {} has received from {} " @@ -778,6 +1027,21 @@ namespace kagome::consensus::grandpa { return; } + if (msg.message.precommits.empty() + or msg.message.auth_data.size() != msg.message.precommits.size()) { + reputation_repository_->change( + peer_id, network::reputation::cost::MALFORMED_COMMIT); + } + + if (auto prev_round = current_round_->getPreviousRound()) { + if (auto finalized_opt = prev_round->finalizedBlock()) { + if (msg.message.target_number < finalized_opt->number) { + reputation_repository_->change( + peer_id, network::reputation::cost::PAST_REJECTION); + } + } + } + if (msg.round < current_round_->roundNumber()) { SL_DEBUG( logger_, @@ -828,6 +1092,9 @@ namespace kagome::consensus::grandpa { res.error().message()); return; } + + reputation_repository_->change( + peer_id, network::reputation::benefit::BASIC_VALIDATED_COMMIT); } outcome::result GrandpaImpl::applyJustification( @@ -890,8 +1157,7 @@ namespace kagome::consensus::grandpa { return VotingRoundError::JUSTIFICATION_FOR_AUTHORITY_SET_IN_PAST; } if (authority_set->id == current_round_->voterSetId() - && justification.round_number - < current_round_->roundNumber()) { + && justification.round_number < current_round_->roundNumber()) { return VotingRoundError::JUSTIFICATION_FOR_ROUND_IN_PAST; } diff --git a/core/consensus/grandpa/impl/grandpa_impl.hpp b/core/consensus/grandpa/impl/grandpa_impl.hpp index d3923fdbc3..906c9c0597 100644 --- a/core/consensus/grandpa/impl/grandpa_impl.hpp +++ b/core/consensus/grandpa/impl/grandpa_impl.hpp @@ -23,6 +23,7 @@ #include "log/logger.hpp" #include "metrics/metrics.hpp" #include "network/peer_manager.hpp" +#include "network/reputation_repository.hpp" #include "network/synchronizer.hpp" #include "runtime/runtime_api/grandpa_api.hpp" @@ -65,17 +66,19 @@ namespace kagome::consensus::grandpa { ~GrandpaImpl() override = default; - GrandpaImpl(std::shared_ptr app_state_manager, - std::shared_ptr environment, - std::shared_ptr crypto_provider, - std::shared_ptr grandpa_api, - const std::shared_ptr &keypair, - std::shared_ptr clock, - std::shared_ptr scheduler, - std::shared_ptr authority_manager, - std::shared_ptr synchronizer, - std::shared_ptr peer_manager, - std::shared_ptr block_tree); + GrandpaImpl( + std::shared_ptr app_state_manager, + std::shared_ptr environment, + std::shared_ptr crypto_provider, + std::shared_ptr grandpa_api, + const std::shared_ptr &keypair, + std::shared_ptr clock, + std::shared_ptr scheduler, + std::shared_ptr authority_manager, + std::shared_ptr synchronizer, + std::shared_ptr peer_manager, + std::shared_ptr block_tree, + std::shared_ptr reputation_repository); /** * Prepares for grandpa round execution: e.g. sets justification observer @@ -218,8 +221,7 @@ namespace kagome::consensus::grandpa { * nullopt otherwise */ std::optional> selectRound( - RoundNumber round_number, - std::optional voter_set_id); + RoundNumber round_number, std::optional voter_set_id); /** * @return Get grandpa::MovableRoundState for the last completed round @@ -254,9 +256,10 @@ namespace kagome::consensus::grandpa { // Note: Duration value was gotten from substrate // https://github.com/paritytech/substrate/blob/efbac7be80c6e8988a25339061078d3e300f132d/bin/node-template/node/src/service.rs#L166 // Perhaps, 333ms is not enough for normal communication during the round - const Clock::Duration round_time_factor_ = std::chrono::milliseconds(333); + const Clock::Duration kRoundTimeFactor = std::chrono::milliseconds(333); - std::shared_ptr current_round_; + const Clock::Duration kCatchupRequestTimeout = + std::chrono::milliseconds(45'000); std::shared_ptr environment_; std::shared_ptr crypto_provider_; @@ -268,13 +271,19 @@ namespace kagome::consensus::grandpa { std::shared_ptr synchronizer_; std::shared_ptr peer_manager_; std::shared_ptr block_tree_; + std::shared_ptr reputation_repository_; + + std::shared_ptr current_round_; + std::optional< + const std::tuple> + pending_catchup_request_; + libp2p::basic::Scheduler::Handle catchup_request_timer_handle_; + libp2p::basic::Scheduler::Handle fallback_timer_handle_; // Metrics metrics::RegistryPtr metrics_registry_ = metrics::createRegistry(); metrics::Gauge *metric_highest_round_; - libp2p::basic::Scheduler::Handle fallback_timer_handle_; - log::Logger logger_ = log::createLogger("Grandpa", "grandpa"); }; diff --git a/core/consensus/grandpa/impl/voting_round_impl.cpp b/core/consensus/grandpa/impl/voting_round_impl.cpp index bc9ca498b4..7c4e201916 100644 --- a/core/consensus/grandpa/impl/voting_round_impl.cpp +++ b/core/consensus/grandpa/impl/voting_round_impl.cpp @@ -877,6 +877,11 @@ namespace kagome::consensus::grandpa { return; } + if (auto ctx_opt = GrandpaContext::get()) { + const auto &ctx = ctx_opt.value(); + ctx->checked_signature_counter++; + } + bool isValid = vote_crypto_provider_->verifyPrimaryPropose(proposal); if (not isValid) { logger_->warn( @@ -884,9 +889,23 @@ namespace kagome::consensus::grandpa { "invalid signature", round_number_, proposal.id); + + if (auto ctx_opt = GrandpaContext::get()) { + const auto &ctx = ctx_opt.value(); + ctx->invalid_signature_counter++; + } + return; } + auto result = voter_set_->indexAndWeight(proposal.id); + if (result == outcome::failure(VoterSet::Error::VOTER_NOT_FOUND)) { + if (auto ctx_opt = GrandpaContext::get()) { + const auto &ctx = ctx_opt.value(); + ctx->unknown_voter_counter++; + } + } + SL_DEBUG(logger_, "Round #{}: Proposal signed by {} was accepted for block {}", round_number_, @@ -899,7 +918,7 @@ namespace kagome::consensus::grandpa { // Check if node hasn't block auto res = env_->hasBlock(proposal.getBlockHash()); if (res.has_value() and not res.value()) { - if (auto ctx_opt = GrandpaContext::get()) { + if (auto ctx_opt = GrandpaContext::get(); ctx_opt.has_value()) { auto ctx = ctx_opt.value(); ctx->missing_blocks.emplace(proposal.getBlockInfo()); } @@ -921,12 +940,23 @@ namespace kagome::consensus::grandpa { bool VotingRoundImpl::onPrevote(const SignedMessage &prevote, Propagation propagation) { + if (auto ctx_opt = GrandpaContext::get()) { + const auto &ctx = ctx_opt.value(); + ctx->checked_signature_counter++; + } + bool isValid = vote_crypto_provider_->verifyPrevote(prevote); if (not isValid) { logger_->warn( "Round #{}: Prevote signed by {} was rejected: invalid signature", round_number_, prevote.id); + + if (auto ctx_opt = GrandpaContext::get()) { + const auto &ctx = ctx_opt.value(); + ctx->invalid_signature_counter++; + } + return false; } @@ -938,6 +968,12 @@ namespace kagome::consensus::grandpa { == outcome::failure(VotingRoundError::VOTE_OF_KNOWN_EQUIVOCATOR)) { return false; } + if (result == outcome::failure(VoterSet::Error::VOTER_NOT_FOUND)) { + if (auto ctx_opt = GrandpaContext::get()) { + const auto &ctx = ctx_opt.value(); + ctx->unknown_voter_counter++; + } + } if (result != outcome::failure(VotingRoundError::EQUIVOCATED_VOTE)) { logger_->warn("Round #{}: Prevote signed by {} was rejected: {}", round_number_, @@ -975,6 +1011,11 @@ namespace kagome::consensus::grandpa { bool VotingRoundImpl::onPrecommit(const SignedMessage &precommit, Propagation propagation) { + if (auto ctx_opt = GrandpaContext::get()) { + const auto &ctx = ctx_opt.value(); + ctx->checked_signature_counter++; + } + bool isValid = vote_crypto_provider_->verifyPrecommit(precommit); if (not isValid) { logger_->warn( @@ -982,6 +1023,12 @@ namespace kagome::consensus::grandpa { "invalid signature", round_number_, precommit.id); + + if (auto ctx_opt = GrandpaContext::get()) { + const auto &ctx = ctx_opt.value(); + ctx->invalid_signature_counter++; + } + return false; } diff --git a/core/injector/CMakeLists.txt b/core/injector/CMakeLists.txt index fb5950d7de..56a3483bac 100644 --- a/core/injector/CMakeLists.txt +++ b/core/injector/CMakeLists.txt @@ -121,7 +121,7 @@ target_link_libraries(application_injector offchain_worker_api offchain_persistent_storage offchain_local_storage - rating_repository + reputation_repository session_keys_api collation_protocol req_collation_protocol diff --git a/core/injector/application_injector.cpp b/core/injector/application_injector.cpp index 30c7e90bcc..da18d55eac 100644 --- a/core/injector/application_injector.cpp +++ b/core/injector/application_injector.cpp @@ -96,7 +96,7 @@ #include "network/impl/extrinsic_observer_impl.hpp" #include "network/impl/grandpa_transmitter_impl.hpp" #include "network/impl/peer_manager_impl.hpp" -#include "network/impl/rating_repository_impl.hpp" +#include "network/impl/reputation_repository_impl.hpp" #include "network/impl/router_libp2p.hpp" #include "network/impl/state_protocol_observer_impl.hpp" #include "network/impl/sync_protocol_observer_impl.hpp" @@ -672,7 +672,7 @@ namespace { injector.template create>(), injector.template create>(), injector.template create>(), - injector.template create>()); + injector.template create>()); auto protocol_factory = injector.template create>(); @@ -1225,7 +1225,7 @@ namespace { di::bind.template to(), di::bind.template to(), di::bind.template to(), - di::bind.template to(), + di::bind.template to(), di::bind.template to(), di::bind.template to(), di::bind.template to(), @@ -1451,7 +1451,8 @@ namespace { injector.template create>(), injector.template create>(), injector.template create>(), - injector.template create>()); + injector.template create>(), + injector.template create>()); auto protocol_factory = injector.template create>(); diff --git a/core/network/impl/CMakeLists.txt b/core/network/impl/CMakeLists.txt index 519bbd7946..595a44a272 100644 --- a/core/network/impl/CMakeLists.txt +++ b/core/network/impl/CMakeLists.txt @@ -81,10 +81,10 @@ target_link_libraries(peer_manager scale_libp2p_types ) -add_library(rating_repository - rating_repository_impl.cpp +add_library(reputation_repository + reputation_repository_impl.cpp ) -target_link_libraries(rating_repository +target_link_libraries(reputation_repository p2p::p2p_peer_id p2p::p2p_basic_scheduler ) diff --git a/core/network/impl/peer_manager_impl.cpp b/core/network/impl/peer_manager_impl.cpp index 94bbf826d9..db832cb99a 100644 --- a/core/network/impl/peer_manager_impl.cpp +++ b/core/network/impl/peer_manager_impl.cpp @@ -45,7 +45,7 @@ namespace kagome::network { std::shared_ptr router, std::shared_ptr storage, std::shared_ptr hasher, - std::shared_ptr peer_rating_repository) + std::shared_ptr reputation_repository) : app_state_manager_(std::move(app_state_manager)), host_(host), identify_(std::move(identify)), @@ -59,7 +59,7 @@ namespace kagome::network { router_{std::move(router)}, storage_{std::move(storage)}, hasher_{std::move(hasher)}, - peer_rating_repository_{std::move(peer_rating_repository)}, + reputation_repository_{std::move(reputation_repository)}, log_(log::createLogger("PeerManager", "network")) { BOOST_ASSERT(app_state_manager_ != nullptr); BOOST_ASSERT(identify_ != nullptr); @@ -69,7 +69,7 @@ namespace kagome::network { BOOST_ASSERT(router_ != nullptr); BOOST_ASSERT(storage_ != nullptr); BOOST_ASSERT(hasher_ != nullptr); - BOOST_ASSERT(peer_rating_repository_ != nullptr); + BOOST_ASSERT(reputation_repository_ != nullptr); // Register metrics registry_->registerGaugeFamily(syncPeerMetricName, @@ -110,11 +110,11 @@ namespace kagome::network { .subscribe([wp = weak_from_this()](const PeerId &peer_id) { if (auto self = wp.lock()) { if (auto rating = - self->peer_rating_repository_->rating(peer_id); + self->reputation_repository_->reputation(peer_id); rating < 0) { SL_DEBUG(self->log_, "Disconnecting from peer {} due to its negative " - "rating {}", + "reputation {}", peer_id, rating); self->disconnectFromPeer(peer_id); @@ -128,12 +128,13 @@ namespace kagome::network { const PeerId &peer_id) { if (auto self = wp.lock()) { SL_DEBUG(self->log_, "Identify received from peer {}", peer_id); - if (auto rating = self->peer_rating_repository_->rating(peer_id); + if (auto rating = self->reputation_repository_->reputation(peer_id); rating < 0) { - SL_DEBUG(self->log_, - "Disconnecting from peer {} due to its negative rating {}", - peer_id, - rating); + SL_DEBUG( + self->log_, + "Disconnecting from peer {} due to its negative reputation {}", + peer_id, + rating); self->disconnectFromPeer(peer_id); return; } @@ -246,10 +247,10 @@ namespace kagome::network { align_timer_.cancel(); - // disconnect from peers with negative rating + // disconnect from peers with negative reputation std::vector peers_to_disconnect; for (const auto &[peer_id, _] : active_peers_) { - if (peer_rating_repository_->rating(peer_id) < 0) { + if (reputation_repository_->reputation(peer_id) < 0) { peers_to_disconnect.emplace_back(peer_id); // we have to store peers somewhere first due to inability to iterate // over active_peers_ and do disconnectFromPeers (which modifies @@ -258,7 +259,7 @@ namespace kagome::network { } for (const auto &peer_id : peers_to_disconnect) { SL_DEBUG(log_, - "Disconnecting from peer {} due to its negative rating", + "Disconnecting from peer {} due to its negative reputation", peer_id); disconnectFromPeer(peer_id); } diff --git a/core/network/impl/peer_manager_impl.hpp b/core/network/impl/peer_manager_impl.hpp index 3fcdf7da26..5b2be0e93f 100644 --- a/core/network/impl/peer_manager_impl.hpp +++ b/core/network/impl/peer_manager_impl.hpp @@ -32,7 +32,7 @@ #include "network/impl/protocols/protocol_factory.hpp" #include "network/impl/stream_engine.hpp" #include "network/protocols/sync_protocol.hpp" -#include "network/rating_repository.hpp" +#include "network/reputation_repository.hpp" #include "network/router.hpp" #include "network/types/block_announce.hpp" #include "network/types/bootstrap_nodes.hpp" @@ -70,7 +70,7 @@ namespace kagome::network { std::shared_ptr router, std::shared_ptr storage, std::shared_ptr hasher, - std::shared_ptr peer_rating_repository); + std::shared_ptr reputation_repository); /** @see AppStateManager::takeControl */ bool prepare(); @@ -172,7 +172,7 @@ namespace kagome::network { std::shared_ptr router_; std::shared_ptr storage_; std::shared_ptr hasher_; - std::shared_ptr peer_rating_repository_; + std::shared_ptr reputation_repository_; libp2p::event::Handle add_peer_handle_; std::unordered_set peers_in_queue_; diff --git a/core/network/impl/protocols/protocol_factory.cpp b/core/network/impl/protocols/protocol_factory.cpp index fde74e8961..e847bcd032 100644 --- a/core/network/impl/protocols/protocol_factory.cpp +++ b/core/network/impl/protocols/protocol_factory.cpp @@ -20,7 +20,7 @@ namespace kagome::network { extrinsic_events_engine, std::shared_ptr ext_event_key_repo, - std::shared_ptr peer_rating_repository, + std::shared_ptr reputation_repository, std::shared_ptr scheduler) : host_(host), app_config_(app_config), @@ -31,14 +31,14 @@ namespace kagome::network { stream_engine_(std::move(stream_engine)), extrinsic_events_engine_{std::move(extrinsic_events_engine)}, ext_event_key_repo_{std::move(ext_event_key_repo)}, - peer_rating_repository_{std::move(peer_rating_repository)}, + reputation_repository_{std::move(reputation_repository)}, scheduler_{std::move(scheduler)} { BOOST_ASSERT(io_context_ != nullptr); BOOST_ASSERT(hasher_ != nullptr); BOOST_ASSERT(stream_engine_ != nullptr); BOOST_ASSERT(extrinsic_events_engine_ != nullptr); BOOST_ASSERT(ext_event_key_repo_ != nullptr); - BOOST_ASSERT(peer_rating_repository_ != nullptr); + BOOST_ASSERT(reputation_repository_ != nullptr); BOOST_ASSERT(scheduler_ != nullptr); } @@ -102,7 +102,7 @@ namespace kagome::network { std::shared_ptr ProtocolFactory::makeSyncProtocol() const { return std::make_shared( - host_, chain_spec_, sync_observer_.lock(), peer_rating_repository_); + host_, chain_spec_, sync_observer_.lock(), reputation_repository_); } } // namespace kagome::network diff --git a/core/network/impl/protocols/protocol_factory.hpp b/core/network/impl/protocols/protocol_factory.hpp index 21542779e5..6ca14f1262 100644 --- a/core/network/impl/protocols/protocol_factory.hpp +++ b/core/network/impl/protocols/protocol_factory.hpp @@ -12,13 +12,12 @@ #include "network/impl/protocols/collation_protocol.hpp" #include "network/impl/protocols/grandpa_protocol.hpp" #include "network/impl/protocols/propagate_transactions_protocol.hpp" -#include "network/impl/protocols/state_protocol_impl.hpp" -#include "network/impl/protocols/request_response_protocol.hpp" #include "network/impl/protocols/protocol_req_collation.hpp" #include "network/impl/protocols/request_response_protocol.hpp" +#include "network/impl/protocols/state_protocol_impl.hpp" #include "network/impl/protocols/sync_protocol_impl.hpp" #include "network/impl/stream_engine.hpp" -#include "network/rating_repository.hpp" +#include "network/reputation_repository.hpp" #include "primitives/event_types.hpp" #include @@ -39,7 +38,7 @@ namespace kagome::network { extrinsic_events_engine, std::shared_ptr ext_event_key_repo, - std::shared_ptr peer_rating_repository, + std::shared_ptr reputation_repository, std::shared_ptr scheduler); void setBlockTree( @@ -111,7 +110,7 @@ namespace kagome::network { extrinsic_events_engine_; std::shared_ptr ext_event_key_repo_; - std::shared_ptr peer_rating_repository_; + std::shared_ptr reputation_repository_; std::shared_ptr scheduler_; std::weak_ptr block_tree_; diff --git a/core/network/impl/protocols/protocol_req_collation.cpp b/core/network/impl/protocols/protocol_req_collation.cpp index b64f333cc8..a1d4d33ed3 100644 --- a/core/network/impl/protocols/protocol_req_collation.cpp +++ b/core/network/impl/protocols/protocol_req_collation.cpp @@ -27,8 +27,7 @@ namespace kagome::network { ScaleMessageReadWriter>{host, kReqCollationProtocol, "ReqCollationProtocol"}, - observer_{std::move(observer)}, - app_config_{app_config} {} + observer_{std::move(observer)} {} protected: outcome::result onRxRequest( @@ -46,7 +45,6 @@ namespace kagome::network { private: std::shared_ptr observer_; - application::AppConfiguration const &app_config_; }; ReqCollationProtocol::ReqCollationProtocol( diff --git a/core/network/impl/protocols/sync_protocol_impl.cpp b/core/network/impl/protocols/sync_protocol_impl.cpp index 3b1bbfd46b..a27873bf4d 100644 --- a/core/network/impl/protocols/sync_protocol_impl.cpp +++ b/core/network/impl/protocols/sync_protocol_impl.cpp @@ -128,16 +128,16 @@ namespace kagome::network { libp2p::Host &host, const application::ChainSpec &chain_spec, std::shared_ptr sync_observer, - std::shared_ptr rating_repository) + std::shared_ptr reputation_repository) : base_(host, {fmt::format(kSyncProtocol.data(), chain_spec.protocolId())}, "SyncProtocol"), sync_observer_(std::move(sync_observer)), - rating_repository_(std::move(rating_repository)), + reputation_repository_(std::move(reputation_repository)), response_cache_(kResponsesCacheCapacity, kResponsesCacheExpirationTimeout) { BOOST_ASSERT(sync_observer_ != nullptr); - BOOST_ASSERT(rating_repository_ != nullptr); + BOOST_ASSERT(reputation_repository_ != nullptr); } bool SyncProtocolImpl::start() { @@ -279,8 +279,10 @@ namespace kagome::network { self->protocolName(), peer_id, block_request.fingerprint()); - self->rating_repository_->downvoteForATime( - peer_id, kResponsesCacheExpirationTimeout); + self->reputation_repository_->changeForATime( + peer_id, + reputation::cost::DUPLICATE_BLOCK_REQUEST, + kResponsesCacheExpirationTimeout); stream->reset(); return; } diff --git a/core/network/impl/protocols/sync_protocol_impl.hpp b/core/network/impl/protocols/sync_protocol_impl.hpp index a312cc47c6..2be1e4c62c 100644 --- a/core/network/impl/protocols/sync_protocol_impl.hpp +++ b/core/network/impl/protocols/sync_protocol_impl.hpp @@ -20,7 +20,7 @@ #include "application/chain_spec.hpp" #include "log/logger.hpp" #include "network/impl/protocols/protocol_base_impl.hpp" -#include "network/rating_repository.hpp" +#include "network/reputation_repository.hpp" #include "network/sync_protocol_observer.hpp" #include "utils/non_copyable.hpp" @@ -104,10 +104,11 @@ namespace kagome::network { NonCopyable, NonMovable { public: - SyncProtocolImpl(libp2p::Host &host, - const application::ChainSpec &chain_spec, - std::shared_ptr sync_observer, - std::shared_ptr rating_repository); + SyncProtocolImpl( + libp2p::Host &host, + const application::ChainSpec &chain_spec, + std::shared_ptr sync_observer, + std::shared_ptr reputation_repository); bool start() override; bool stop() override; @@ -144,7 +145,7 @@ namespace kagome::network { const static inline auto kSyncProtocolName = "SyncProtocol"s; ProtocolBaseImpl base_; std::shared_ptr sync_observer_; - std::shared_ptr rating_repository_; + std::shared_ptr reputation_repository_; detail::BlocksResponseCache response_cache_; }; diff --git a/core/network/impl/rating_repository_impl.cpp b/core/network/impl/rating_repository_impl.cpp deleted file mode 100644 index 032646a13b..0000000000 --- a/core/network/impl/rating_repository_impl.cpp +++ /dev/null @@ -1,87 +0,0 @@ -/** - * Copyright Soramitsu Co., Ltd. All Rights Reserved. - * SPDX-License-Identifier: Apache-2.0 - */ - -#include "network/impl/rating_repository_impl.hpp" - -#include - -namespace kagome::network { - - PeerRatingRepositoryImpl::PeerRatingRepositoryImpl( - std::shared_ptr scheduler) - : scheduler_{std::move(scheduler)} { - BOOST_ASSERT(scheduler_); - } - - PeerScore PeerRatingRepositoryImpl::rating( - const PeerRatingRepository::PeerId &peer_id) const { - return rating_table_.count(peer_id) == 0 ? 0 : rating_table_.at(peer_id); - } - - PeerScore PeerRatingRepositoryImpl::upvote( - const PeerRatingRepository::PeerId &peer_id) { - ensurePeerPresence(peer_id); - return ++rating_table_[peer_id]; - } - - PeerScore PeerRatingRepositoryImpl::downvote( - const PeerRatingRepository::PeerId &peer_id) { - ensurePeerPresence(peer_id); - return --rating_table_[peer_id]; - } - - PeerScore PeerRatingRepositoryImpl::update( - const PeerRatingRepository::PeerId &peer_id, PeerScore diff) { - ensurePeerPresence(peer_id); - return rating_table_[peer_id] += diff; - } - - void PeerRatingRepositoryImpl::ensurePeerPresence(const PeerId &peer_id) { - rating_table_.try_emplace(peer_id, 0); - } - - PeerScore PeerRatingRepositoryImpl::upvoteForATime( - const PeerRatingRepository::PeerId &peer_id, - std::chrono::seconds duration) { - auto score = upvote(peer_id); - scheduler_->schedule( - [wp{weak_from_this()}, peer_id] { - if (auto self = wp.lock()) { - self->downvote(peer_id); - } - }, - duration); - return score; - } - - PeerScore PeerRatingRepositoryImpl::downvoteForATime( - const PeerRatingRepository::PeerId &peer_id, - std::chrono::seconds duration) { - auto score = downvote(peer_id); - scheduler_->schedule( - [wp{weak_from_this()}, peer_id] { - if (auto self = wp.lock()) { - self->upvote(peer_id); - } - }, - duration); - return score; - } - - PeerScore PeerRatingRepositoryImpl::updateForATime( - const PeerRatingRepository::PeerId &peer_id, - PeerScore diff, - std::chrono::seconds duration) { - auto score = update(peer_id, diff); - scheduler_->schedule( - [wp{weak_from_this()}, peer_id, diff] { - if (auto self = wp.lock()) { - self->update(peer_id, -1 * diff); - } - }, - duration); - return score; - } -} // namespace kagome::network diff --git a/core/network/impl/rating_repository_impl.hpp b/core/network/impl/rating_repository_impl.hpp deleted file mode 100644 index 6c47b0550a..0000000000 --- a/core/network/impl/rating_repository_impl.hpp +++ /dev/null @@ -1,52 +0,0 @@ -/** - * Copyright Soramitsu Co., Ltd. All Rights Reserved. - * SPDX-License-Identifier: Apache-2.0 - */ - -#ifndef KAGOME_RATING_REPOSITORY_IMPL_HPP -#define KAGOME_RATING_REPOSITORY_IMPL_HPP - -#include "network/rating_repository.hpp" - -#include -#include - -#include - -namespace kagome::network { - - class PeerRatingRepositoryImpl - : public PeerRatingRepository, - public std::enable_shared_from_this { - public: - PeerRatingRepositoryImpl( - std::shared_ptr scheduler); - - PeerScore rating(const PeerId &peer_id) const override; - - PeerScore upvote(const PeerId &peer_id) override; - - PeerScore downvote(const PeerId &peer_id) override; - - PeerScore update(const PeerId &peer_id, PeerScore diff) override; - - PeerScore upvoteForATime(const PeerId &peer_id, - std::chrono::seconds duration) override; - - PeerScore downvoteForATime(const PeerId &peer_id, - std::chrono::seconds duration) override; - - PeerScore updateForATime(const PeerId &peer_id, - PeerScore diff, - std::chrono::seconds duration) override; - - private: - void ensurePeerPresence(const PeerId &peer_id); - - std::shared_ptr scheduler_; - std::unordered_map rating_table_; - }; - -} // namespace kagome::network - -#endif // KAGOME_RATING_REPOSITORY_IMPL_HPP diff --git a/core/network/impl/reputation_repository_impl.cpp b/core/network/impl/reputation_repository_impl.cpp new file mode 100644 index 0000000000..3e632d8b0d --- /dev/null +++ b/core/network/impl/reputation_repository_impl.cpp @@ -0,0 +1,77 @@ +/** + * Copyright Soramitsu Co., Ltd. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#include "network/impl/reputation_repository_impl.hpp" + +#include + +using namespace std::chrono_literals; + +namespace kagome::network { + + ReputationRepositoryImpl::ReputationRepositoryImpl( + std::shared_ptr scheduler) + : scheduler_{std::move(scheduler)} { + BOOST_ASSERT(scheduler_); + + tick_handler_ = scheduler_->scheduleWithHandle([&] { tick(); }, 1s); + } + + Reputation ReputationRepositoryImpl::reputation( + const ReputationRepository::PeerId &peer_id) const { + return rating_table_.count(peer_id) == 0 ? 0 : rating_table_.at(peer_id); + } + + Reputation ReputationRepositoryImpl::change( + const ReputationRepository::PeerId &peer_id, ReputationChange diff) { + ensurePeerPresence(peer_id); + return rating_table_[peer_id] += diff.value; + } + + void ReputationRepositoryImpl::ensurePeerPresence(const PeerId &peer_id) { + rating_table_.try_emplace(peer_id, 0); + } + + Reputation ReputationRepositoryImpl::changeForATime( + const ReputationRepository::PeerId &peer_id, + ReputationChange diff, + std::chrono::seconds duration) { + auto score = change(peer_id, diff); + scheduler_->schedule( + [wp{weak_from_this()}, peer_id, diff] { + if (auto self = wp.lock()) { + self->change(peer_id, {-diff.value, "Cancel by timeout"}); + } + }, + duration); + return score; + } + + void ReputationRepositoryImpl::tick() { + // For each elapsed second, move the node reputation towards zero. + // If we multiply each second the reputation by `k` (where `k` is 0..1), it + // takes `ln(0.5) / ln(k)` seconds to reduce the reputation by half. Use + // this formula to empirically determine a value of `k` that looks correct. + for (auto it = rating_table_.begin(); it != rating_table_.end();) { + auto cit = it++; + auto &[peer_id, reputation] = *cit; + + // We use `k = 0.98`, so we divide by `50`. With that value, it takes 34.3 + // seconds to reduce the reputation by half. + auto diff = reputation / 50; + + if (diff == 0) { + diff = reputation > 0 ? -1 : 1; + } + reputation -= diff; + + if (reputation == 0) { + rating_table_.erase(it); + } + } + tick_handler_.reschedule(1s); + } + +} // namespace kagome::network diff --git a/core/network/impl/reputation_repository_impl.hpp b/core/network/impl/reputation_repository_impl.hpp new file mode 100644 index 0000000000..4b24b540ad --- /dev/null +++ b/core/network/impl/reputation_repository_impl.hpp @@ -0,0 +1,46 @@ +/** + * Copyright Soramitsu Co., Ltd. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#ifndef KAGOME_NETWORK_REPUTATIONREPOSITORYIMPL +#define KAGOME_NETWORK_REPUTATIONREPOSITORYIMPL + +#include "network/reputation_repository.hpp" + +#include +#include + +#include + +namespace kagome::network { + + class ReputationRepositoryImpl + : public ReputationRepository, + public std::enable_shared_from_this { + public: + ReputationRepositoryImpl( + std::shared_ptr scheduler); + + Reputation reputation(const PeerId &peer_id) const override; + + Reputation change(const PeerId &peer_id, ReputationChange diff) override; + + Reputation changeForATime(const PeerId &peer_id, + ReputationChange diff, + std::chrono::seconds duration) override; + + private: + void ensurePeerPresence(const PeerId &peer_id); + + void tick(); + + std::shared_ptr scheduler_; + std::unordered_map rating_table_; + + libp2p::basic::Scheduler::Handle tick_handler_; + }; + +} // namespace kagome::network + +#endif // KAGOME_NETWORK_REPUTATIONREPOSITORYIMPL diff --git a/core/network/impl/stream_engine.hpp b/core/network/impl/stream_engine.hpp index d01929c4d1..dfc6d335c1 100644 --- a/core/network/impl/stream_engine.hpp +++ b/core/network/impl/stream_engine.hpp @@ -21,7 +21,7 @@ #include "network/helpers/peer_id_formatter.hpp" #include "network/helpers/scale_message_read_writer.hpp" #include "network/protocol_base.hpp" -#include "network/rating_repository.hpp" +#include "network/reputation_repository.hpp" #include "subscription/subscriber.hpp" #include "subscription/subscription_engine.hpp" #include "utils/safe_object.hpp" @@ -151,8 +151,8 @@ namespace kagome::network { StreamEngine &operator=(StreamEngine &&) = delete; ~StreamEngine() = default; - StreamEngine(std::shared_ptr peer_rating_repository) - : peer_rating_repository_(std::move(peer_rating_repository)), + StreamEngine(std::shared_ptr reputation_repository) + : reputation_repository_(std::move(reputation_repository)), logger_{log::createLogger("StreamEngine", "network")} {} template @@ -526,9 +526,9 @@ namespace kagome::network { if (stream_res == outcome::failure( std::make_error_code(std::errc::not_connected))) { - self->peer_rating_repository_->updateForATime( + self->reputation_repository_->changeForATime( peer_id, - -1000, + reputation::cost::UNEXPECTED_DISCONNECT, kDownVoteByDisconnectionExpirationTimeout); return; } @@ -586,7 +586,7 @@ namespace kagome::network { }); } - std::shared_ptr peer_rating_repository_; + std::shared_ptr reputation_repository_; log::Logger logger_; SafeObject streams_; diff --git a/core/network/rating_repository.hpp b/core/network/rating_repository.hpp deleted file mode 100644 index 131004883b..0000000000 --- a/core/network/rating_repository.hpp +++ /dev/null @@ -1,78 +0,0 @@ -/** - * Copyright Soramitsu Co., Ltd. All Rights Reserved. - * SPDX-License-Identifier: Apache-2.0 - */ - -#ifndef KAGOME_RATING_REPOSITORY_HPP -#define KAGOME_RATING_REPOSITORY_HPP - -#include - -#include - -namespace kagome::network { - - using PeerScore = std::int32_t; - - /** - * Storage to handle peers' rating - */ - class PeerRatingRepository { - public: - using PeerId = libp2p::peer::PeerId; - - virtual ~PeerRatingRepository() = default; - - /// Current peer rating - virtual PeerScore rating(const PeerId &peer_id) const = 0; - - /// Raise peer rating by one, returns resulting rating - virtual PeerScore upvote(const PeerId &peer_id) = 0; - - /** - * Raise peer rating by one for a specified amount of time. - * When time is over, the rating decreases automatically by one. - * @param peer_id - peer identifier - * @param duration - amount of time to increase peer rating for - * @return - resulting peer rating - */ - virtual PeerScore upvoteForATime(const PeerId &peer_id, - std::chrono::seconds duration) = 0; - - /// Decrease peer rating by one, returns resulting rating - virtual PeerScore downvote(const PeerId &peer_id) = 0; - - /** - * Decrease peer rating by one for a specified amount of time. - * When time is over, the rating increases automatically by one. - * @param peer_id - peer identifier - * @param duration - amount of time to decrease peer rating for - * @return - resulting peer rating - */ - virtual PeerScore downvoteForATime(const PeerId &peer_id, - std::chrono::seconds duration) = 0; - - /** - * Change peer rating by arbitrary amount of points - * @param peer_id - peer identifier - * @param diff - rating increment or decrement - * @return - resulting peer rating - */ - virtual PeerScore update(const PeerId &peer_id, PeerScore diff) = 0; - - /** - * Change peer rating by arbitrary amount of points for a specified amount - * of time - * @param peer_id - peer identifier - * @param difff - rating increment or decrement - * @param duration - amount of time to change peer rating for - * @return - resulting peer rating - */ - virtual PeerScore updateForATime(const PeerId &peer_id, - PeerScore diff, - std::chrono::seconds duration) = 0; - }; - -} // namespace kagome::network - -#endif // KAGOME_RATING_REPOSITORY_HPP diff --git a/core/network/reputation_change.hpp b/core/network/reputation_change.hpp new file mode 100644 index 0000000000..4afc92b717 --- /dev/null +++ b/core/network/reputation_change.hpp @@ -0,0 +1,106 @@ +/** + * Copyright Soramitsu Co., Ltd. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#ifndef KAGOME_NETWORK_REPUTATIONCHANGE +#define KAGOME_NETWORK_REPUTATIONCHANGE + +#include + +#include + +namespace kagome::network { + + using Reputation = std::int32_t; + + struct ReputationChange { + public: + const Reputation value; + const std::string_view reason; + + ReputationChange operator+(Reputation delta) const { + if (delta < 0) { + if (value < 0 and value + delta > 0) { // negative overflow + return {std::numeric_limits::min(), reason}; + } + } else if (delta > 0) { + if (value > 0 and value + delta < 0) { // positive overflow + return {std::numeric_limits::max(), reason}; + } + } + return {value + delta, reason}; + } + + ReputationChange operator*(size_t times) const { + auto limit = value < 0 ? std::numeric_limits::min() + : std::numeric_limits::max(); + if (static_cast(limit / value) < times) { // overflow + return {limit, reason}; + } + return {static_cast(value * times), reason}; + } + }; + + namespace reputation { + + // clang-format off + namespace cost { + const ReputationChange UNEXPECTED_DISCONNECT = {-1000, "Network: Unexpected disconnect"}; + const ReputationChange DUPLICATE_BLOCK_REQUEST = {-50, "Sync: Duplicate block request"}; + + const ReputationChange PAST_REJECTION = {-50, "Grandpa: Past message"}; + + const ReputationChange BAD_SIGNATURE = {-100, "Grandpa: Bad signature"}; + const ReputationChange MALFORMED_CATCH_UP = {-1000, "Grandpa: Malformed catch-up"}; + const ReputationChange MALFORMED_COMMIT = {-1000, "Grandpa: Malformed commit"}; + + // A message received that's from the future relative to our view. always misbehavior. + const ReputationChange FUTURE_MESSAGE = {-500, "Grandpa: Future message"}; + + const ReputationChange UNKNOWN_VOTER = {-150, "Grandpa: Unknown voter"}; + + // invalid neighbor message, considering the last one + const ReputationChange INVALID_VIEW_CHANGE = {-500, "Grandpa: Invalid view change"}; + + // could not decode neighbor message. bytes-length of the packet. + // TODO need to catch scale-decoder error for that + const ReputationChange UNDECODABLE_NEIGHBOR_MESSAGE = {-5, "Grandpa: Bad packet"}; + + const ReputationChange PER_UNDECODABLE_BYTE = {-5, ""}; + + // bad signature in catch-up response (for each checked signature) + const ReputationChange BAD_CATCHUP_RESPONSE = {-25, "Grandpa: Bad catch-up message"}; + + const ReputationChange PER_SIGNATURE_CHECKED = {-25, ""}; + + const ReputationChange PER_BLOCK_LOADED = {-10, ""}; + const ReputationChange INVALID_CATCH_UP = {-5000, "Grandpa: Invalid catch-up"}; + const ReputationChange INVALID_COMMIT = {-5000, "Grandpa: Invalid commit"}; + const ReputationChange OUT_OF_SCOPE_MESSAGE = {-500, "Grandpa: Out-of-scope message"}; + const ReputationChange CATCH_UP_REQUEST_TIMEOUT = {-200, "Grandpa: Catch-up request timeout"}; + + // cost of answering a catch-up request + const ReputationChange CATCH_UP_REPLY = {-200, "Grandpa: Catch-up reply"}; + + // A message received that cannot be evaluated relative to our view. + // This happens before we have a view and have sent out neighbor packets. + // always misbehavior. + const ReputationChange HONEST_OUT_OF_SCOPE_CATCH_UP = {-200, "Grandpa: Out-of-scope catch-up"}; + + } // namespace cost + + namespace benefit { + const ReputationChange NEIGHBOR_MESSAGE = {100, "Grandpa: Neighbor message"}; + const ReputationChange ROUND_MESSAGE = {100, "Grandpa: Round message"}; + const ReputationChange BASIC_VALIDATED_CATCH_UP = {200, "Grandpa: Catch-up message"}; + const ReputationChange BASIC_VALIDATED_COMMIT = {100, "Grandpa: Commit"}; + const ReputationChange PER_EQUIVOCATION = {10, ""}; + } // namespace benefit + // clang-format on + + } // namespace reputation + +} // namespace kagome::network + +#endif // KAGOME_NETWORK_REPUTATIONCHANGE diff --git a/core/network/reputation_repository.hpp b/core/network/reputation_repository.hpp new file mode 100644 index 0000000000..9c3ad4e1d5 --- /dev/null +++ b/core/network/reputation_repository.hpp @@ -0,0 +1,54 @@ +/** + * Copyright Soramitsu Co., Ltd. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#ifndef KAGOME_NETWORK_REPUTATIONREPOSITORY +#define KAGOME_NETWORK_REPUTATIONREPOSITORY + +#include + +#include + +#include "network/reputation_change.hpp" + +namespace kagome::network { + + using Reputation = std::int32_t; + + /** + * Storage to handle peers' reputation + */ + class ReputationRepository { + public: + using PeerId = libp2p::peer::PeerId; + + virtual ~ReputationRepository() = default; + + /// Current peer reputation + virtual Reputation reputation(const PeerId &peer_id) const = 0; + + /** + * Change peer reputation by arbitrary amount of points + * @param peer_id - peer identifier + * @param diff - reputation increment or decrement + * @return - resulting peer reputation + */ + virtual Reputation change(const PeerId &peer_id, ReputationChange diff) = 0; + + /** + * Change peer reputation by arbitrary amount of points for a specified + * amount of time + * @param peer_id - peer identifier + * @param difff - reputation increment or decrement + * @param duration - amount of time to change peer reputation for + * @return - resulting peer reputation + */ + virtual Reputation changeForATime(const PeerId &peer_id, + ReputationChange diff, + std::chrono::seconds duration) = 0; + }; + +} // namespace kagome::network + +#endif // KAGOME_NETWORK_REPUTATIONREPOSITORY diff --git a/test/core/network/stream_engine_test.cpp b/test/core/network/stream_engine_test.cpp index c06fade03a..a522a32a13 100644 --- a/test/core/network/stream_engine_test.cpp +++ b/test/core/network/stream_engine_test.cpp @@ -10,13 +10,13 @@ #include "mock/core/network/protocols/state_protocol_mock.hpp" #include "mock/core/network/protocols/sync_protocol_mock.hpp" -#include "mock/core/network/rating_repository_mock.hpp" +#include "mock/core/network/reputation_repository_mock.hpp" #include "mock/libp2p/connection/stream_mock.hpp" #include "testutil/literals.hpp" #include "testutil/outcome.hpp" #include "testutil/prepare_loggers.hpp" -using kagome::network::PeerRatingRepositoryMock; +using kagome::network::ReputationRepositoryMock; using kagome::network::StreamEngine; using libp2p::connection::StreamMock; using libp2p::peer::PeerId; @@ -35,12 +35,12 @@ namespace kagome::network { } void SetUp() override { - peer_rating_repository = std::make_shared(); - stream_engine = std::make_shared(peer_rating_repository); + reputation_repository = std::make_shared(); + stream_engine = std::make_shared(reputation_repository); } std::shared_ptr stream_engine; - std::shared_ptr peer_rating_repository; + std::shared_ptr reputation_repository; }; static constexpr int lucky_peers = 4; diff --git a/test/mock/core/network/rating_repository_mock.hpp b/test/mock/core/network/rating_repository_mock.hpp deleted file mode 100644 index 54afc8bf25..0000000000 --- a/test/mock/core/network/rating_repository_mock.hpp +++ /dev/null @@ -1,43 +0,0 @@ -/** - * Copyright Soramitsu Co., Ltd. All Rights Reserved. - * SPDX-License-Identifier: Apache-2.0 - */ - -#ifndef KAGOME_RATING_REPOSITORY_MOCK_HPP -#define KAGOME_RATING_REPOSITORY_MOCK_HPP - -#include - -#include "network/rating_repository.hpp" - -namespace kagome::network { - - class PeerRatingRepositoryMock : public PeerRatingRepository { - public: - MOCK_METHOD(PeerScore, rating, (const PeerId &), (const override)); - - MOCK_METHOD(PeerScore, upvote, (const PeerId &), (override)); - - MOCK_METHOD(PeerScore, downvote, (const PeerId &), (override)); - - MOCK_METHOD(PeerScore, update, (const PeerId &, PeerScore), (override)); - - MOCK_METHOD(PeerScore, - upvoteForATime, - (const PeerId &, std::chrono::seconds), - (override)); - - MOCK_METHOD(PeerScore, - downvoteForATime, - (const PeerId &, std::chrono::seconds), - (override)); - - MOCK_METHOD(PeerScore, - updateForATime, - (const PeerId &, PeerScore, std::chrono::seconds), - (override)); - }; - -} // namespace kagome::network - -#endif // KAGOME_RATING_REPOSITORY_MOCK_HPP diff --git a/test/mock/core/network/reputation_repository_mock.hpp b/test/mock/core/network/reputation_repository_mock.hpp new file mode 100644 index 0000000000..58771d5ba4 --- /dev/null +++ b/test/mock/core/network/reputation_repository_mock.hpp @@ -0,0 +1,32 @@ +/** + * Copyright Soramitsu Co., Ltd. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#ifndef KAGOME_NETWORK_REPUTATIONREPOSITORYMOCK +#define KAGOME_NETWORK_REPUTATIONREPOSITORYMOCK + +#include + +#include "network/reputation_repository.hpp" + +namespace kagome::network { + + class ReputationRepositoryMock : public ReputationRepository { + public: + MOCK_METHOD(Reputation, reputation, (const PeerId &), (const override)); + + MOCK_METHOD(Reputation, + change, + (const PeerId &, ReputationChange), + (override)); + + MOCK_METHOD(Reputation, + changeForATime, + (const PeerId &, ReputationChange, std::chrono::seconds), + (override)); + }; + +} // namespace kagome::network + +#endif // KAGOME_NETWORK_REPUTATIONREPOSITORYMOCK From 173cc30ea93194978331369e4ed169695530a970 Mon Sep 17 00:00:00 2001 From: Dmitriy Khaustov aka xDimon Date: Thu, 29 Sep 2022 22:32:18 +0800 Subject: [PATCH 2/7] fix: review issues Signed-off-by: Dmitriy Khaustov aka xDimon --- core/consensus/grandpa/impl/grandpa_impl.cpp | 11 ++- core/consensus/grandpa/impl/grandpa_impl.hpp | 3 - core/log/configurator.cpp | 1 + core/network/impl/peer_manager_impl.cpp | 6 +- core/network/impl/peer_manager_impl.hpp | 2 +- .../impl/reputation_repository_impl.cpp | 74 ++++++++++++++----- .../impl/reputation_repository_impl.hpp | 6 +- 7 files changed, 75 insertions(+), 28 deletions(-) diff --git a/core/consensus/grandpa/impl/grandpa_impl.cpp b/core/consensus/grandpa/impl/grandpa_impl.cpp index 6337f55e10..0247bfe349 100644 --- a/core/consensus/grandpa/impl/grandpa_impl.cpp +++ b/core/consensus/grandpa/impl/grandpa_impl.cpp @@ -601,6 +601,7 @@ namespace kagome::consensus::grandpa { void GrandpaImpl::onCatchUpResponse(const libp2p::peer::PeerId &peer_id, const network::CatchUpResponse &msg) { + bool need_cleanup_when_exiting_scope = false; GrandpaContext::Guard cg; auto ctx = GrandpaContext::get().value(); @@ -634,11 +635,15 @@ namespace kagome::consensus::grandpa { return; } - auto cleanup = gsl::finally([this] { + need_cleanup_when_exiting_scope = true; + } + + auto cleanup = gsl::finally([&] { + if (need_cleanup_when_exiting_scope) { catchup_request_timer_handle_.cancel(); pending_catchup_request_.reset(); - }); - } + } + }); BOOST_ASSERT(current_round_ != nullptr); // Ignore message of peer whose round in different voter set diff --git a/core/consensus/grandpa/impl/grandpa_impl.hpp b/core/consensus/grandpa/impl/grandpa_impl.hpp index bfe05204ca..13ab0d7ecb 100644 --- a/core/consensus/grandpa/impl/grandpa_impl.hpp +++ b/core/consensus/grandpa/impl/grandpa_impl.hpp @@ -259,9 +259,6 @@ namespace kagome::consensus::grandpa { */ void loadMissingBlocks(); - // Note: Duration value was gotten from substrate - // https://github.com/paritytech/substrate/blob/efbac7be80c6e8988a25339061078d3e300f132d/bin/node-template/node/src/service.rs#L166 - // Perhaps, 333ms is not enough for normal communication during the round const Clock::Duration round_time_factor_; std::shared_ptr environment_; diff --git a/core/log/configurator.cpp b/core/log/configurator.cpp index f1fa8a52cf..ae5611c7b8 100644 --- a/core/log/configurator.cpp +++ b/core/log/configurator.cpp @@ -74,6 +74,7 @@ namespace kagome::log { - name: telemetry - name: network children: + - name: reputation - name: synchronizer - name: kagome_protocols children: diff --git a/core/network/impl/peer_manager_impl.cpp b/core/network/impl/peer_manager_impl.cpp index db832cb99a..1cb5aa75c8 100644 --- a/core/network/impl/peer_manager_impl.cpp +++ b/core/network/impl/peer_manager_impl.cpp @@ -24,8 +24,8 @@ OUTCOME_CPP_DEFINE_CATEGORY(kagome::network, PeerManagerImpl::Error, e) { return "Process handling from undeclared collator"; case E::OUT_OF_VIEW: return "Processing para hash, which is out of view"; - case E::DOUPLICATE: - return "Processing doublicated hash"; + case E::DUPLICATE: + return "Processing duplicated hash"; } return "Unknown error in ChainSpecImpl"; } @@ -229,7 +229,7 @@ namespace kagome::network { return Error::OUT_OF_VIEW; if (peer_state.collator_state.value().advertisements.count(para_hash) != 0) - return Error::DOUPLICATE; + return Error::DUPLICATE; peer_state.collator_state.value().advertisements.insert( std::move(para_hash)); diff --git a/core/network/impl/peer_manager_impl.hpp b/core/network/impl/peer_manager_impl.hpp index 5b2be0e93f..82e30842f4 100644 --- a/core/network/impl/peer_manager_impl.hpp +++ b/core/network/impl/peer_manager_impl.hpp @@ -54,7 +54,7 @@ namespace kagome::network { public: static constexpr std::chrono::seconds kTimeoutForConnecting{15}; - enum class Error { UNDECLARED_COLLATOR = 1, OUT_OF_VIEW, DOUPLICATE }; + enum class Error { UNDECLARED_COLLATOR = 1, OUT_OF_VIEW, DUPLICATE }; PeerManagerImpl( std::shared_ptr app_state_manager, diff --git a/core/network/impl/reputation_repository_impl.cpp b/core/network/impl/reputation_repository_impl.cpp index 3e632d8b0d..2017ca1403 100644 --- a/core/network/impl/reputation_repository_impl.cpp +++ b/core/network/impl/reputation_repository_impl.cpp @@ -7,13 +7,16 @@ #include +#include "network/helpers/peer_id_formatter.hpp" + using namespace std::chrono_literals; namespace kagome::network { ReputationRepositoryImpl::ReputationRepositoryImpl( std::shared_ptr scheduler) - : scheduler_{std::move(scheduler)} { + : scheduler_{std::move(scheduler)}, + log_(log::createLogger("Reputation", "reputation")) { BOOST_ASSERT(scheduler_); tick_handler_ = scheduler_->scheduleWithHandle([&] { tick(); }, 1s); @@ -21,32 +24,61 @@ namespace kagome::network { Reputation ReputationRepositoryImpl::reputation( const ReputationRepository::PeerId &peer_id) const { - return rating_table_.count(peer_id) == 0 ? 0 : rating_table_.at(peer_id); + return reputation_table_.count(peer_id) == 0 + ? 0 + : reputation_table_.at(peer_id); } Reputation ReputationRepositoryImpl::change( const ReputationRepository::PeerId &peer_id, ReputationChange diff) { ensurePeerPresence(peer_id); - return rating_table_[peer_id] += diff.value; + auto reputation = reputation_table_[peer_id] += diff.value; + SL_DEBUG(log_, + "Reputation of peer {} was changed by {} points to {} points. " + "Reason: `{}'", + peer_id, + diff.value, + reputation, + diff.reason); + return reputation; } void ReputationRepositoryImpl::ensurePeerPresence(const PeerId &peer_id) { - rating_table_.try_emplace(peer_id, 0); + reputation_table_.try_emplace(peer_id, 0); } Reputation ReputationRepositoryImpl::changeForATime( const ReputationRepository::PeerId &peer_id, ReputationChange diff, std::chrono::seconds duration) { - auto score = change(peer_id, diff); - scheduler_->schedule( - [wp{weak_from_this()}, peer_id, diff] { - if (auto self = wp.lock()) { - self->change(peer_id, {-diff.value, "Cancel by timeout"}); - } - }, - duration); - return score; + ensurePeerPresence(peer_id); + auto reputation = reputation_table_[peer_id] += diff.value; + SL_DEBUG(log_, + "Reputation of peer {} was changed by {} points to {} points " + "for {} seconds. Reason: `{}'", + peer_id, + diff.value, + reputation, + duration.count(), + diff.reason); + + auto value = static_cast( + (std::signbit(diff.value) ? 1 : -1) // raise or reduce + * static_cast(std::abs(diff.value)) // original + * 0.98 * duration.count() // multiplier considering timeout + ); + + if (value != 0) { + scheduler_->schedule( + [wp = weak_from_this(), peer_id, value] { + if (auto self = wp.lock()) { + self->change(peer_id, {value, "Revert by timeout"}); + } + }, + duration); + } + + return reputation; } void ReputationRepositoryImpl::tick() { @@ -54,9 +86,10 @@ namespace kagome::network { // If we multiply each second the reputation by `k` (where `k` is 0..1), it // takes `ln(0.5) / ln(k)` seconds to reduce the reputation by half. Use // this formula to empirically determine a value of `k` that looks correct. - for (auto it = rating_table_.begin(); it != rating_table_.end();) { + for (auto it = reputation_table_.begin(); it != reputation_table_.end();) { auto cit = it++; - auto &[peer_id, reputation] = *cit; + auto &peer_id = cit->first; + auto &reputation = cit->second; // We use `k = 0.98`, so we divide by `50`. With that value, it takes 34.3 // seconds to reduce the reputation by half. @@ -67,11 +100,18 @@ namespace kagome::network { } reputation -= diff; + SL_TRACE( + log_, + "Reputation of peer {} was changed by {} points to {} points by tick", + peer_id, + -diff, + reputation); + if (reputation == 0) { - rating_table_.erase(it); + reputation_table_.erase(cit); } } - tick_handler_.reschedule(1s); + std::ignore = tick_handler_.reschedule(1s); } } // namespace kagome::network diff --git a/core/network/impl/reputation_repository_impl.hpp b/core/network/impl/reputation_repository_impl.hpp index 4b24b540ad..f845d0330c 100644 --- a/core/network/impl/reputation_repository_impl.hpp +++ b/core/network/impl/reputation_repository_impl.hpp @@ -13,6 +13,8 @@ #include +#include "log/logger.hpp" + namespace kagome::network { class ReputationRepositoryImpl @@ -36,9 +38,11 @@ namespace kagome::network { void tick(); std::shared_ptr scheduler_; - std::unordered_map rating_table_; + std::unordered_map reputation_table_; libp2p::basic::Scheduler::Handle tick_handler_; + + log::Logger log_; }; } // namespace kagome::network From 8af65a6bc0f17a43bd190ee9fe78748499cf4867 Mon Sep 17 00:00:00 2001 From: Dmitriy Khaustov aka xDimon Date: Thu, 29 Sep 2022 23:00:26 +0800 Subject: [PATCH 3/7] fix: review issues Signed-off-by: Dmitriy Khaustov aka xDimon --- core/network/impl/reputation_repository_impl.cpp | 11 ++--------- core/network/impl/reputation_repository_impl.hpp | 2 -- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/core/network/impl/reputation_repository_impl.cpp b/core/network/impl/reputation_repository_impl.cpp index 2017ca1403..7020037503 100644 --- a/core/network/impl/reputation_repository_impl.cpp +++ b/core/network/impl/reputation_repository_impl.cpp @@ -24,14 +24,12 @@ namespace kagome::network { Reputation ReputationRepositoryImpl::reputation( const ReputationRepository::PeerId &peer_id) const { - return reputation_table_.count(peer_id) == 0 - ? 0 - : reputation_table_.at(peer_id); + auto it = reputation_table_.find(peer_id); + return it != reputation_table_.end() ? it->second : 0; } Reputation ReputationRepositoryImpl::change( const ReputationRepository::PeerId &peer_id, ReputationChange diff) { - ensurePeerPresence(peer_id); auto reputation = reputation_table_[peer_id] += diff.value; SL_DEBUG(log_, "Reputation of peer {} was changed by {} points to {} points. " @@ -43,15 +41,10 @@ namespace kagome::network { return reputation; } - void ReputationRepositoryImpl::ensurePeerPresence(const PeerId &peer_id) { - reputation_table_.try_emplace(peer_id, 0); - } - Reputation ReputationRepositoryImpl::changeForATime( const ReputationRepository::PeerId &peer_id, ReputationChange diff, std::chrono::seconds duration) { - ensurePeerPresence(peer_id); auto reputation = reputation_table_[peer_id] += diff.value; SL_DEBUG(log_, "Reputation of peer {} was changed by {} points to {} points " diff --git a/core/network/impl/reputation_repository_impl.hpp b/core/network/impl/reputation_repository_impl.hpp index f845d0330c..d1bca14a6d 100644 --- a/core/network/impl/reputation_repository_impl.hpp +++ b/core/network/impl/reputation_repository_impl.hpp @@ -33,8 +33,6 @@ namespace kagome::network { std::chrono::seconds duration) override; private: - void ensurePeerPresence(const PeerId &peer_id); - void tick(); std::shared_ptr scheduler_; From 04658b643b3f6f15d1854e76c9a9713da089932d Mon Sep 17 00:00:00 2001 From: Dmitriy Khaustov aka xDimon Date: Thu, 29 Sep 2022 23:32:04 +0800 Subject: [PATCH 4/7] fix: review issues Signed-off-by: Dmitriy Khaustov aka xDimon --- core/network/impl/reputation_repository_impl.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/core/network/impl/reputation_repository_impl.cpp b/core/network/impl/reputation_repository_impl.cpp index 7020037503..b90c08473f 100644 --- a/core/network/impl/reputation_repository_impl.cpp +++ b/core/network/impl/reputation_repository_impl.cpp @@ -56,9 +56,8 @@ namespace kagome::network { diff.reason); auto value = static_cast( - (std::signbit(diff.value) ? 1 : -1) // raise or reduce - * static_cast(std::abs(diff.value)) // original - * 0.98 * duration.count() // multiplier considering timeout + -static_cast(diff.value) // opposite original + * 0.98 * duration.count() // multiplier considering timeout ); if (value != 0) { From 9f2257d991112dc2149b389d662ffe553220895a Mon Sep 17 00:00:00 2001 From: Dmitriy Khaustov aka xDimon Date: Thu, 29 Sep 2022 23:37:52 +0800 Subject: [PATCH 5/7] fix: review issues Signed-off-by: Dmitriy Khaustov aka xDimon --- core/network/impl/reputation_repository_impl.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/network/impl/reputation_repository_impl.cpp b/core/network/impl/reputation_repository_impl.cpp index b90c08473f..0532992e39 100644 --- a/core/network/impl/reputation_repository_impl.cpp +++ b/core/network/impl/reputation_repository_impl.cpp @@ -56,8 +56,8 @@ namespace kagome::network { diff.reason); auto value = static_cast( - -static_cast(diff.value) // opposite original - * 0.98 * duration.count() // multiplier considering timeout + -static_cast(diff.value) // opposite original + * std::pow(0.98, duration.count()) // multiplier considering timeout ); if (value != 0) { From e202c85d79a86b985ba13011a0ba53f829e00a72 Mon Sep 17 00:00:00 2001 From: Dmitriy Khaustov aka xDimon Date: Fri, 30 Sep 2022 00:03:14 +0800 Subject: [PATCH 6/7] fix: review issues Signed-off-by: Dmitriy Khaustov aka xDimon --- core/network/impl/reputation_repository_impl.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/core/network/impl/reputation_repository_impl.cpp b/core/network/impl/reputation_repository_impl.cpp index 0532992e39..ba7e8da54f 100644 --- a/core/network/impl/reputation_repository_impl.cpp +++ b/core/network/impl/reputation_repository_impl.cpp @@ -62,9 +62,16 @@ namespace kagome::network { if (value != 0) { scheduler_->schedule( - [wp = weak_from_this(), peer_id, value] { + [wp = weak_from_this(), peer_id, value, reason = diff.reason] { if (auto self = wp.lock()) { - self->change(peer_id, {value, "Revert by timeout"}); + auto reputation = self->reputation_table_[peer_id] += value; + SL_DEBUG(self->log_, + "Reputation of peer {} was changed by {} points to {} " + "points. Reason: reverse of `{}'", + peer_id, + value, + reputation, + reason); } }, duration); From 60838da3cf86c533d36456d35b00795c3cd84550 Mon Sep 17 00:00:00 2001 From: Dmitriy Khaustov aka xDimon Date: Fri, 30 Sep 2022 00:15:17 +0800 Subject: [PATCH 7/7] fix: review issues Signed-off-by: Dmitriy Khaustov aka xDimon --- core/network/impl/reputation_repository_impl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/network/impl/reputation_repository_impl.cpp b/core/network/impl/reputation_repository_impl.cpp index ba7e8da54f..a76927c684 100644 --- a/core/network/impl/reputation_repository_impl.cpp +++ b/core/network/impl/reputation_repository_impl.cpp @@ -95,7 +95,7 @@ namespace kagome::network { auto diff = reputation / 50; if (diff == 0) { - diff = reputation > 0 ? -1 : 1; + diff = reputation < 0 ? -1 : 1; } reputation -= diff;