-
Notifications
You must be signed in to change notification settings - Fork 324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[contractstaking]fix vote bug when change delegate #3888
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3888 +/- ##
==========================================
+ Coverage 75.38% 75.56% +0.18%
==========================================
Files 303 328 +25
Lines 25923 27792 +1869
==========================================
+ Hits 19541 21001 +1460
- Misses 5360 5714 +354
- Partials 1022 1077 +55
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
@@ -140,11 +143,11 @@ func (s *Indexer) BucketTypes() ([]*BucketType, error) { | |||
|
|||
// PutBlock puts a block into indexer | |||
func (s *Indexer) PutBlock(ctx context.Context, blk *block.Block) error { | |||
if blk.Height() < s.contractDeployHeight { | |||
if blk.Height() < s.contractDeployHeight || blk.Height() <= s.height.Load().(uint64) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tho it's guaranteed in the filedao (e.g.
iotex-core/blockchain/filedao/filedao_v2.go
Line 225 in 32de9ce
if blk.Height() != tip.Height+1 { |
@@ -630,3 +955,24 @@ func transfer(r *require.Assertions, handler *contractStakingEventHandler, owner | |||
}) | |||
r.NoError(err) | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are two events Transfer
and Staked
emitted simultaneously in the contract as same as on L898-L956?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
if oldBi != nil { | ||
oldDelegate := oldBi.Delegate.String() | ||
if oldDelegate != newDelegate { | ||
delete(s.candidateBucketMap[oldDelegate], id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will contractStakingCache.merge()
always return nil
?
@@ -372,11 +331,21 @@ func (s *contractStakingCache) putBucketType(id uint64, bt *BucketType) { | |||
} | |||
|
|||
func (s *contractStakingCache) putBucketInfo(id uint64, bi *bucketInfo) { | |||
oldBi := s.bucketInfoMap[id] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests for the issue
func TestContractStakingCache_Debug(t *testing.T) {
cache := newContractStakingCache("")
cache.PutBucketType(1, &BucketType{Amount: big.NewInt(100), Duration: 100, ActivatedAt: 1})
cache.PutBucketType(1, &BucketType{Amount: big.NewInt(100), Duration: 200, ActivatedAt: 1})
cache.PutBucketType(1, &BucketType{Amount: big.NewInt(200), Duration: 100, ActivatedAt: 1})
log.L().Info("debug", zap.Any("propertyBucketTypeMap", cache.propertyBucketTypeMap))
cache.PutBucketInfo(1, &bucketInfo{TypeIndex: 1, CreatedAt: 1, UnlockedAt: maxBlockNumber, UnstakedAt: maxBlockNumber, Delegate: identityset.Address(1), Owner: identityset.Address(2)})
cache.PutBucketInfo(1, &bucketInfo{TypeIndex: 1, CreatedAt: 1, UnlockedAt: maxBlockNumber, UnstakedAt: maxBlockNumber, Delegate: identityset.Address(2), Owner: identityset.Address(3)})
log.L().Info("debug", zap.Any("candidateBucketMap", cache.candidateBucketMap))
}
code for reference
func (s *contractStakingCache) putBucketType(id uint64, bt *BucketType) {
if oldBt, existed := s.bucketTypeMap[id]; existed {
amount := oldBt.Amount.Int64()
if _, existed := s.propertyBucketTypeMap[amount]; !existed {
panic("property bucket type map not found")
}
delete(s.propertyBucketTypeMap[amount], oldBt.Duration)
if len(s.propertyBucketTypeMap[amount]) == 0 {
delete(s.propertyBucketTypeMap, amount)
}
}
s.bucketTypeMap[id] = bt
amount := bt.Amount.Int64()
if _, ok := s.propertyBucketTypeMap[amount]; !ok {
s.propertyBucketTypeMap[amount] = make(map[uint64]uint64)
}
s.propertyBucketTypeMap[amount][bt.Duration] = id
}
func (s *contractStakingCache) putBucketInfo(id uint64, bi *bucketInfo) {
// delete old candidate bucket map
if oldBi, existed := s.bucketInfoMap[id]; existed {
oldDelegate := oldBi.Delegate.String()
if _, existed := s.candidateBucketMap[oldDelegate]; !existed {
panic("candidate bucket map not found") // TODO: remove this panic
}
delete(s.candidateBucketMap[oldDelegate], id)
if len(s.candidateBucketMap[oldDelegate]) == 0 {
delete(s.candidateBucketMap, oldDelegate)
}
}
s.bucketInfoMap[id] = bi
// update candidate bucket map
newDelegate := bi.Delegate.String()
if _, ok := s.candidateBucketMap[newDelegate]; !ok {
s.candidateBucketMap[newDelegate] = make(map[uint64]bool)
}
s.candidateBucketMap[newDelegate][id] = true
}
@@ -24,18 +24,6 @@ func newContractStakingDelta() *contractStakingDelta { | |||
} | |||
} | |||
|
|||
func (s *contractStakingDelta) PutHeight(height uint64) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the design map[deltaState]map[uint64]*bucketInfo
is misleading on L27
if err := s.cache.Merge(delta); err != nil { | ||
s.reloadCache() | ||
return err | ||
} | ||
// update db |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new state at old height without lock?
return err | ||
} | ||
s.putTotalBucketCount(s.totalBucketCount + delta.AddedBucketCnt()) | ||
return nil | ||
} | ||
|
||
func (s *contractStakingCache) PutTotalBucketCount(count uint64) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the purpose of a public PutTotalBucketCount
func?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the two principles could easily avoid dead lock:
Lock & Unlock
is called in public method instead of private- public method can't call public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems only used in test code, could be removed
@@ -140,11 +143,11 @@ func (s *Indexer) BucketTypes() ([]*BucketType, error) { | |||
|
|||
// PutBlock puts a block into indexer | |||
func (s *Indexer) PutBlock(ctx context.Context, blk *block.Block) error { | |||
if blk.Height() < s.contractDeployHeight { | |||
if blk.Height() < s.contractDeployHeight || blk.Height() <= s.height.Load().(uint64) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All buckets are returned from Indexer.Buckets()
, Indexer.TotalBucketCount()
and Indexer.BucketsByCandidate()
, but only the active buckets are returned from Indexer.BucketTypes()
. Is this expected?
Description
The issue is that when
changeDelegate
is called on a bucket, the votes of the old delegate are not reduced.The issue is caused by two factors:
candidateBucketMap
cache
read operations return a pointer, which means that the clean cache may have already been updated before themerge
.pre-required: #3887
Fixes #(issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: