From 96449d803a91b258e42bed8a8e1fb8970cbe3383 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Wed, 30 Aug 2023 14:02:42 +0300 Subject: [PATCH 1/7] native: rework native NEO next block validators cache Blockchain passes his own pure unwrapped DAO to (*Blockchain).ComputeNextBlockValidators which means that native RW NEO cache structure stored inside this DAO can be modified by anyone who uses exported ComputeNextBlockValidators Blockchain API, and technically it's valid, and we should allow this, because it's the only purpose of `validators` caching. However, at the same time some RPC server is allowed to request a subsequent wrapped DAO for some test invocation. It means that descendant wrapped DAO eventually will request RW NEO cache and try to `Copy()` the underlying's DAO cache which is in direct use of ComputeNextBlockValidators. Here's the race: ComputeNextBlockValidators called by Consensus service tries to update cached `validators` value, and descendant wrapped DAO created by the RPC server tries to copy DAO's native cache and read the cached `validators` value. So the problem is that native cache not designated to handle concurrent access between parent DAO layer and derived (wrapped) DAO layer. I've carefully reviewed all the usages of native cache, and turns out that the described situation is the only place where parent DAO is used directly to modify its cache concurrently with some descendant DAO that is trying to access the cache. All other usages of native cache (not only NEO, but also all other native contrcts) strictly rely on the hierarchical DAO structure and don't try to perform these concurrent operations between DAO layers. There's also persist operation, but it keeps cache RW lock taken, so it doesn't have this problem as far. Thus, in this commit we rework NEO's `validators` cache value so that it always contain the relevant list for upper Blockchain's DAO and is updated every PostPersist (if needed). Note: we must be very careful extending our native cache in the future, every usage of native cache must be checked against the described problem. Close #2989. Signed-off-by: Anna Shaleva --- pkg/consensus/consensus.go | 6 +- pkg/core/blockchain.go | 14 +++-- pkg/core/blockchain_core_test.go | 5 +- pkg/core/native/native_neo.go | 78 ++++++++++++++++++------- pkg/core/native/native_test/neo_test.go | 8 +-- 5 files changed, 76 insertions(+), 35 deletions(-) diff --git a/pkg/consensus/consensus.go b/pkg/consensus/consensus.go index 9a29f8c2d..8120e1d16 100644 --- a/pkg/consensus/consensus.go +++ b/pkg/consensus/consensus.go @@ -48,7 +48,7 @@ type Ledger interface { GetNextBlockValidators() ([]*keys.PublicKey, error) GetStateRoot(height uint32) (*state.MPTRoot, error) GetTransaction(util.Uint256) (*transaction.Transaction, uint32, error) - GetValidators() ([]*keys.PublicKey, error) + GetValidators() []*keys.PublicKey PoolTx(t *transaction.Transaction, pools ...*mempool.Pool) error SubscribeForBlocks(ch chan *coreb.Block) UnsubscribeFromBlocks(ch chan *coreb.Block) @@ -679,7 +679,7 @@ func (s *service) getValidators(txes ...block.Transaction) []crypto.PublicKey { if txes == nil { pKeys, err = s.Chain.GetNextBlockValidators() } else { - pKeys, err = s.Chain.GetValidators() + pKeys = s.Chain.GetValidators() } if err != nil { s.log.Error("error while trying to get validators", zap.Error(err)) @@ -725,7 +725,7 @@ func (s *service) newBlockFromContext(ctx *dbft.Context) block.Block { var err error cfg := s.Chain.GetConfig().ProtocolConfiguration if cfg.ShouldUpdateCommitteeAt(ctx.BlockIndex) { - validators, err = s.Chain.GetValidators() + validators = s.Chain.GetValidators() } else { validators, err = s.Chain.GetNextBlockValidators() } diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index c2f3e9e00..a75fdad95 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -2697,12 +2697,18 @@ func (bc *Blockchain) GetCommittee() (keys.PublicKeys, error) { return pubs, nil } -// GetValidators returns current validators. -func (bc *Blockchain) GetValidators() ([]*keys.PublicKey, error) { - return bc.contracts.NEO.ComputeNextBlockValidators(bc.blockHeight, bc.dao) +// GetValidators returns current validators. Validators list returned from this +// method may be updated every block (depending on register/unregister/vote +// calls to NeoToken contract), not only once per (committee size) number of +// blocks. +func (bc *Blockchain) GetValidators() []*keys.PublicKey { + return bc.contracts.NEO.ComputeNextBlockValidators(bc.dao) } -// GetNextBlockValidators returns next block validators. +// GetNextBlockValidators returns next block validators. Validators list returned +// from this method is the sorted top NumOfCNs number of public keys from the +// current committee, thus, validators list returned from this method is being +// updated once per (committee size) number of blocks. func (bc *Blockchain) GetNextBlockValidators() ([]*keys.PublicKey, error) { return bc.contracts.NEO.GetNextBlockValidatorsInternal(bc.dao), nil } diff --git a/pkg/core/blockchain_core_test.go b/pkg/core/blockchain_core_test.go index edfeabc25..e17ee7df9 100644 --- a/pkg/core/blockchain_core_test.go +++ b/pkg/core/blockchain_core_test.go @@ -261,8 +261,7 @@ func TestChainWithVolatileNumOfValidators(t *testing.T) { priv0 := testchain.PrivateKeyByID(0) - vals, err := bc.GetValidators() - require.NoError(t, err) + vals := bc.GetValidators() script, err := smartcontract.CreateDefaultMultiSigRedeemScript(vals) require.NoError(t, err) curWit := transaction.Witness{ @@ -280,7 +279,7 @@ func TestChainWithVolatileNumOfValidators(t *testing.T) { } // Mimic consensus. if bc.config.ShouldUpdateCommitteeAt(uint32(i)) { - vals, err = bc.GetValidators() + vals = bc.GetValidators() } else { vals, err = bc.GetNextBlockValidators() } diff --git a/pkg/core/native/native_neo.go b/pkg/core/native/native_neo.go index 6c8742dbb..ce5ddd5bc 100644 --- a/pkg/core/native/native_neo.go +++ b/pkg/core/native/native_neo.go @@ -50,7 +50,12 @@ type NeoCache struct { votesChanged bool nextValidators keys.PublicKeys - validators keys.PublicKeys + // validators contains cached next block validators. This list is updated every + // PostPersist if candidates votes ratio has been changed or register/unregister + // operation was performed within the persisted block. The updated value is + // being persisted following the standard layered DAO persist rules, so that + // external users will always get the proper value with upper Blockchain's DAO. + validators keys.PublicKeys // committee contains cached committee members and their votes. // It is updated once in a while depending on committee size // (every 28 blocks for mainnet). It's value @@ -269,6 +274,7 @@ func (n *NEO) Initialize(ic *interop.Context) error { cache := &NeoCache{ gasPerVoteCache: make(map[string]big.Int), votesChanged: true, + validators: nil, // will be updated in genesis PostPersist. } // We need cache to be present in DAO before the subsequent call to `mint`. @@ -325,6 +331,12 @@ func (n *NEO) InitializeCache(blockHeight uint32, d *dao.Simple) error { cache.gasPerBlock = n.getSortedGASRecordFromDAO(d) cache.registerPrice = getIntWithKey(n.ID, d, []byte{prefixRegisterPrice}) + numOfCNs := n.cfg.GetNumOfCNs(blockHeight + 1) + err := n.updateCachedValidators(d, cache, blockHeight, numOfCNs) + if err != nil { + return fmt.Errorf("failed to update next block validators cache: %w", err) + } + d.SetCache(n.ID, cache) return nil } @@ -353,6 +365,20 @@ func (n *NEO) updateCache(cache *NeoCache, cvs keysWithVotes, blockHeight uint32 return nil } +// updateCachedValidators sets validators cache that will be used by external users +// to retrieve next block validators list. Thus, it stores the list of validators +// computed using the currently persisted block state. +func (n *NEO) updateCachedValidators(d *dao.Simple, cache *NeoCache, blockHeight uint32, numOfCNs int) error { + result, _, err := n.computeCommitteeMembers(blockHeight, d) + if err != nil { + return fmt.Errorf("failed to compute committee members: %w", err) + } + result = result[:numOfCNs] + sort.Sort(result) + cache.validators = result + return nil +} + func (n *NEO) updateCommittee(cache *NeoCache, ic *interop.Context) error { if !cache.votesChanged { // We need to put in storage anyway, as it affects dumps @@ -399,6 +425,7 @@ func (n *NEO) PostPersist(ic *interop.Context) error { committeeReward := new(big.Int).Mul(gas, bigCommitteeRewardRatio) n.GAS.mint(ic, pubs[index].GetScriptHash(), committeeReward.Div(committeeReward, big100), false) + var isCacheRW bool if n.cfg.ShouldUpdateCommitteeAt(ic.Block.Index) { var voterReward = new(big.Int).Set(bigVoterRewardRatio) voterReward.Mul(voterReward, gas) @@ -408,9 +435,8 @@ func (n *NEO) PostPersist(ic *interop.Context) error { voterReward.Div(voterReward, big100) var ( - cs = cache.committee - isCacheRW bool - key = make([]byte, 34) + cs = cache.committee + key = make([]byte, 34) ) for i := range cs { if cs[i].Votes.Sign() > 0 { @@ -437,6 +463,21 @@ func (n *NEO) PostPersist(ic *interop.Context) error { } } } + // Update next block validators cache for external users. + var ( + h = ic.Block.Index // consider persisting block as stored to get _next_ block validators + numOfCNs = n.cfg.GetNumOfCNs(h + 1) + ) + if cache.validators == nil || numOfCNs != len(cache.validators) { + if !isCacheRW { + cache = ic.DAO.GetRWCache(n.ID).(*NeoCache) + } + err := n.updateCachedValidators(ic.DAO, cache, h, numOfCNs) + if err != nil { + return fmt.Errorf("failed to update next block validators cache: %w", err) + } + } + return nil } @@ -1042,24 +1083,21 @@ func (n *NEO) getAccountState(ic *interop.Context, args []stackitem.Item) stacki return item } -// ComputeNextBlockValidators returns an actual list of current validators. -func (n *NEO) ComputeNextBlockValidators(blockHeight uint32, d *dao.Simple) (keys.PublicKeys, error) { - numOfCNs := n.cfg.GetNumOfCNs(blockHeight + 1) - // Most of the time it should be OK with RO cache, thus try to retrieve - // validators without RW cache creation to avoid cached values copying. +// ComputeNextBlockValidators computes an actual list of current validators that is +// relevant for the latest persisted block and based on the latest changes made by +// register/unregister/vote calls. +// Note: this method isn't actually "computes" new committee list and calculates +// new validators list from it. Instead, it uses cache, but the cache itself is +// updated every block. +func (n *NEO) ComputeNextBlockValidators(d *dao.Simple) keys.PublicKeys { + // It should always be OK with RO cache if using lower-layered DAO with proper + // cache set. cache := d.GetROCache(n.ID).(*NeoCache) - if vals := cache.validators; vals != nil && numOfCNs == len(vals) { - return vals.Copy(), nil + if vals := cache.validators; vals != nil { + return vals.Copy() } - cache = d.GetRWCache(n.ID).(*NeoCache) - result, _, err := n.computeCommitteeMembers(blockHeight, d) - if err != nil { - return nil, err - } - result = result[:numOfCNs] - sort.Sort(result) - cache.validators = result - return result, nil + // It's a program error not to have the right value in lower cache. + panic(fmt.Errorf("unexpected validators cache content: %v", cache.validators)) } func (n *NEO) getCommittee(ic *interop.Context, _ []stackitem.Item) stackitem.Item { diff --git a/pkg/core/native/native_test/neo_test.go b/pkg/core/native/native_test/neo_test.go index cbaefa883..dcb9ac415 100644 --- a/pkg/core/native/native_test/neo_test.go +++ b/pkg/core/native/native_test/neo_test.go @@ -138,8 +138,7 @@ func TestNEO_Vote(t *testing.T) { require.NoError(t, err) standBySorted = standBySorted[:validatorsCount] sort.Sort(standBySorted) - pubs, err := e.Chain.GetValidators() - require.NoError(t, err) + pubs := e.Chain.GetValidators() require.Equal(t, standBySorted, keys.PublicKeys(pubs)) // voters vote for candidates. The aim of this test is to check if voting @@ -176,7 +175,7 @@ func TestNEO_Vote(t *testing.T) { } // We still haven't voted enough validators in. - pubs, err = e.Chain.GetValidators() + pubs = e.Chain.GetValidators() require.NoError(t, err) require.Equal(t, standBySorted, keys.PublicKeys(pubs)) @@ -267,8 +266,7 @@ func TestNEO_Vote(t *testing.T) { advanceChain(t) - pubs, err = e.Chain.GetValidators() - require.NoError(t, err) + pubs = e.Chain.GetValidators() for i := range pubs { require.NotEqual(t, candidates[0], pubs[i]) require.NotEqual(t, candidates[len(candidates)-1], pubs[i]) From 07e1bc7cd7ae4e68f9f67f412e28408433c11b30 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Wed, 30 Aug 2023 19:48:20 +0300 Subject: [PATCH 2/7] core: rename (*Blockchain).GetValidators to ComputeNextBlockValidators We have two similar blockchain APIs: GetNextBlockValidators and GetValidators. It's hard to distinguish them, thus renaming it to match the meaning, so what we have now is: GetNextBlockValidators literally just returns the top of the committee that was elected in the start of batch of CommitteeSize blocks batch. It doesn't change its valie every block. ComputeNextBlockValidators literally computes the list of validators based on the most fresh committee members information got from the NeoToken's storage and based on the latest register/unregister/vote events. The list returned by this method may be updated every block. Signed-off-by: Anna Shaleva --- pkg/consensus/consensus.go | 6 +++--- pkg/core/blockchain.go | 16 +++++++++------- pkg/core/blockchain_core_test.go | 4 ++-- pkg/core/native/native_test/neo_test.go | 6 +++--- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/pkg/consensus/consensus.go b/pkg/consensus/consensus.go index 8120e1d16..633cda9c8 100644 --- a/pkg/consensus/consensus.go +++ b/pkg/consensus/consensus.go @@ -48,7 +48,7 @@ type Ledger interface { GetNextBlockValidators() ([]*keys.PublicKey, error) GetStateRoot(height uint32) (*state.MPTRoot, error) GetTransaction(util.Uint256) (*transaction.Transaction, uint32, error) - GetValidators() []*keys.PublicKey + ComputeNextBlockValidators() []*keys.PublicKey PoolTx(t *transaction.Transaction, pools ...*mempool.Pool) error SubscribeForBlocks(ch chan *coreb.Block) UnsubscribeFromBlocks(ch chan *coreb.Block) @@ -679,7 +679,7 @@ func (s *service) getValidators(txes ...block.Transaction) []crypto.PublicKey { if txes == nil { pKeys, err = s.Chain.GetNextBlockValidators() } else { - pKeys = s.Chain.GetValidators() + pKeys = s.Chain.ComputeNextBlockValidators() } if err != nil { s.log.Error("error while trying to get validators", zap.Error(err)) @@ -725,7 +725,7 @@ func (s *service) newBlockFromContext(ctx *dbft.Context) block.Block { var err error cfg := s.Chain.GetConfig().ProtocolConfiguration if cfg.ShouldUpdateCommitteeAt(ctx.BlockIndex) { - validators = s.Chain.GetValidators() + validators = s.Chain.ComputeNextBlockValidators() } else { validators, err = s.Chain.GetNextBlockValidators() } diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index a75fdad95..ced06626f 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -2697,18 +2697,20 @@ func (bc *Blockchain) GetCommittee() (keys.PublicKeys, error) { return pubs, nil } -// GetValidators returns current validators. Validators list returned from this -// method may be updated every block (depending on register/unregister/vote -// calls to NeoToken contract), not only once per (committee size) number of -// blocks. -func (bc *Blockchain) GetValidators() []*keys.PublicKey { +// ComputeNextBlockValidators returns current validators. Validators list +// returned from this method may be updated every block (depending on +// register/unregister/vote calls to NeoToken contract), not only once per +// (committee size) number of blocks. +func (bc *Blockchain) ComputeNextBlockValidators() []*keys.PublicKey { return bc.contracts.NEO.ComputeNextBlockValidators(bc.dao) } // GetNextBlockValidators returns next block validators. Validators list returned // from this method is the sorted top NumOfCNs number of public keys from the -// current committee, thus, validators list returned from this method is being -// updated once per (committee size) number of blocks. +// committee of the current dBFT round (that was calculated once for the +// CommitteeSize number of blocks), thus, validators list returned from this +// method is being updated once per (committee size) number of blocks, but not +// every block. func (bc *Blockchain) GetNextBlockValidators() ([]*keys.PublicKey, error) { return bc.contracts.NEO.GetNextBlockValidatorsInternal(bc.dao), nil } diff --git a/pkg/core/blockchain_core_test.go b/pkg/core/blockchain_core_test.go index e17ee7df9..1d45c115b 100644 --- a/pkg/core/blockchain_core_test.go +++ b/pkg/core/blockchain_core_test.go @@ -261,7 +261,7 @@ func TestChainWithVolatileNumOfValidators(t *testing.T) { priv0 := testchain.PrivateKeyByID(0) - vals := bc.GetValidators() + vals := bc.ComputeNextBlockValidators() script, err := smartcontract.CreateDefaultMultiSigRedeemScript(vals) require.NoError(t, err) curWit := transaction.Witness{ @@ -279,7 +279,7 @@ func TestChainWithVolatileNumOfValidators(t *testing.T) { } // Mimic consensus. if bc.config.ShouldUpdateCommitteeAt(uint32(i)) { - vals = bc.GetValidators() + vals = bc.ComputeNextBlockValidators() } else { vals, err = bc.GetNextBlockValidators() } diff --git a/pkg/core/native/native_test/neo_test.go b/pkg/core/native/native_test/neo_test.go index dcb9ac415..e0a46f5d2 100644 --- a/pkg/core/native/native_test/neo_test.go +++ b/pkg/core/native/native_test/neo_test.go @@ -138,7 +138,7 @@ func TestNEO_Vote(t *testing.T) { require.NoError(t, err) standBySorted = standBySorted[:validatorsCount] sort.Sort(standBySorted) - pubs := e.Chain.GetValidators() + pubs := e.Chain.ComputeNextBlockValidators() require.Equal(t, standBySorted, keys.PublicKeys(pubs)) // voters vote for candidates. The aim of this test is to check if voting @@ -175,7 +175,7 @@ func TestNEO_Vote(t *testing.T) { } // We still haven't voted enough validators in. - pubs = e.Chain.GetValidators() + pubs = e.Chain.ComputeNextBlockValidators() require.NoError(t, err) require.Equal(t, standBySorted, keys.PublicKeys(pubs)) @@ -266,7 +266,7 @@ func TestNEO_Vote(t *testing.T) { advanceChain(t) - pubs = e.Chain.GetValidators() + pubs = e.Chain.ComputeNextBlockValidators() for i := range pubs { require.NotEqual(t, candidates[0], pubs[i]) require.NotEqual(t, candidates[len(candidates)-1], pubs[i]) From 312534af7c6d3858527da5e3c9cc41e90afbb366 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 1 Sep 2023 15:10:42 +0300 Subject: [PATCH 3/7] native: don't recalculate validators every block Recalculate them once per epoch. Consensus is aware of it and must call CalculateNextValidators exactly when needed. Signed-off-by: Anna Shaleva --- pkg/consensus/consensus.go | 18 ++++++++- pkg/core/native/native_neo.go | 76 +++++++++++++++++++++-------------- 2 files changed, 63 insertions(+), 31 deletions(-) diff --git a/pkg/consensus/consensus.go b/pkg/consensus/consensus.go index 633cda9c8..540bcdb2a 100644 --- a/pkg/consensus/consensus.go +++ b/pkg/consensus/consensus.go @@ -677,9 +677,22 @@ func (s *service) getValidators(txes ...block.Transaction) []crypto.PublicKey { err error ) if txes == nil { + // getValidators with empty args is used by dbft to fill the list of + // block's validators, thus should return validators from the current + // epoch without recalculation. pKeys, err = s.Chain.GetNextBlockValidators() } else { - pKeys = s.Chain.ComputeNextBlockValidators() + // getValidators with non-empty args is used by dbft to fill block's + // NextConsensus field, thus should return proper value for NextConsensus. + cfg := s.Chain.GetConfig().ProtocolConfiguration + if cfg.ShouldUpdateCommitteeAt(s.dbft.Context.BlockIndex) { + // Calculate NextConsensus based on the most fresh chain state. + pKeys = s.Chain.ComputeNextBlockValidators() + } else { + // Take the cached validators that are relevant for the current dBFT epoch + // to make NextConsensus. + pKeys, err = s.Chain.GetNextBlockValidators() + } } if err != nil { s.log.Error("error while trying to get validators", zap.Error(err)) @@ -725,8 +738,11 @@ func (s *service) newBlockFromContext(ctx *dbft.Context) block.Block { var err error cfg := s.Chain.GetConfig().ProtocolConfiguration if cfg.ShouldUpdateCommitteeAt(ctx.BlockIndex) { + // Calculate NextConsensus based on the most fresh chain state. validators = s.Chain.ComputeNextBlockValidators() } else { + // Take the cached validators that are relevant for the current dBFT epoch + // to make NextConsensus. validators, err = s.Chain.GetNextBlockValidators() } if err != nil { diff --git a/pkg/core/native/native_neo.go b/pkg/core/native/native_neo.go index ce5ddd5bc..d6f9ed84d 100644 --- a/pkg/core/native/native_neo.go +++ b/pkg/core/native/native_neo.go @@ -50,11 +50,13 @@ type NeoCache struct { votesChanged bool nextValidators keys.PublicKeys - // validators contains cached next block validators. This list is updated every - // PostPersist if candidates votes ratio has been changed or register/unregister - // operation was performed within the persisted block. The updated value is - // being persisted following the standard layered DAO persist rules, so that - // external users will always get the proper value with upper Blockchain's DAO. + // validators contains cached next block validators. This list is updated once + // per dBFT epoch in PostPersist of the last block in the epoch if candidates + // votes ratio has been changed or register/unregister operation was performed + // within the last processed epoch. The updated value is being persisted + // following the standard layered DAO persist rules, so that external users + // will get the proper value with upper Blockchain's DAO (but this value is + // relevant only by the moment of first epoch block creation). validators keys.PublicKeys // committee contains cached committee members and their votes. // It is updated once in a while depending on committee size @@ -274,7 +276,7 @@ func (n *NEO) Initialize(ic *interop.Context) error { cache := &NeoCache{ gasPerVoteCache: make(map[string]big.Int), votesChanged: true, - validators: nil, // will be updated in genesis PostPersist. + validators: nil, // will be updated in the last epoch block's PostPersist right before the committee update block. } // We need cache to be present in DAO before the subsequent call to `mint`. @@ -317,6 +319,11 @@ func (n *NEO) InitializeCache(blockHeight uint32, d *dao.Simple) error { cache := &NeoCache{ gasPerVoteCache: make(map[string]big.Int), votesChanged: true, + // If it's a block in the middle of dbft epoch, then no one must access + // this field, and it will be updated during the PostPersist of the last + // block in the current epoch. If it's the laast block of the current epoch, + // then update this cache manually below. + validators: nil, } var committee = keysWithVotes{} @@ -331,10 +338,14 @@ func (n *NEO) InitializeCache(blockHeight uint32, d *dao.Simple) error { cache.gasPerBlock = n.getSortedGASRecordFromDAO(d) cache.registerPrice = getIntWithKey(n.ID, d, []byte{prefixRegisterPrice}) - numOfCNs := n.cfg.GetNumOfCNs(blockHeight + 1) - err := n.updateCachedValidators(d, cache, blockHeight, numOfCNs) - if err != nil { - return fmt.Errorf("failed to update next block validators cache: %w", err) + // Update next block validators cache for external users if committee should be + // updated in the next block. + if n.cfg.ShouldUpdateCommitteeAt(blockHeight + 1) { + var numOfCNs = n.cfg.GetNumOfCNs(blockHeight + 1) + err := n.updateCachedValidators(d, cache, blockHeight, numOfCNs) + if err != nil { + return fmt.Errorf("failed to update next block validators cache: %w", err) + } } d.SetCache(n.ID, cache) @@ -366,8 +377,9 @@ func (n *NEO) updateCache(cache *NeoCache, cvs keysWithVotes, blockHeight uint32 } // updateCachedValidators sets validators cache that will be used by external users -// to retrieve next block validators list. Thus, it stores the list of validators -// computed using the currently persisted block state. +// to retrieve next block validators list of the next dBFT epoch that wasn't yet +// started. Thus, it stores the list of validators computed using the persisted +// blocks state of the latest epoch. func (n *NEO) updateCachedValidators(d *dao.Simple, cache *NeoCache, blockHeight uint32, numOfCNs int) error { result, _, err := n.computeCommitteeMembers(blockHeight, d) if err != nil { @@ -463,18 +475,21 @@ func (n *NEO) PostPersist(ic *interop.Context) error { } } } - // Update next block validators cache for external users. - var ( - h = ic.Block.Index // consider persisting block as stored to get _next_ block validators - numOfCNs = n.cfg.GetNumOfCNs(h + 1) - ) - if cache.validators == nil || numOfCNs != len(cache.validators) { - if !isCacheRW { - cache = ic.DAO.GetRWCache(n.ID).(*NeoCache) - } - err := n.updateCachedValidators(ic.DAO, cache, h, numOfCNs) - if err != nil { - return fmt.Errorf("failed to update next block validators cache: %w", err) + // Update next block validators cache for external users if committee should be + // updated in the next block. + if n.cfg.ShouldUpdateCommitteeAt(ic.Block.Index + 1) { + var ( + h = ic.Block.Index // consider persisting block as stored to get _next_ block validators + numOfCNs = n.cfg.GetNumOfCNs(h + 1) + ) + if cache.validators == nil || numOfCNs != len(cache.validators) { + if !isCacheRW { + cache = ic.DAO.GetRWCache(n.ID).(*NeoCache) + } + err := n.updateCachedValidators(ic.DAO, cache, h, numOfCNs) + if err != nil { + return fmt.Errorf("failed to update next block validators cache: %w", err) + } } } @@ -1084,11 +1099,11 @@ func (n *NEO) getAccountState(ic *interop.Context, args []stackitem.Item) stacki } // ComputeNextBlockValidators computes an actual list of current validators that is -// relevant for the latest persisted block and based on the latest changes made by -// register/unregister/vote calls. +// relevant for the latest processed dBFT epoch and based on the changes made by +// register/unregister/vote calls during the latest epoch. // Note: this method isn't actually "computes" new committee list and calculates -// new validators list from it. Instead, it uses cache, but the cache itself is -// updated every block. +// new validators list from it. Instead, it uses cache, and the cache itself is +// updated during the PostPersist of the last block of every epoch. func (n *NEO) ComputeNextBlockValidators(d *dao.Simple) keys.PublicKeys { // It should always be OK with RO cache if using lower-layered DAO with proper // cache set. @@ -1096,8 +1111,9 @@ func (n *NEO) ComputeNextBlockValidators(d *dao.Simple) keys.PublicKeys { if vals := cache.validators; vals != nil { return vals.Copy() } - // It's a program error not to have the right value in lower cache. - panic(fmt.Errorf("unexpected validators cache content: %v", cache.validators)) + // It's a caller's program error to call ComputeNextBlockValidators not having + // the right value in lower cache. + panic("bug: unexpected external call to validators cache") } func (n *NEO) getCommittee(ic *interop.Context, _ []stackitem.Item) stackitem.Item { From 794658b54c2e2cbab142e9d923ebc477401ac129 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 1 Sep 2023 15:33:01 +0300 Subject: [PATCH 4/7] native: rename NEO's cached `validators` list to `newEpochNextValidators` Adjust all comments, make the field name match its meaning. Signed-off-by: Anna Shaleva --- pkg/core/native/native_neo.go | 38 +++++++++++++++++------------------ 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/pkg/core/native/native_neo.go b/pkg/core/native/native_neo.go index d6f9ed84d..324d7a18b 100644 --- a/pkg/core/native/native_neo.go +++ b/pkg/core/native/native_neo.go @@ -50,14 +50,14 @@ type NeoCache struct { votesChanged bool nextValidators keys.PublicKeys - // validators contains cached next block validators. This list is updated once + // newEpochNextValidators contains cached next block newEpochNextValidators. This list is updated once // per dBFT epoch in PostPersist of the last block in the epoch if candidates // votes ratio has been changed or register/unregister operation was performed // within the last processed epoch. The updated value is being persisted // following the standard layered DAO persist rules, so that external users // will get the proper value with upper Blockchain's DAO (but this value is // relevant only by the moment of first epoch block creation). - validators keys.PublicKeys + newEpochNextValidators keys.PublicKeys // committee contains cached committee members and their votes. // It is updated once in a while depending on committee size // (every 28 blocks for mainnet). It's value @@ -132,9 +132,9 @@ func (c *NeoCache) Copy() dao.NativeContractCache { func copyNeoCache(src, dst *NeoCache) { dst.votesChanged = src.votesChanged // Can safely omit copying because the new array is created each time - // validators list, nextValidators and committee are updated. + // newEpochNextValidators list, nextValidators and committee are updated. dst.nextValidators = src.nextValidators - dst.validators = src.validators + dst.newEpochNextValidators = src.newEpochNextValidators dst.committee = src.committee dst.committeeHash = src.committeeHash @@ -274,9 +274,9 @@ func (n *NEO) Initialize(ic *interop.Context) error { } cache := &NeoCache{ - gasPerVoteCache: make(map[string]big.Int), - votesChanged: true, - validators: nil, // will be updated in the last epoch block's PostPersist right before the committee update block. + gasPerVoteCache: make(map[string]big.Int), + votesChanged: true, + newEpochNextValidators: nil, // will be updated in the last epoch block's PostPersist right before the committee update block. } // We need cache to be present in DAO before the subsequent call to `mint`. @@ -323,7 +323,7 @@ func (n *NEO) InitializeCache(blockHeight uint32, d *dao.Simple) error { // this field, and it will be updated during the PostPersist of the last // block in the current epoch. If it's the laast block of the current epoch, // then update this cache manually below. - validators: nil, + newEpochNextValidators: nil, } var committee = keysWithVotes{} @@ -344,7 +344,7 @@ func (n *NEO) InitializeCache(blockHeight uint32, d *dao.Simple) error { var numOfCNs = n.cfg.GetNumOfCNs(blockHeight + 1) err := n.updateCachedValidators(d, cache, blockHeight, numOfCNs) if err != nil { - return fmt.Errorf("failed to update next block validators cache: %w", err) + return fmt.Errorf("failed to update next block newEpochNextValidators cache: %w", err) } } @@ -376,7 +376,7 @@ func (n *NEO) updateCache(cache *NeoCache, cvs keysWithVotes, blockHeight uint32 return nil } -// updateCachedValidators sets validators cache that will be used by external users +// updateCachedValidators sets newEpochNextValidators cache that will be used by external users // to retrieve next block validators list of the next dBFT epoch that wasn't yet // started. Thus, it stores the list of validators computed using the persisted // blocks state of the latest epoch. @@ -387,7 +387,7 @@ func (n *NEO) updateCachedValidators(d *dao.Simple, cache *NeoCache, blockHeight } result = result[:numOfCNs] sort.Sort(result) - cache.validators = result + cache.newEpochNextValidators = result return nil } @@ -475,20 +475,20 @@ func (n *NEO) PostPersist(ic *interop.Context) error { } } } - // Update next block validators cache for external users if committee should be + // Update next block newEpochNextValidators cache for external users if committee should be // updated in the next block. if n.cfg.ShouldUpdateCommitteeAt(ic.Block.Index + 1) { var ( - h = ic.Block.Index // consider persisting block as stored to get _next_ block validators + h = ic.Block.Index // consider persisting block as stored to get _next_ block newEpochNextValidators numOfCNs = n.cfg.GetNumOfCNs(h + 1) ) - if cache.validators == nil || numOfCNs != len(cache.validators) { + if cache.newEpochNextValidators == nil || numOfCNs != len(cache.newEpochNextValidators) { if !isCacheRW { cache = ic.DAO.GetRWCache(n.ID).(*NeoCache) } err := n.updateCachedValidators(ic.DAO, cache, h, numOfCNs) if err != nil { - return fmt.Errorf("failed to update next block validators cache: %w", err) + return fmt.Errorf("failed to update next block newEpochNextValidators cache: %w", err) } } } @@ -831,7 +831,7 @@ func (n *NEO) UnregisterCandidateInternal(ic *interop.Context, pub *keys.PublicK return nil } cache := ic.DAO.GetRWCache(n.ID).(*NeoCache) - cache.validators = nil + cache.newEpochNextValidators = nil c := new(candidate).FromBytes(si) emitEvent := c.Registered c.Registered = false @@ -957,7 +957,7 @@ func (n *NEO) ModifyAccountVotes(acc *state.NEOBalance, d *dao.Simple, value *bi return nil } } - cache.validators = nil + cache.newEpochNextValidators = nil return putConvertibleToDAO(n.ID, d, key, cd) } return nil @@ -1108,12 +1108,12 @@ func (n *NEO) ComputeNextBlockValidators(d *dao.Simple) keys.PublicKeys { // It should always be OK with RO cache if using lower-layered DAO with proper // cache set. cache := d.GetROCache(n.ID).(*NeoCache) - if vals := cache.validators; vals != nil { + if vals := cache.newEpochNextValidators; vals != nil { return vals.Copy() } // It's a caller's program error to call ComputeNextBlockValidators not having // the right value in lower cache. - panic("bug: unexpected external call to validators cache") + panic("bug: unexpected external call to newEpochNextValidators cache") } func (n *NEO) getCommittee(ic *interop.Context, _ []stackitem.Item) stackitem.Item { From 27dddb4d50c4d040926c5115c25e721a18e8822b Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 1 Sep 2023 16:00:53 +0300 Subject: [PATCH 5/7] native: refactor argument of NEO's getCommitteeMembers function No funcional changes, just refactoring. It doesn't need the whole cache, only the set of committee keys with votes. Signed-off-by: Anna Shaleva --- pkg/core/native/native_neo.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/core/native/native_neo.go b/pkg/core/native/native_neo.go index 324d7a18b..923b50f11 100644 --- a/pkg/core/native/native_neo.go +++ b/pkg/core/native/native_neo.go @@ -363,7 +363,7 @@ func (n *NEO) initConfigCache(cfg config.ProtocolConfiguration) error { func (n *NEO) updateCache(cache *NeoCache, cvs keysWithVotes, blockHeight uint32) error { cache.committee = cvs - var committee = getCommitteeMembers(cache) + var committee = getCommitteeMembers(cache.committee) script, err := smartcontract.CreateMajorityMultiSigRedeemScript(committee.Copy()) if err != nil { return err @@ -431,7 +431,7 @@ func (n *NEO) OnPersist(ic *interop.Context) error { func (n *NEO) PostPersist(ic *interop.Context) error { gas := n.GetGASPerBlock(ic.DAO, ic.Block.Index) cache := ic.DAO.GetROCache(n.ID).(*NeoCache) - pubs := getCommitteeMembers(cache) + pubs := getCommitteeMembers(cache.committee) committeeSize := n.cfg.GetCommitteeSize(ic.Block.Index) index := int(ic.Block.Index) % committeeSize committeeReward := new(big.Int).Mul(gas, bigCommitteeRewardRatio) @@ -1137,11 +1137,10 @@ func (n *NEO) modifyVoterTurnout(d *dao.Simple, amount *big.Int) error { // GetCommitteeMembers returns public keys of nodes in committee using cached value. func (n *NEO) GetCommitteeMembers(d *dao.Simple) keys.PublicKeys { cache := d.GetROCache(n.ID).(*NeoCache) - return getCommitteeMembers(cache) + return getCommitteeMembers(cache.committee) } -func getCommitteeMembers(cache *NeoCache) keys.PublicKeys { - var cvs = cache.committee +func getCommitteeMembers(cvs keysWithVotes) keys.PublicKeys { var committee = make(keys.PublicKeys, len(cvs)) var err error for i := range committee { From f78f915071f70483772af408f028ec7caef2f517 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 1 Sep 2023 16:16:05 +0300 Subject: [PATCH 6/7] native: optimize NEO's committee/validators cache handling Do not recalculate new committee/validators value in the start of every subsequent epoch. Use values that was calculated in the PostPersist method of the previously processed block in the end of the previous epoch. Signed-off-by: Anna Shaleva --- pkg/core/native/native_neo.go | 90 +++++++++++++++++++---------------- 1 file changed, 49 insertions(+), 41 deletions(-) diff --git a/pkg/core/native/native_neo.go b/pkg/core/native/native_neo.go index 923b50f11..6fbfd4c1b 100644 --- a/pkg/core/native/native_neo.go +++ b/pkg/core/native/native_neo.go @@ -63,8 +63,13 @@ type NeoCache struct { // (every 28 blocks for mainnet). It's value // is always equal to the value stored by `prefixCommittee`. committee keysWithVotes + // newEpochCommittee contains cached committee members updated once per dBFT + // epoch in PostPersist of the last block in the epoch. + newEpochCommittee keysWithVotes // committeeHash contains the script hash of the committee. committeeHash util.Uint160 + // newEpochCommitteeHash contains the script hash of the newEpochCommittee. + newEpochCommitteeHash util.Uint160 // gasPerVoteCache contains the last updated value of GAS per vote reward for candidates. // It is set in state-modifying methods only and read in `PostPersist`, thus is not protected @@ -274,9 +279,13 @@ func (n *NEO) Initialize(ic *interop.Context) error { } cache := &NeoCache{ - gasPerVoteCache: make(map[string]big.Int), - votesChanged: true, - newEpochNextValidators: nil, // will be updated in the last epoch block's PostPersist right before the committee update block. + gasPerVoteCache: make(map[string]big.Int), + votesChanged: true, + // Will be updated in the last epoch block's PostPersist right before the committee update block. + // NeoToken's deployment (and initialization) isn't intended to be performed not-in-genesis block anyway. + newEpochNextValidators: nil, + newEpochCommittee: nil, + newEpochCommitteeHash: util.Uint160{}, } // We need cache to be present in DAO before the subsequent call to `mint`. @@ -319,11 +328,13 @@ func (n *NEO) InitializeCache(blockHeight uint32, d *dao.Simple) error { cache := &NeoCache{ gasPerVoteCache: make(map[string]big.Int), votesChanged: true, - // If it's a block in the middle of dbft epoch, then no one must access - // this field, and it will be updated during the PostPersist of the last - // block in the current epoch. If it's the laast block of the current epoch, + // If it's a block in the middle of dBFT epoch, then no one must access + // these NewEpoch* fields, and they will be updated during the PostPersist of the last + // block in the current epoch. If it's the last block of the current epoch, // then update this cache manually below. newEpochNextValidators: nil, + newEpochCommittee: nil, + newEpochCommitteeHash: util.Uint160{}, } var committee = keysWithVotes{} @@ -338,13 +349,13 @@ func (n *NEO) InitializeCache(blockHeight uint32, d *dao.Simple) error { cache.gasPerBlock = n.getSortedGASRecordFromDAO(d) cache.registerPrice = getIntWithKey(n.ID, d, []byte{prefixRegisterPrice}) - // Update next block validators cache for external users if committee should be + // Update newEpoch* cache for external users if committee should be // updated in the next block. if n.cfg.ShouldUpdateCommitteeAt(blockHeight + 1) { var numOfCNs = n.cfg.GetNumOfCNs(blockHeight + 1) - err := n.updateCachedValidators(d, cache, blockHeight, numOfCNs) + err := n.updateCachedNewEpochValues(d, cache, blockHeight, numOfCNs) if err != nil { - return fmt.Errorf("failed to update next block newEpochNextValidators cache: %w", err) + return fmt.Errorf("failed to update next block newEpoch* cache: %w", err) } } @@ -376,37 +387,28 @@ func (n *NEO) updateCache(cache *NeoCache, cvs keysWithVotes, blockHeight uint32 return nil } -// updateCachedValidators sets newEpochNextValidators cache that will be used by external users -// to retrieve next block validators list of the next dBFT epoch that wasn't yet -// started. Thus, it stores the list of validators computed using the persisted -// blocks state of the latest epoch. -func (n *NEO) updateCachedValidators(d *dao.Simple, cache *NeoCache, blockHeight uint32, numOfCNs int) error { - result, _, err := n.computeCommitteeMembers(blockHeight, d) +// updateCachedNewEpochValues sets newEpochNextValidators, newEpochCommittee and +// newEpochCommitteeHash cache that will be used by external users to retrieve +// next block validators list of the next dBFT epoch that wasn't yet started and +// will be used by corresponding values initialisation on the next epoch start. +// The updated new epoch cached values computed using the persisted blocks state +// of the latest epoch. +func (n *NEO) updateCachedNewEpochValues(d *dao.Simple, cache *NeoCache, blockHeight uint32, numOfCNs int) error { + committee, cvs, err := n.computeCommitteeMembers(blockHeight, d) if err != nil { return fmt.Errorf("failed to compute committee members: %w", err) } - result = result[:numOfCNs] - sort.Sort(result) - cache.newEpochNextValidators = result - return nil -} + cache.newEpochCommittee = cvs -func (n *NEO) updateCommittee(cache *NeoCache, ic *interop.Context) error { - if !cache.votesChanged { - // We need to put in storage anyway, as it affects dumps - ic.DAO.PutStorageItem(n.ID, prefixCommittee, cache.committee.Bytes(ic.DAO.GetItemCtx())) - return nil - } - - _, cvs, err := n.computeCommitteeMembers(ic.BlockHeight(), ic.DAO) + script, err := smartcontract.CreateMajorityMultiSigRedeemScript(committee.Copy()) if err != nil { return err } - if err := n.updateCache(cache, cvs, ic.BlockHeight()); err != nil { - return err - } - cache.votesChanged = false - ic.DAO.PutStorageItem(n.ID, prefixCommittee, cvs.Bytes(ic.DAO.GetItemCtx())) + cache.newEpochCommitteeHash = hash.Hash160(script) + + nextVals := committee[:numOfCNs].Copy() + sort.Sort(nextVals) + cache.newEpochNextValidators = nextVals return nil } @@ -414,15 +416,20 @@ func (n *NEO) updateCommittee(cache *NeoCache, ic *interop.Context) error { func (n *NEO) OnPersist(ic *interop.Context) error { if n.cfg.ShouldUpdateCommitteeAt(ic.Block.Index) { cache := ic.DAO.GetRWCache(n.ID).(*NeoCache) + // Cached newEpoch* values always have proper value set (either by PostPersist + // during the last epoch block handling or by initialization code). oldKeys := cache.nextValidators oldCom := cache.committee if n.cfg.GetNumOfCNs(ic.Block.Index) != len(oldKeys) || n.cfg.GetCommitteeSize(ic.Block.Index) != len(oldCom) { - cache.votesChanged = true - } - if err := n.updateCommittee(cache, ic); err != nil { - return err + cache.nextValidators = cache.newEpochNextValidators + cache.committee = cache.newEpochCommittee + cache.committeeHash = cache.newEpochCommitteeHash + cache.votesChanged = false } + + // We need to put in storage anyway, as it affects dumps + ic.DAO.PutStorageItem(n.ID, prefixCommittee, cache.committee.Bytes(ic.DAO.GetItemCtx())) } return nil } @@ -475,8 +482,9 @@ func (n *NEO) PostPersist(ic *interop.Context) error { } } } - // Update next block newEpochNextValidators cache for external users if committee should be - // updated in the next block. + // Update newEpoch cache for external users and further committee, committeeHash + // and nextBlockValidators cache initialisation if committee should be updated in + // the next block. if n.cfg.ShouldUpdateCommitteeAt(ic.Block.Index + 1) { var ( h = ic.Block.Index // consider persisting block as stored to get _next_ block newEpochNextValidators @@ -486,9 +494,9 @@ func (n *NEO) PostPersist(ic *interop.Context) error { if !isCacheRW { cache = ic.DAO.GetRWCache(n.ID).(*NeoCache) } - err := n.updateCachedValidators(ic.DAO, cache, h, numOfCNs) + err := n.updateCachedNewEpochValues(ic.DAO, cache, h, numOfCNs) if err != nil { - return fmt.Errorf("failed to update next block newEpochNextValidators cache: %w", err) + return fmt.Errorf("failed to update next block newEpoch* cache: %w", err) } } } From d9644204eeb01ce84c3fee93c0dccc5ab5b65504 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 1 Sep 2023 18:15:36 +0300 Subject: [PATCH 7/7] native: make newEpochNextValidators always contain non-empty value If it's the end of epoch, then it contains the updated validators list recalculated during the last block's PostPersist. If it's middle of the epoch, then it contains previously calculated value (value for the previous completed epoch) that is equal to the current nextValidators cache value. Signed-off-by: Anna Shaleva --- pkg/consensus/consensus.go | 28 +++-------------- pkg/core/blockchain.go | 9 ++++-- pkg/core/native/native_neo.go | 57 ++++++++++++++++++----------------- 3 files changed, 40 insertions(+), 54 deletions(-) diff --git a/pkg/consensus/consensus.go b/pkg/consensus/consensus.go index 540bcdb2a..79d965cdf 100644 --- a/pkg/consensus/consensus.go +++ b/pkg/consensus/consensus.go @@ -683,16 +683,9 @@ func (s *service) getValidators(txes ...block.Transaction) []crypto.PublicKey { pKeys, err = s.Chain.GetNextBlockValidators() } else { // getValidators with non-empty args is used by dbft to fill block's - // NextConsensus field, thus should return proper value for NextConsensus. - cfg := s.Chain.GetConfig().ProtocolConfiguration - if cfg.ShouldUpdateCommitteeAt(s.dbft.Context.BlockIndex) { - // Calculate NextConsensus based on the most fresh chain state. - pKeys = s.Chain.ComputeNextBlockValidators() - } else { - // Take the cached validators that are relevant for the current dBFT epoch - // to make NextConsensus. - pKeys, err = s.Chain.GetNextBlockValidators() - } + // NextConsensus field, ComputeNextBlockValidators will return proper + // value for NextConsensus wrt dBFT epoch start/end. + pKeys = s.Chain.ComputeNextBlockValidators() } if err != nil { s.log.Error("error while trying to get validators", zap.Error(err)) @@ -734,20 +727,7 @@ func (s *service) newBlockFromContext(ctx *dbft.Context) block.Block { block.PrevStateRoot = sr.Root } - var validators keys.PublicKeys - var err error - cfg := s.Chain.GetConfig().ProtocolConfiguration - if cfg.ShouldUpdateCommitteeAt(ctx.BlockIndex) { - // Calculate NextConsensus based on the most fresh chain state. - validators = s.Chain.ComputeNextBlockValidators() - } else { - // Take the cached validators that are relevant for the current dBFT epoch - // to make NextConsensus. - validators, err = s.Chain.GetNextBlockValidators() - } - if err != nil { - s.log.Fatal(fmt.Sprintf("failed to get validators: %s", err.Error())) - } + var validators = s.Chain.ComputeNextBlockValidators() script, err := smartcontract.CreateDefaultMultiSigRedeemScript(validators) if err != nil { s.log.Fatal(fmt.Sprintf("failed to create multisignature script: %s", err.Error())) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index ced06626f..df97ada55 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -2698,9 +2698,12 @@ func (bc *Blockchain) GetCommittee() (keys.PublicKeys, error) { } // ComputeNextBlockValidators returns current validators. Validators list -// returned from this method may be updated every block (depending on -// register/unregister/vote calls to NeoToken contract), not only once per -// (committee size) number of blocks. +// returned from this method is updated once per CommitteeSize number of blocks. +// 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. func (bc *Blockchain) ComputeNextBlockValidators() []*keys.PublicKey { return bc.contracts.NEO.ComputeNextBlockValidators(bc.dao) } diff --git a/pkg/core/native/native_neo.go b/pkg/core/native/native_neo.go index 6fbfd4c1b..f9eb2b640 100644 --- a/pkg/core/native/native_neo.go +++ b/pkg/core/native/native_neo.go @@ -139,9 +139,11 @@ func copyNeoCache(src, dst *NeoCache) { // Can safely omit copying because the new array is created each time // newEpochNextValidators list, nextValidators and committee are updated. dst.nextValidators = src.nextValidators - dst.newEpochNextValidators = src.newEpochNextValidators dst.committee = src.committee dst.committeeHash = src.committeeHash + dst.newEpochNextValidators = src.newEpochNextValidators + dst.newEpochCommittee = src.newEpochCommittee + dst.newEpochCommitteeHash = src.newEpochCommitteeHash dst.registerPrice = src.registerPrice @@ -281,11 +283,6 @@ func (n *NEO) Initialize(ic *interop.Context) error { cache := &NeoCache{ gasPerVoteCache: make(map[string]big.Int), votesChanged: true, - // Will be updated in the last epoch block's PostPersist right before the committee update block. - // NeoToken's deployment (and initialization) isn't intended to be performed not-in-genesis block anyway. - newEpochNextValidators: nil, - newEpochCommittee: nil, - newEpochCommitteeHash: util.Uint160{}, } // We need cache to be present in DAO before the subsequent call to `mint`. @@ -317,6 +314,11 @@ func (n *NEO) Initialize(ic *interop.Context) error { setIntWithKey(n.ID, ic.DAO, []byte{prefixRegisterPrice}, DefaultRegisterPrice) cache.registerPrice = int64(DefaultRegisterPrice) + var numOfCNs = n.cfg.GetNumOfCNs(ic.Block.Index + 1) + err = n.updateCachedNewEpochValues(ic.DAO, cache, ic.BlockHeight(), numOfCNs) + if err != nil { + return fmt.Errorf("failed to update next block newEpoch* cache: %w", err) + } return nil } @@ -328,13 +330,6 @@ func (n *NEO) InitializeCache(blockHeight uint32, d *dao.Simple) error { cache := &NeoCache{ gasPerVoteCache: make(map[string]big.Int), votesChanged: true, - // If it's a block in the middle of dBFT epoch, then no one must access - // these NewEpoch* fields, and they will be updated during the PostPersist of the last - // block in the current epoch. If it's the last block of the current epoch, - // then update this cache manually below. - newEpochNextValidators: nil, - newEpochCommittee: nil, - newEpochCommitteeHash: util.Uint160{}, } var committee = keysWithVotes{} @@ -349,14 +344,21 @@ func (n *NEO) InitializeCache(blockHeight uint32, d *dao.Simple) error { cache.gasPerBlock = n.getSortedGASRecordFromDAO(d) cache.registerPrice = getIntWithKey(n.ID, d, []byte{prefixRegisterPrice}) - // Update newEpoch* cache for external users if committee should be - // updated in the next block. + // Update newEpoch* cache for external users. It holds values for the previous + // dBFT epoch if the current one isn't yet finished. if n.cfg.ShouldUpdateCommitteeAt(blockHeight + 1) { var numOfCNs = n.cfg.GetNumOfCNs(blockHeight + 1) err := n.updateCachedNewEpochValues(d, cache, blockHeight, numOfCNs) if err != nil { return fmt.Errorf("failed to update next block newEpoch* cache: %w", err) } + } else { + // nextValidators, committee and committee hash are filled in by this moment + // via n.updateCache call. + cache.newEpochNextValidators = cache.nextValidators.Copy() + cache.newEpochCommittee = make(keysWithVotes, len(cache.committee)) + copy(cache.newEpochCommittee, cache.committee) + cache.newEpochCommitteeHash = cache.committeeHash } d.SetCache(n.ID, cache) @@ -418,15 +420,10 @@ func (n *NEO) OnPersist(ic *interop.Context) error { cache := ic.DAO.GetRWCache(n.ID).(*NeoCache) // Cached newEpoch* values always have proper value set (either by PostPersist // during the last epoch block handling or by initialization code). - oldKeys := cache.nextValidators - oldCom := cache.committee - if n.cfg.GetNumOfCNs(ic.Block.Index) != len(oldKeys) || - n.cfg.GetCommitteeSize(ic.Block.Index) != len(oldCom) { - cache.nextValidators = cache.newEpochNextValidators - cache.committee = cache.newEpochCommittee - cache.committeeHash = cache.newEpochCommitteeHash - cache.votesChanged = false - } + cache.nextValidators = cache.newEpochNextValidators + cache.committee = cache.newEpochCommittee + cache.committeeHash = cache.newEpochCommitteeHash + cache.votesChanged = false // We need to put in storage anyway, as it affects dumps ic.DAO.PutStorageItem(n.ID, prefixCommittee, cache.committee.Bytes(ic.DAO.GetItemCtx())) @@ -490,7 +487,9 @@ func (n *NEO) PostPersist(ic *interop.Context) error { h = ic.Block.Index // consider persisting block as stored to get _next_ block newEpochNextValidators numOfCNs = n.cfg.GetNumOfCNs(h + 1) ) - if cache.newEpochNextValidators == nil || numOfCNs != len(cache.newEpochNextValidators) { + if cache.votesChanged || + numOfCNs != len(cache.newEpochNextValidators) || + n.cfg.GetCommitteeSize(h+1) != len(cache.newEpochCommittee) { if !isCacheRW { cache = ic.DAO.GetRWCache(n.ID).(*NeoCache) } @@ -839,7 +838,9 @@ func (n *NEO) UnregisterCandidateInternal(ic *interop.Context, pub *keys.PublicK return nil } cache := ic.DAO.GetRWCache(n.ID).(*NeoCache) - cache.newEpochNextValidators = nil + // Not only current committee/validators cache is interested in votesChanged, but also + // newEpoch cache, thus, modify votesChanged to update the latter. + cache.votesChanged = true c := new(candidate).FromBytes(si) emitEvent := c.Registered c.Registered = false @@ -1120,7 +1121,9 @@ func (n *NEO) ComputeNextBlockValidators(d *dao.Simple) keys.PublicKeys { return vals.Copy() } // It's a caller's program error to call ComputeNextBlockValidators not having - // the right value in lower cache. + // the right value in lower cache. With the current scheme of handling + // newEpochNextValidators cache this code is expected to be unreachable, but + // let's have this panic here just in case. panic("bug: unexpected external call to newEpochNextValidators cache") }