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.
61a066583e/src/Neo/SmartContract/ApplicationEngine.cs (L634).

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

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.<br>`amount` (int) - amount of GAS to deposit.<br>`data` represents array of two parameters: <br>1. `to` (uint160) - account of the deposit owner.<br>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.<br>`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.<br>`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.<br>`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). |

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

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)

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")

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))

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())

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 {

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, &notary.OnNEP17PaymentData{Account: &multisigHash, Till: e.Chain.BlockHeight() + 1})
+		gasCommitteeInvoker.Invoke(t, true, "transfer", multisigHash, notaryHash, depositAmount, &notary.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)

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)
 	}