From d7d850ac7db402021f26f962f62970035e9feb00 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 21 Dec 2023 17:35:52 +0300 Subject: [PATCH 1/2] native: never set Neo's newEpoch* cache values to nil We have cache update mechanism (Neo's cache votesChanged flag), it must be used for current epoch and new epoch cached values update. And the cached current/new epoch values themselves must always contain valid information for the current/new epoch. These cached values must only be changed once per epoch, never set them to nil. This commit prevents CN node panic described in #3253 when dBFT tries to retrieve new epoch validators with some votes modifications made before at the same dBFT epoch. Close #3253. Signed-off-by: Anna Shaleva --- pkg/core/native/native_neo.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/core/native/native_neo.go b/pkg/core/native/native_neo.go index 67b2f7d57..620174a7f 100644 --- a/pkg/core/native/native_neo.go +++ b/pkg/core/native/native_neo.go @@ -971,7 +971,6 @@ func (n *NEO) ModifyAccountVotes(acc *state.NEOBalance, d *dao.Simple, value *bi return nil } } - cache.newEpochNextValidators = nil return putConvertibleToDAO(n.ID, d, key, cd) } return nil From 47aefad7ea2d51a033bed567e0f5ff6e1ae425d1 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 21 Dec 2023 18:03:22 +0300 Subject: [PATCH 2/2] consensus: remove unnecessary call to ComputeNextBlockValidators dBFT doesn't use validators got from this call to GetValidators callback, because NeoGo doesn't properly set WithGetConsensusAddress, and thus this call can be safely skipped. Instead, NeoGo fills NextConsensus field by itself in NewBlockFromContext callback. This commit technically doesn't perform any functional changes and doesn't affect the problem described in #3253 in any way. This commit is just a removal of the code that was never used by NeoGo library. This commit is a direct consequence of https://github.com/nspcc-dev/dbft/issues/84. Signed-off-by: Anna Shaleva --- pkg/consensus/consensus.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/pkg/consensus/consensus.go b/pkg/consensus/consensus.go index 908683705..c40e72b16 100644 --- a/pkg/consensus/consensus.go +++ b/pkg/consensus/consensus.go @@ -681,12 +681,12 @@ func (s *service) getValidators(txes ...block.Transaction) []crypto.PublicKey { // block's validators, thus should return validators from the current // epoch without recalculation. pKeys, err = s.Chain.GetNextBlockValidators() - } else { - // getValidators with non-empty args is used by dbft to fill block's - // NextConsensus field, ComputeNextBlockValidators will return proper - // value for NextConsensus wrt dBFT epoch start/end. - pKeys = s.Chain.ComputeNextBlockValidators() } + // getValidators with non-empty args is used by dbft to fill block's + // NextConsensus field, but NeoGo doesn't provide WithGetConsensusAddress + // callback and fills NextConsensus by itself via WithNewBlockFromContext + // callback. Thus, leave pKeys empty if txes != nil. + if err != nil { s.log.Error("error while trying to get validators", zap.Error(err)) } @@ -727,6 +727,16 @@ func (s *service) newBlockFromContext(ctx *dbft.Context) block.Block { block.PrevStateRoot = sr.Root } + // ComputeNextBlockValidators returns proper set of validators wrt dBFT epochs + // boundary. I.e. for the last block in the dBFT epoch this method returns the + // list of validators recalculated from the latest relevant information about + // NEO votes; in this case list of validators may differ from the one returned + // by GetNextBlockValidators. For the not-last block of dBFT epoch this method + // returns the same list as GetNextBlockValidators. Note, that by this moment + // we must be sure that previous block was successfully persisted to chain + // (i.e. PostPersist was completed for native Neo contract and PostPersist + // execution cache was persisted to s.Chain's DAO), otherwise the wrong + // (outdated, relevant for the previous dBFT epoch) value will be returned. var validators = s.Chain.ComputeNextBlockValidators() script, err := smartcontract.CreateDefaultMultiSigRedeemScript(validators) if err != nil {