From a8ed4849705fe9e4dc7ca56fc3b228312db8c65c Mon Sep 17 00:00:00 2001 From: Alexander Lednev <57529355+iceseer@users.noreply.github.com> Date: Thu, 7 Sep 2023 11:35:50 +0300 Subject: [PATCH] Fix/voting round impl zombie (#1794) * voting round impl fixup Signed-off-by: iceseer --------- Signed-off-by: iceseer Co-authored-by: kamilsa --- core/consensus/grandpa/impl/grandpa_impl.cpp | 12 ++--- .../grandpa/impl/voting_round_impl.cpp | 54 ++++++++++++------- .../grandpa/impl/voting_round_impl.hpp | 19 ++++--- core/consensus/grandpa/voting_round.hpp | 2 +- .../voting_round/voting_round_test.cpp | 20 +++---- 5 files changed, 65 insertions(+), 42 deletions(-) diff --git a/core/consensus/grandpa/impl/grandpa_impl.cpp b/core/consensus/grandpa/impl/grandpa_impl.cpp index e60dd6ed77..28353dfba4 100644 --- a/core/consensus/grandpa/impl/grandpa_impl.cpp +++ b/core/consensus/grandpa/impl/grandpa_impl.cpp @@ -254,7 +254,7 @@ namespace kagome::consensus::grandpa { auto vote_crypto_provider = std::make_shared( keypair, crypto_provider_, round_state.round_number, config.voters); - auto new_round = std::make_shared( + auto new_round = VotingRoundImpl::create( shared_from_this(), std::move(config), hasher_, @@ -313,7 +313,7 @@ namespace kagome::consensus::grandpa { auto vote_crypto_provider = std::make_shared( keypair, crypto_provider_, new_round_number, config.voters); - auto new_round = std::make_shared( + auto new_round = VotingRoundImpl::create( shared_from_this(), std::move(config), hasher_, @@ -1357,7 +1357,7 @@ namespace kagome::consensus::grandpa { auto voters = VoterSet::make(authorities).value(); MovableRoundState state; state.round_number = justification.round_number; - VotingRoundImpl round{ + auto round = VotingRoundImpl::create( shared_from_this(), GrandpaConfig{voters, justification.round_number, {}, {}}, hasher_, @@ -1369,9 +1369,9 @@ namespace kagome::consensus::grandpa { std::make_shared( primitives::BlockInfo{}, voters, environment_), scheduler_, - state, - }; - promise_res->set_value(round.validatePrecommitJustification(justification)); + state); + promise_res->set_value( + round->validatePrecommitJustification(justification)); } void GrandpaImpl::applyJustification( diff --git a/core/consensus/grandpa/impl/voting_round_impl.cpp b/core/consensus/grandpa/impl/voting_round_impl.cpp index 32f7b43f14..0df6854ac9 100644 --- a/core/consensus/grandpa/impl/voting_round_impl.cpp +++ b/core/consensus/grandpa/impl/voting_round_impl.cpp @@ -227,8 +227,13 @@ namespace kagome::consensus::grandpa { SL_DEBUG(logger_, "Round #{}: Start round", round_number_); - pending_timer_handle_ = - scheduler_->scheduleWithHandle([&] { pending(); }, pending_interval_); + pending_timer_handle_ = scheduler_->scheduleWithHandle( + [wself{weak_from_this()}] { + if (auto self = wself.lock()) { + self->pending(); + } + }, + pending_interval_); sendNeighborMessage(); @@ -281,12 +286,14 @@ namespace kagome::consensus::grandpa { } stage_timer_handle_ = scheduler_->scheduleWithHandle( - [&] { - if (stage_ == Stage::PREVOTE_RUNS) { - SL_DEBUG(logger_, - "Round #{}: Time of prevote stage is out", - round_number_); - endPrevoteStage(); + [wself{weak_from_this()}] { + if (auto self = wself.lock()) { + if (self->stage_ == Stage::PREVOTE_RUNS) { + SL_DEBUG(self->logger_, + "Round #{}: Time of prevote stage is out", + self->round_number_); + self->endPrevoteStage(); + } } }, toMilliseconds(duration_ * 2 - (scheduler_->now() - start_time_))); @@ -346,12 +353,14 @@ namespace kagome::consensus::grandpa { } stage_timer_handle_ = scheduler_->scheduleWithHandle( - [&] { - if (stage_ == Stage::PRECOMMIT_RUNS) { - SL_DEBUG(logger_, - "Round #{}: Time of precommit stage is out", - round_number_); - endPrecommitStage(); + [wself{weak_from_this()}] { + if (auto self = wself.lock()) { + if (self->stage_ == Stage::PRECOMMIT_RUNS) { + SL_DEBUG(self->logger_, + "Round #{}: Time of precommit stage is out", + self->round_number_); + self->endPrecommitStage(); + } } }, toMilliseconds(duration_ * 4 - (scheduler_->now() - start_time_))); @@ -1086,9 +1095,11 @@ namespace kagome::consensus::grandpa { need_to_update_estimate = true; } if (prevote_ghost_) { - scheduler_->schedule([&] { - if (stage_ == Stage::PRECOMMIT_WAITS_FOR_PREVOTES) { - endPrecommitStage(); + scheduler_->schedule([wself{weak_from_this()}] { + if (auto self = wself.lock()) { + if (self->stage_ == Stage::PRECOMMIT_WAITS_FOR_PREVOTES) { + self->endPrecommitStage(); + } } }); } @@ -1609,7 +1620,12 @@ namespace kagome::consensus::grandpa { SL_DEBUG(logger_, "Resend votes of recent rounds"); resend(shared_from_this()); - pending_timer_handle_ = - scheduler_->scheduleWithHandle([&] { pending(); }, pending_interval_); + pending_timer_handle_ = scheduler_->scheduleWithHandle( + [wself{weak_from_this()}] { + if (auto self = wself.lock()) { + self->pending(); + } + }, + pending_interval_); } } // namespace kagome::consensus::grandpa diff --git a/core/consensus/grandpa/impl/voting_round_impl.hpp b/core/consensus/grandpa/impl/voting_round_impl.hpp index 17d26412a8..de3702a2cb 100644 --- a/core/consensus/grandpa/impl/voting_round_impl.hpp +++ b/core/consensus/grandpa/impl/voting_round_impl.hpp @@ -25,7 +25,12 @@ namespace kagome::consensus::grandpa { namespace kagome::consensus::grandpa { - class VotingRoundImpl : public VotingRound { + class VotingRoundImpl : public VotingRound, + public std::enable_shared_from_this { + protected: + // This ctor is needed only for tests purposes + VotingRoundImpl() : round_number_{}, duration_{} {} + private: VotingRoundImpl(const std::shared_ptr &grandpa, const GrandpaConfig &config, @@ -37,11 +42,6 @@ namespace kagome::consensus::grandpa { std::shared_ptr vote_graph, std::shared_ptr scheduler); - protected: - // This ctor is needed only for tests purposes - VotingRoundImpl() : round_number_{}, duration_{} {} - - public: VotingRoundImpl( const std::shared_ptr &grandpa, const GrandpaConfig &config, @@ -66,6 +66,13 @@ namespace kagome::consensus::grandpa { const std::shared_ptr &scheduler, const std::shared_ptr &previous_round); + public: + template + static std::shared_ptr create(Args &&...args) { + return std::shared_ptr( + new VotingRoundImpl(std::forward(args)...)); + } + enum class Stage { // Initial stage, round is just created INIT, diff --git a/core/consensus/grandpa/voting_round.hpp b/core/consensus/grandpa/voting_round.hpp index 0f5ebb349a..d83b907611 100644 --- a/core/consensus/grandpa/voting_round.hpp +++ b/core/consensus/grandpa/voting_round.hpp @@ -15,7 +15,7 @@ namespace kagome::consensus::grandpa { /** * Handles execution of one grandpa round. For details @see VotingRoundImpl */ - class VotingRound : public std::enable_shared_from_this { + class VotingRound { public: virtual ~VotingRound() = default; diff --git a/test/core/consensus/grandpa/voting_round/voting_round_test.cpp b/test/core/consensus/grandpa/voting_round/voting_round_test.cpp index c1f8059faf..bb18e42532 100644 --- a/test/core/consensus/grandpa/voting_round/voting_round_test.cpp +++ b/test/core/consensus/grandpa/voting_round/voting_round_test.cpp @@ -160,16 +160,16 @@ class VotingRoundTest : public testing::Test, .WillRepeatedly(ReturnRef(finalized_in_prev_round_)); EXPECT_CALL(*previous_round_, doCommit()).Times(AnyNumber()); - round_ = std::make_shared(grandpa_, - config, - hasher_, - env_, - vote_crypto_provider_, - prevotes_, - precommits_, - vote_graph_, - scheduler_, - previous_round_); + round_ = VotingRoundImpl::create(grandpa_, + config, + hasher_, + env_, + vote_crypto_provider_, + prevotes_, + precommits_, + vote_graph_, + scheduler_, + previous_round_); } SignedMessage preparePrimaryPropose(const Id &id,