Skip to content
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.

Sign up for GitHub

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

fix test and speedup validation #3842

Merged
merged 3 commits into from
Apr 10, 2023
Merged

fix test and speedup validation #3842

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d3a165
fix test and speedup validation
Liuhaai Feb 14, 2023
270b507
checkNonceContinuity()
dustinxie Apr 6, 2023
0594a62
special handling for genesis block not needed
dustinxie Apr 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions action/protocol/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ type (
CorrectGasRefund bool
FixRewardErroCheckPosition bool
EnableWeb3Rewarding bool
EnableNodeInfo bool
SkipSystemActionNonce bool
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EnableNodeInfo is not used at all

}

// FeatureWithHeightCtx provides feature check functions.
Expand Down Expand Up @@ -247,7 +247,7 @@ func WithFeatureCtx(ctx context.Context) context.Context {
CorrectGasRefund: g.IsOkhotsk(height),
FixRewardErroCheckPosition: g.IsOkhotsk(height),
EnableWeb3Rewarding: g.IsPalau(height),
EnableNodeInfo: g.IsPalau(height),
SkipSystemActionNonce: g.IsPalau(height),
},
)
}
Expand Down
1 change: 0 additions & 1 deletion blockchain/integrity/benchmark_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,6 @@ func newChainInDB() (blockchain.Blockchain, actpool.ActPool, error) {
var genesisNonce uint64 = 0

// make a transfer from genesisAccount to a and b,because stateTX cannot store data in height 0
genesisNonce++
tsf, err := action.SignedTransfer(userA.String(), genesisPriKey, genesisNonce, big.NewInt(1e17), []byte{}, testutil.TestGasLimit, big.NewInt(testutil.TestGasPriceInt64))
if err != nil {
return nil, nil, err
Expand Down
101 changes: 94 additions & 7 deletions blockchain/integrity/integrity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1466,9 +1466,6 @@ func TestBlockchain_AccountState(t *testing.T) {
require := require.New(t)

cfg := config.Default
// disable account-based testing
// create chain
cfg.Genesis.InitBalanceMap[identityset.Address(0).String()] = "100"
ctx := genesis.WithGenesisContext(context.Background(), cfg.Genesis)
registry := protocol.NewRegistry()
acc := account.NewProtocol(rewarding.DepositGas)
Expand All @@ -1480,14 +1477,104 @@ func TestBlockchain_AccountState(t *testing.T) {
require.NoError(err)
dao := blockdao.NewBlockDAOInMemForTest([]blockdao.BlockIndexer{sf})
bc := blockchain.NewBlockchain(cfg.Chain, cfg.Genesis, dao, factory.NewMinter(sf, ap))
require.NoError(bc.Start(context.Background()))
require.NoError(bc.Start(ctx))
require.NotNil(bc)
defer func() {
require.NoError(bc.Stop(ctx))
}()
s, err := accountutil.AccountState(ctx, sf, identityset.Address(0))
require.NoError(err)
require.Equal(uint64(1), s.PendingNonce())
require.Equal(big.NewInt(100), s.Balance)
require.Equal(hash.ZeroHash256, s.Root)
require.Equal([]byte(nil), s.CodeHash)
require.Equal(unit.ConvertIotxToRau(100000000), s.Balance)
require.Zero(s.Root)
require.Nil(s.CodeHash)
}

func TestNewAccountAction(t *testing.T) {
require := require.New(t)

cfg := config.Default
cfg.Genesis.OkhotskBlockHeight = 1
ctx := genesis.WithGenesisContext(context.Background(), cfg.Genesis)
registry := protocol.NewRegistry()
acc := account.NewProtocol(rewarding.DepositGas)
require.NoError(acc.Register(registry))
factoryCfg := factory.GenerateConfig(cfg.Chain, cfg.Genesis)
sf, err := factory.NewFactory(factoryCfg, db.NewMemKVStore(), factory.RegistryOption(registry))
require.NoError(err)
ap, err := actpool.NewActPool(cfg.Genesis, sf, cfg.ActPool)
require.NoError(err)
dao := blockdao.NewBlockDAOInMemForTest([]blockdao.BlockIndexer{sf})
bc := blockchain.NewBlockchain(cfg.Chain, cfg.Genesis, dao, factory.NewMinter(sf, ap))
require.NoError(bc.Start(ctx))
require.NotNil(bc)
defer func() {
require.NoError(bc.Stop(ctx))
}()

// create a new address, transfer 4 IOTX
newSk, err := iotexcrypto.HexStringToPrivateKey("55499c1b09f687488af9e4ee9e2bd53c7c8c3ddc69d4d9345a04b13030cffabe")
require.NoError(err)
newAddr := newSk.PublicKey().Address()
tx, err := action.SignedTransfer(newAddr.String(), identityset.PrivateKey(0), 1, big.NewInt(4*unit.Iotx), nil, testutil.TestGasLimit, testutil.TestGasPrice)
require.NoError(err)
require.NoError(ap.Add(ctx, tx))
blk, err := bc.MintNewBlock(testutil.TimestampNow())
require.NoError(err)
require.NoError(bc.CommitBlock(blk))
ap.Reset()

// initiate transfer from new address
tx, err = action.SignedTransfer(identityset.Address(0).String(), newSk, 0, big.NewInt(unit.Iotx), nil, testutil.TestGasLimit, testutil.TestGasPrice)
require.NoError(err)
require.NoError(ap.Add(ctx, tx))
tx1, err := action.SignedTransfer(identityset.Address(1).String(), newSk, 1, big.NewInt(unit.Iotx), nil, testutil.TestGasLimit, testutil.TestGasPrice)
require.NoError(err)
require.NoError(ap.Add(ctx, tx1))
blk1, err := bc.MintNewBlock(testutil.TimestampNow())
require.NoError(err)
require.NoError(bc.CommitBlock(blk1))
ap.Reset()

// commit 2 blocks into a new chain
for _, validateNonce := range []bool{false, true} {
if validateNonce {
cfg.Genesis.PalauBlockHeight = 2
} else {
cfg.Genesis.PalauBlockHeight = 20
}
ctx = genesis.WithGenesisContext(context.Background(), cfg.Genesis)
factoryCfg = factory.GenerateConfig(cfg.Chain, cfg.Genesis)
sf1, err := factory.NewFactory(factoryCfg, db.NewMemKVStore(), factory.RegistryOption(registry))
require.NoError(err)
dao1 := blockdao.NewBlockDAOInMemForTest([]blockdao.BlockIndexer{sf1})
bc1 := blockchain.NewBlockchain(cfg.Chain, cfg.Genesis, dao1, factory.NewMinter(sf1, ap))
require.NoError(bc1.Start(ctx))
require.NotNil(bc1)
defer func() {
require.NoError(bc1.Stop(ctx))
}()
require.NoError(bc1.CommitBlock(blk))
err = bc1.CommitBlock(blk1)
if validateNonce {
require.NoError(err)
} else {
require.Equal(action.ErrNonceTooHigh, errors.Cause(err))
}

// verify new addr
s, err := accountutil.AccountState(ctx, sf1, newAddr)
require.NoError(err)
if validateNonce {
require.EqualValues(2, s.PendingNonce())
require.Equal(big.NewInt(2*unit.Iotx), s.Balance)
} else {
require.Zero(s.PendingNonce())
require.Equal(big.NewInt(4*unit.Iotx), s.Balance)
}
require.Zero(s.Root)
require.Nil(s.CodeHash)
}
}

func TestBlocks(t *testing.T) {
Expand Down
42 changes: 38 additions & 4 deletions state/factory/workingset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,21 +338,49 @@ func (ws *workingSet) validateNonce(ctx context.Context, blk *block.Block) error
if blk.Height() == 0 {
return nil
}
return ws.checkNonceContinuity(ctx, accountNonceMap)
}

func (ws *workingSet) validateNonce2(ctx context.Context, blk *block.Block) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better name than validateNonce2?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

accountNonceMap := make(map[string][]uint64)
for _, selp := range blk.Actions {
if action.IsSystemAction(selp) {
continue
}

caller := selp.SenderAddress()
if caller == nil {
return errors.New("failed to get address")
}
srcAddr := caller.String()
if _, ok := accountNonceMap[srcAddr]; !ok {
accountNonceMap[srcAddr] = make([]uint64, 0)
}
accountNonceMap[srcAddr] = append(accountNonceMap[srcAddr], selp.Nonce())
}

// Special handling for genesis block
if blk.Height() == 0 {
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicated with L338-L340, moved into checkNonceContinuity()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested it's actually not needed

}
return ws.checkNonceContinuity(ctx, accountNonceMap)
}

func (ws *workingSet) checkNonceContinuity(ctx context.Context, accountNonceMap map[string][]uint64) error {
// Verify each account's Nonce
for srcAddr, receivedNonces := range accountNonceMap {
addr, _ := address.FromString(srcAddr)
confirmedState, err := accountutil.AccountState(ctx, ws, addr)
if err != nil {
return errors.Wrapf(err, "failed to get the confirmed nonce of address %s", srcAddr)
}
receivedNonces := receivedNonces
sort.Slice(receivedNonces, func(i, j int) bool { return receivedNonces[i] < receivedNonces[j] })
pendingNonce := confirmedState.PendingNonce()
for i, nonce := range receivedNonces {
if nonce != pendingNonce+uint64(i) {
return errors.Wrapf(
action.ErrNonceTooHigh,
"the %d nonce %d of address %s (init pending nonce %d) is not continuously increasing",
"the %d-th nonce %d of address %s (init pending nonce %d) is not continuously increasing",
i,
nonce,
srcAddr,
Expand Down Expand Up @@ -517,8 +545,14 @@ func updateReceiptIndex(receipts []*action.Receipt) {
}

func (ws *workingSet) ValidateBlock(ctx context.Context, blk *block.Block) error {
if err := ws.validateNonce(ctx, blk); err != nil {
return errors.Wrap(err, "failed to validate nonce")
if protocol.MustGetFeatureCtx(ctx).SkipSystemActionNonce {
if err := ws.validateNonce2(ctx, blk); err != nil {
return errors.Wrap(err, "failed to validate nonce")
}
} else {
if err := ws.validateNonce(ctx, blk); err != nil {
return errors.Wrap(err, "failed to validate nonce")
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block 1 has 2 tx, from the same sender io1vtm2zgn830pn6auc2cvnchgwdaefa9gr4z0s86, both with nonce=0, one is a transfer, the other is grant reward (system action)

validateNonce2() will ignore the grant reward, but return error for the transfer tx (since it's expecting nonce=1 but got nonce=0), so we have to keep the height check and 2 validate()

}
if err := ws.process(ctx, blk.RunnableActions().Actions()); err != nil {
log.L().Error("Failed to update state.", zap.Uint64("height", ws.height), zap.Error(err))
Expand Down
2 changes: 1 addition & 1 deletion testutil/timestamp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ import (
func TestTimestamp(t *testing.T) {
assert := assert.New(t)
time1 := TimestampNow()
assert.True(time.Now().After(time1))
assert.False(time.Now().Before(time1))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sometimes it happens that time.Now() == time1, causing CI test to fail

}