From 3b11f98cd02c0ddd2c629c788d0bee15ec55af61 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 21 Nov 2023 10:49:05 +0300 Subject: [PATCH] core: review usages of (*intero.Context).BlockHeight method This method returns persisted block height and doesn't take into account persisting block height. Some of the callers of this method relay on the wrong assumption that BlockHeight() returns persisting block index. Fix improper usages of this method and adjust tests. Ref. https://github.com/neo-project/neo/blob/61a066583eef53c8eb7b770dcc3deb0305ab11db/src/Neo/SmartContract/ApplicationEngine.cs#L634. Signed-off-by: Anna Shaleva --- docs/notary.md | 2 +- pkg/core/interop/context.go | 8 +++++--- pkg/core/interop/contract/account_test.go | 6 ++++-- pkg/core/interop/runtime/ext_test.go | 10 ++++++++-- pkg/core/native/designate.go | 2 +- pkg/core/native/interop.go | 2 +- pkg/core/native/native_test/gas_test.go | 4 ++-- pkg/core/native/native_test/notary_test.go | 4 ++-- pkg/core/native/notary.go | 7 ++++--- 9 files changed, 28 insertions(+), 17 deletions(-) diff --git a/docs/notary.md b/docs/notary.md index b75d40a45..bd68bef1c 100644 --- a/docs/notary.md +++ b/docs/notary.md @@ -81,7 +81,7 @@ It exposes several methods to the outside world: | Method | Parameters | Return value | Description | | --- | --- | --- | --- | | `onNEP17Payment` | `from` (uint160) - GAS sender account.
`amount` (int) - amount of GAS to deposit.
`data` represents array of two parameters:
1. `to` (uint160) - account of the deposit owner.
2. `till` (int) - deposit lock height. | `bool` | Automatically called after GAS transfer to Notary native contract address and records deposited amount as belonging to `to` address with a lock till `till` chain's height. Can only be invoked from native GAS contract. Must be witnessed by `from`. `to` can be left unspecified (null), with a meaning that `to` is the same address as `from`. `amount` can't be less than 2×`FEE` for the first deposit call for the `to` address. Each successive deposit call must have `till` value equal to or more than the previous successful call (allowing for renewal), if it has additional amount of GAS it adds up to the already deposited value.| -| `lockDepositUntil` | `address` (uint160) - account of the deposit owner.
`till` (int) - new height deposit is valid until (can't be less than previous value). | `void` | Updates deposit expiration value. Must be witnessed by `address`. | +| `lockDepositUntil` | `address` (uint160) - account of the deposit owner.
`till` (int) - new height deposit is valid until (can't be less than previous value and can't be less than the height when transaction is accepted to the chain plus one). | `void` | Updates deposit expiration value. Must be witnessed by `address`. | | `withdraw` | `from` (uint160) - account of the deposit owner.
`to` (uint160) - account to transfer GAS to. | `bool` | Sends all deposited GAS for `from` address to `to` address. Must be witnessed by `from`. `to` can be left unspecified (null), with a meaning that `to` is the same address as `from`. It can only be successful if the lock has already expired, attempting to withdraw the deposit before that height fails. Partial withdrawal is not supported. Returns boolean result, `true` for successful calls and `false` for failed ones. | | `balanceOf` | `addr` (uint160) - account of the deposit owner. | `int` | Returns deposited GAS amount for specified address (integer). | | `expirationOf` | `addr` (uint160) - account of the deposit owner. | `int` | Returns deposit lock height for specified address (integer). | diff --git a/pkg/core/interop/context.go b/pkg/core/interop/context.go index 778a1cade..0aa71c29a 100644 --- a/pkg/core/interop/context.go +++ b/pkg/core/interop/context.go @@ -371,7 +371,9 @@ func (ic *Context) Exec() error { return ic.VM.Run() } -// BlockHeight returns current block height got from Context's block if it's set. +// BlockHeight returns the latest persisted and stored block height/index. +// Persisting block index is not taken into account. If Context's block is set, +// then BlockHeight calculations relies on persisting block index. func (ic *Context) BlockHeight() uint32 { if ic.Block != nil { return ic.Block.Index - 1 // Persisting block is not yet stored. @@ -393,7 +395,7 @@ func (ic *Context) GetBlock(hash util.Uint256) (*block.Block, error) { if err != nil { return nil, err } - if block.Index > ic.BlockHeight() { + if block.Index > ic.BlockHeight() { // persisting block is not reachable. return nil, storage.ErrKeyNotFound } return block, nil @@ -403,7 +405,7 @@ func (ic *Context) GetBlock(hash util.Uint256) (*block.Block, error) { func (ic *Context) IsHardforkEnabled(hf config.Hardfork) bool { height, ok := ic.Hardforks[hf.String()] if ok { - return ic.BlockHeight() >= height + return (ic.BlockHeight() + 1) >= height // persisting block should be taken into account. } // Completely rely on proper hardforks initialisation made by core.NewBlockchain. return false diff --git a/pkg/core/interop/contract/account_test.go b/pkg/core/interop/contract/account_test.go index bedb97730..e03a4ea1e 100644 --- a/pkg/core/interop/contract/account_test.go +++ b/pkg/core/interop/contract/account_test.go @@ -117,11 +117,12 @@ func TestCreateMultisigAccount(t *testing.T) { }) } -func TestCreateAccount_Hardfork(t *testing.T) { +func TestCreateAccount_HFAspidochelone(t *testing.T) { + const enabledHeight = 3 bc, acc := chain.NewSingleWithCustomConfig(t, func(c *config.Blockchain) { c.P2PSigExtensions = true // `basicchain.Init` requires Notary enabled c.Hardforks = map[string]uint32{ - config.HFAspidochelone.String(): 2, + config.HFAspidochelone.String(): enabledHeight, } }) e := neotest.NewExecutor(t, bc, acc, acc) @@ -161,6 +162,7 @@ func TestCreateAccount_Hardfork(t *testing.T) { e.CheckHalt(t, tx2Multisig.Hash()) // block #3: updated prices (larger than the previous ones) + require.Equal(t, uint32(enabledHeight-1), e.Chain.BlockHeight()) tx3Standard := createAccTx(t, standardScript) tx3Multisig := createAccTx(t, multisigScript) e.AddNewBlock(t, tx3Standard, tx3Multisig) diff --git a/pkg/core/interop/runtime/ext_test.go b/pkg/core/interop/runtime/ext_test.go index c0764fd9f..d33c1043f 100644 --- a/pkg/core/interop/runtime/ext_test.go +++ b/pkg/core/interop/runtime/ext_test.go @@ -691,11 +691,14 @@ func TestNotify(t *testing.T) { } func TestSystemRuntimeNotify_HFBasilisk(t *testing.T) { - const ntfName = "Hello, world!" + const ( + ntfName = "Hello, world!" + enabledHeight = 3 + ) bc, acc := chain.NewSingleWithCustomConfig(t, func(c *config.Blockchain) { c.Hardforks = map[string]uint32{ - config.HFBasilisk.String(): 2, + config.HFBasilisk.String(): enabledHeight, } }) e := neotest.NewExecutor(t, bc, acc, acc) @@ -738,6 +741,8 @@ func TestSystemRuntimeNotify_HFBasilisk(t *testing.T) { } ctrInv := e.NewInvoker(ctr.Hash, e.Validator) + // Block 0 is genesis. + // Block 1: deploy contract. e.DeployContract(t, ctr, nil) @@ -745,6 +750,7 @@ func TestSystemRuntimeNotify_HFBasilisk(t *testing.T) { ctrInv.Invoke(t, nil, "main") // Block 3: bad event should fault the execution. + require.Equal(t, uint32(enabledHeight-1), e.Chain.BlockHeight()) ctrInv.InvokeFail(t, "System.Runtime.Notify failed: notification Hello, world! is invalid: parameter 0 type mismatch: Integer (manifest) vs Boolean (notification)", "main") diff --git a/pkg/core/native/designate.go b/pkg/core/native/designate.go index 9cac6c7c1..0e295d593 100644 --- a/pkg/core/native/designate.go +++ b/pkg/core/native/designate.go @@ -204,7 +204,7 @@ func (s *Designate) getDesignatedByRole(ic *interop.Context, args []stackitem.It panic(ErrInvalidIndex) } index := ind.Uint64() - if index > uint64(ic.BlockHeight()+1) { + if index > uint64(ic.BlockHeight()+1) { // persisting block should be taken into account. panic(ErrInvalidIndex) } pubs, _, err := s.GetDesignatedByRole(ic.DAO, r, uint32(index)) diff --git a/pkg/core/native/interop.go b/pkg/core/native/interop.go index 00f0f3c73..7f250b4a7 100644 --- a/pkg/core/native/interop.go +++ b/pkg/core/native/interop.go @@ -34,7 +34,7 @@ func Call(ic *interop.Context) error { if len(history) == 0 { return fmt.Errorf("native contract %s is disabled", meta.Name) } - if history[0] > ic.BlockHeight() { + if history[0] > ic.BlockHeight() { // persisting block must not be taken into account. return fmt.Errorf("native contract %s is active after height = %d", meta.Name, history[0]) } m, ok := meta.GetMethodByOffset(ic.VM.Context().IP()) diff --git a/pkg/core/native/native_test/gas_test.go b/pkg/core/native/native_test/gas_test.go index e92f9097b..05a2f83dd 100644 --- a/pkg/core/native/native_test/gas_test.go +++ b/pkg/core/native/native_test/gas_test.go @@ -90,9 +90,9 @@ func TestGAS_RewardWithP2PSigExtensionsEnabled(t *testing.T) { e.CheckGASBalance(t, notaryNode.GetScriptHash(), big.NewInt(0)) } - // deposit GAS for `signer` with lock until the next block + // deposit GAS for `signer` with lock until the next block (inclusively) depositAmount := 100_0000 + (2+int64(nKeys))*notaryServiceFeePerKey // sysfee + netfee of the next transaction - gasCommitteeInvoker.Invoke(t, true, "transfer", e.CommitteeHash, notaryHash, depositAmount, []any{e.CommitteeHash, e.Chain.BlockHeight() + 1}) + gasCommitteeInvoker.Invoke(t, true, "transfer", e.CommitteeHash, notaryHash, depositAmount, []any{e.CommitteeHash, e.Chain.BlockHeight() + 2}) // save initial GAS total supply getGASTS := func(t *testing.T) int64 { diff --git a/pkg/core/native/native_test/notary_test.go b/pkg/core/native/native_test/notary_test.go index 1f846cd3e..9f08f6333 100644 --- a/pkg/core/native/native_test/notary_test.go +++ b/pkg/core/native/native_test/notary_test.go @@ -197,12 +197,12 @@ func TestNotary_NotaryNodesReward(t *testing.T) { e.CheckGASBalance(t, notaryNode.GetScriptHash(), big.NewInt(0)) } - // deposit GAS for `signer` with lock until the next block + // deposit GAS for `signer` with lock until the next block inclusively depositAmount := 100_0000 + (2+int64(nKeys))*feePerKey // sysfee + netfee of the next transaction if !spendFullDeposit { depositAmount += 1_0000 } - gasCommitteeInvoker.Invoke(t, true, "transfer", multisigHash, notaryHash, depositAmount, ¬ary.OnNEP17PaymentData{Account: &multisigHash, Till: e.Chain.BlockHeight() + 1}) + gasCommitteeInvoker.Invoke(t, true, "transfer", multisigHash, notaryHash, depositAmount, ¬ary.OnNEP17PaymentData{Account: &multisigHash, Till: e.Chain.BlockHeight() + 2}) // send transaction with Notary contract as a sender tx := transaction.New([]byte{byte(opcode.PUSH1)}, 1_000_000) diff --git a/pkg/core/native/notary.go b/pkg/core/native/notary.go index f58e628a7..75224f2d0 100644 --- a/pkg/core/native/notary.go +++ b/pkg/core/native/notary.go @@ -232,8 +232,8 @@ func (n *Notary) onPayment(ic *interop.Context, args []stackitem.Item) stackitem currentHeight := ic.BlockHeight() deposit := n.GetDepositFor(ic.DAO, to) till := toUint32(additionalParams[1]) - if till < currentHeight { - panic(fmt.Errorf("`till` shouldn't be less then the chain's height %d", currentHeight)) + if till < currentHeight+2 { + panic(fmt.Errorf("`till` shouldn't be less then the chain's height + 1 (%d at min)", currentHeight+2)) } if deposit != nil && till < deposit.Till { panic(fmt.Errorf("`till` shouldn't be less then the previous value %d", deposit.Till)) @@ -272,7 +272,7 @@ func (n *Notary) lockDepositUntil(ic *interop.Context, args []stackitem.Item) st return stackitem.NewBool(false) } till := toUint32(args[1]) - if till < ic.BlockHeight() { + if till < (ic.BlockHeight() + 1 + 1) { // deposit can't expire at the current persisting block. return stackitem.NewBool(false) } deposit := n.GetDepositFor(ic.DAO, addr) @@ -308,6 +308,7 @@ func (n *Notary) withdraw(ic *interop.Context, args []stackitem.Item) stackitem. if deposit == nil { return stackitem.NewBool(false) } + // Allow withdrawal only after `till` block was persisted, thus, use ic.BlockHeight(). if ic.BlockHeight() < deposit.Till { return stackitem.NewBool(false) }