[blockindex] restrict height for contract_staking_indexer during read operation #3927

envestcc · 2023-08-22T13:40:48Z

Description

Motivation:
Strictly limiting the blocks during PutBlock can avoid block skipping issues
Specifying the height during queries can help detect errors in the indexer at an earlier stage
Fixes #(issue)

Type of change

Please delete options that are not relevant.

[] Bug fix (non-breaking change which fixes an issue)
[] New feature (non-breaking change which adds functionality)
Code refactor or improvement
[] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)
[] This change requires a documentation update

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

make test
[] fullsync
[] Other test (please specify)

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

codecov · 2023-08-22T13:57:47Z

Codecov Report

Merging #3927 (e8343f2) into master (e1f0636) will increase coverage by 0.66%.
Report is 81 commits behind head on master.
The diff coverage is 73.82%.

❗ Current head e8343f2 differs from pull request most recent head 4ab8f36. Consider uploading reports for the commit 4ab8f36 to get more accurate results

@@            Coverage Diff             @@
##           master    #3927      +/-   ##
==========================================
+ Coverage   75.38%   76.04%   +0.66%     
==========================================
  Files         303      328      +25     
  Lines       25923    27897    +1974     
==========================================
+ Hits        19541    21215    +1674     
- Misses       5360     5590     +230     
- Partials     1022     1092      +70

Files Changed	Coverage Δ
action/protocol/execution/evm/evm.go	`43.52% <0.00%> (-2.95%)`	⬇️
action/protocol/execution/evm/evmstatedbadapter.go	`66.77% <ø> (ø)`
action/protocol/poll/consortium.go	`0.00% <0.00%> (ø)`
action/protocol/poll/staking_committee.go	`43.85% <0.00%> (ø)`
...tion/protocol/staking/contractstake_bucket_type.go	`0.00% <0.00%> (ø)`
action/receipt.go	`82.17% <ø> (ø)`
api/grpcserver.go	`86.40% <0.00%> (ø)`
api/loglistener.go	`25.00% <0.00%> (ø)`
api/websocket.go	`4.10% <0.00%> (-1.07%)`	⬇️
blockchain/config.go	`74.54% <ø> (ø)`
... and 65 more

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

dustinxie · 2023-08-22T23:29:26Z

blockindex/contractstaking/cache.go

+		return nil
+	}
+	// indexer only store latest height
+	if height != s.height {


should be height > s.height?

in action/protocol/poll/util.go, it is called with epoch start height

func createPostSystemActions(ctx context.Context, sr protocol.StateReader, p Protocol) { xxx l, err := p.CalculateCandidatesByHeight(ctx, sr, epochHeight) xxx }

dustinxie · 2023-08-22T23:34:07Z

blockindex/contractstaking/dirty_cache.go

@@ -105,7 +105,7 @@ func (dirty *contractStakingDirty) finalize() (batch.KVStoreBatch, *contractStak

 func (dirty *contractStakingDirty) finalizeBatch() batch.KVStoreBatch {
 	dirty.once.Do(func() {
-		total := dirty.clean.TotalBucketCount() + dirty.delta.AddedBucketCnt()
+		total := dirty.clean.totalBucketCount + dirty.delta.AddedBucketCnt()


not needed?
TotalBucketCount() has a lock inside

ok, use 0 height to query latest state

dustinxie · 2023-08-22T23:34:16Z

blockindex/contractstaking/dirty_cache.go

@@ -125,7 +125,7 @@ func (dirty *contractStakingDirty) matchBucketType(amount *big.Int, duration uin
 }

 func (dirty *contractStakingDirty) getBucketTypeCount() uint64 {
-	return dirty.clean.BucketTypeCount() + dirty.delta.AddedBucketTypeCnt()
+	return uint64(len(dirty.clean.bucketTypeMap)) + dirty.delta.AddedBucketTypeCnt()


same as above

dustinxie · 2023-08-22T23:41:34Z

action/protocol/staking/protocol.go

@@ -476,7 +476,11 @@ func (p *Protocol) ActiveCandidates(ctx context.Context, sr protocol.StateReader
 	featureCtx := protocol.MustGetFeatureCtx(ctx)
 	for i := range list {
 		if p.contractStakingIndexer != nil && featureCtx.AddContractStakingVotes {
-			list[i].Votes.Add(list[i].Votes, p.contractStakingIndexer.CandidateVotes(list[i].Owner))
+			contractVotes, err := p.contractStakingIndexer.CandidateVotes(list[i].Owner, height)


height here equals to the height of block to be committed, that is, current factory/indexer's height + 1
so should use height-1? pls confirm

should be covered by unit test

dustinxie · 2023-08-22T23:47:14Z

blockindex/contractstaking/cache.go

+		return errors.Wrapf(ErrInvalidHeight, "expected %d, actual %d", s.height, height)
+	}
+	return nil
+}


add test cases to test ErrInvalidHeight

dustinxie · 2023-08-22T23:48:16Z

e2etest/contract_staking_test.go

@@ -1342,7 +1342,7 @@ func TestContractStaking(t *testing.T) {
 		receipts, blk := writeContract(bc, sf, dao, ap, []*callParam{&param}, r)
 		r.Len(receipts, 1)
 		r.EqualValues(iotextypes.ReceiptStatus_Success, receipts[0].Status)
-		buckets, err := indexer.Buckets()
+		buckets, err := indexer.Buckets(blk.Height())


if blk.Height() == indexer height + 1, it should fire an error here

block has been committed after writeContract

dustinxie · 2023-08-24T01:56:56Z

action/protocol/staking/protocol.go

@@ -467,6 +467,10 @@ func (p *Protocol) Validate(ctx context.Context, act action.Action, sr protocol.

 // ActiveCandidates returns all active candidates in candidate center
 func (p *Protocol) ActiveCandidates(ctx context.Context, sr protocol.StateReader, height uint64) (state.CandidateList, error) {
+	height, err := sr.Height()


height will override the 3rd input param, use a diff name?

dustinxie · 2023-08-24T04:05:38Z

action/protocol/staking/contractstake_indexer.go

-		BucketTypes() ([]*ContractStakingBucketType, error)
+		BucketTypes(height uint64) ([]*ContractStakingBucketType, error)
+		// Height returns the indexer height
+		Height() (uint64, error)


not needed?

yes, forget to revert

dustinxie · 2023-08-24T04:09:22Z

blockindex/contractstaking/indexer_test.go

+				noErr(indexer.Buckets(h))
+				noErr(indexer.BucketTypes(h))
+				noErr(indexer.BucketsByCandidate(delegate1, h))
+				noErr(indexer.BucketsByIndices([]uint64{1, 2, 3, 4, 5, 8}, h))


just use r.NoErr()

it does not work for function with multiple return values

dustinxie · 2023-08-24T04:09:47Z

blockindex/contractstaking/indexer_test.go

+				noErr(indexer.Bucket(1, h))
+				noErr(indexer.TotalBucketCount(h))
+			} else {
+				hasErr(indexer.Buckets(h))


just use r.ErrorIs(err, ErrInvalidHeight)

this can simplify the error checking only in one line

not significant/necessary, other places also have similar test code

_, err := func(x, y, z) r.Error(err)

also, no need to test all these funcs, they just call the same validateHeight()
only need to call cache.validateHeight() on these test cases

and move to cache_test.go

sonarcloud · 2023-08-24T05:11:15Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
7.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Liuhaai · 2023-08-24T21:07:09Z

blockindex/contractstaking/cache.go

+		return nil, false, err
+	}
+	bt, ok := s.getBucket(id)
+	return bt, ok, nil


BucketInfo and BucketType without height?

Currently, it is only necessary to add height restrictions to the indexer's API

envestcc added 2 commits August 22, 2023 17:14

add height limit for contract staking indexer

6f3b8fd

update tests

a5fcd3a

envestcc requested review from CoderZhi, dustinxie, Liuhaai and a team as code owners August 22, 2023 13:40

dustinxie reviewed Aug 22, 2023

View reviewed changes

address comments

0f88d0e

dustinxie reviewed Aug 24, 2023

View reviewed changes

address comments

07e537b

dustinxie reviewed Aug 24, 2023

View reviewed changes

envestcc added 2 commits August 24, 2023 12:34

address comments

f10a948

address comments

4ab8f36

dustinxie approved these changes Aug 24, 2023

View reviewed changes

Liuhaai reviewed Aug 24, 2023

View reviewed changes

Liuhaai approved these changes Aug 25, 2023

View reviewed changes

dustinxie merged commit 1a63b13 into iotexproject:master Aug 25, 2023
3 of 4 checks passed

envestcc mentioned this pull request Aug 27, 2023

[blockindex] return empty result when contract not deployed #3928

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[blockindex] restrict height for contract_staking_indexer during read operation #3927

[blockindex] restrict height for contract_staking_indexer during read operation #3927

envestcc commented Aug 22, 2023

codecov bot commented Aug 22, 2023 •

edited

Loading

dustinxie Aug 22, 2023 •

edited

Loading

dustinxie Aug 22, 2023

envestcc Aug 23, 2023

dustinxie Aug 22, 2023

dustinxie Aug 22, 2023

CoderZhi Aug 23, 2023

dustinxie Aug 22, 2023

dustinxie Aug 22, 2023

envestcc Aug 23, 2023

dustinxie Aug 24, 2023

dustinxie Aug 24, 2023

envestcc Aug 24, 2023

dustinxie Aug 24, 2023

envestcc Aug 24, 2023

dustinxie Aug 24, 2023

envestcc Aug 24, 2023

dustinxie Aug 24, 2023 •

edited

Loading

dustinxie Aug 24, 2023 •

edited

Loading

dustinxie Aug 24, 2023 •

edited

Loading

sonarcloud bot commented Aug 24, 2023

Liuhaai Aug 24, 2023 •

edited

Loading

envestcc Aug 25, 2023 •

edited

Loading

[blockindex] restrict height for contract_staking_indexer during read operation #3927

[blockindex] restrict height for contract_staking_indexer during read operation #3927

Conversation

envestcc commented Aug 22, 2023

Description

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

dustinxie Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinxie Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

dustinxie Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

dustinxie Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Aug 24, 2023

Liuhaai Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

envestcc Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2023 •

edited

Loading

dustinxie Aug 22, 2023 •

edited

Loading

dustinxie Aug 24, 2023 •

edited

Loading

dustinxie Aug 24, 2023 •

edited

Loading

dustinxie Aug 24, 2023 •

edited

Loading

Liuhaai Aug 24, 2023 •

edited

Loading

envestcc Aug 25, 2023 •

edited

Loading