Merge pull request #3211 from nspcc-dev/fix-curr-height

core: review usages of (*intero.Context).BlockHeight method

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

@@ -79,13 +79,13 @@ func TestNotary_Pipeline(t *testing.T) {
 	neoCommitteeInvoker.InvokeFail(t, "only GAS can be accepted for deposit", "transfer", multisigHash, notaryHash, int64(1), &notary.OnNEP17PaymentData{Till: uint32(depositLock)})
 
 	// `onPayment`: insufficient first deposit
-	gasCommitteeInvoker.InvokeFail(t, "first deposit can not be less then", "transfer", multisigHash, notaryHash, int64(2*feePerKey-1), &notary.OnNEP17PaymentData{Till: uint32(depositLock)})
+	gasCommitteeInvoker.InvokeFail(t, "first deposit can not be less than", "transfer", multisigHash, notaryHash, int64(2*feePerKey-1), &notary.OnNEP17PaymentData{Till: uint32(depositLock)})
 
 	// `onPayment`: invalid `data` (missing `till` parameter)
 	gasCommitteeInvoker.InvokeFail(t, "`data` parameter should be an array of 2 elements", "transfer", multisigHash, notaryHash, 2*feePerKey, []any{nil})
 
 	// `onPayment`: invalid `data` (outdated `till` parameter)
-	gasCommitteeInvoker.InvokeFail(t, "`till` shouldn't be less then the chain's height", "transfer", multisigHash, notaryHash, 2*feePerKey, &notary.OnNEP17PaymentData{})
+	gasCommitteeInvoker.InvokeFail(t, "`till` shouldn't be less than the chain's height", "transfer", multisigHash, notaryHash, 2*feePerKey, &notary.OnNEP17PaymentData{})
 
 	// `onPayment`: good
 	gasCommitteeInvoker.Invoke(t, true, "transfer", multisigHash, notaryHash, 2*feePerKey, &notary.OnNEP17PaymentData{Till: uint32(depositLock)})
@@ -107,13 +107,13 @@ func TestNotary_Pipeline(t *testing.T) {
 	// `expirationOf`: check `till` is updated.
 	notaryCommitteeInvoker.Invoke(t, depositLock+1, "expirationOf", multisigHash)
 
-	// `onPayment`: empty payment, should fail because `till` less then the previous one
+	// `onPayment`: empty payment, should fail because `till` less than the previous one
-	gasCommitteeInvoker.InvokeFail(t, "`till` shouldn't be less then the previous value", "transfer", multisigHash, notaryHash, int64(0), &notary.OnNEP17PaymentData{Account: &multisigHash, Till: uint32(depositLock)})
+	gasCommitteeInvoker.InvokeFail(t, "`till` shouldn't be less than the previous value", "transfer", multisigHash, notaryHash, int64(0), &notary.OnNEP17PaymentData{Account: &multisigHash, Till: uint32(depositLock)})
 	checkBalanceOf(t, notaryHash, 3*feePerKey)
 	notaryCommitteeInvoker.Invoke(t, depositLock+1, "expirationOf", multisigHash)
 
-	// `onPayment`: empty payment, should fail because `till` less then the chain height
+	// `onPayment`: empty payment, should fail because `till` less than the chain height
-	gasCommitteeInvoker.InvokeFail(t, "`till` shouldn't be less then the chain's height", "transfer", multisigHash, notaryHash, int64(0), &notary.OnNEP17PaymentData{Account: &multisigHash, Till: uint32(1)})
+	gasCommitteeInvoker.InvokeFail(t, "`till` shouldn't be less than the chain's height", "transfer", multisigHash, notaryHash, int64(0), &notary.OnNEP17PaymentData{Account: &multisigHash, Till: uint32(1)})
 	checkBalanceOf(t, notaryHash, 3*feePerKey)
 	notaryCommitteeInvoker.Invoke(t, depositLock+1, "expirationOf", multisigHash)
 
@@ -126,11 +126,11 @@ func TestNotary_Pipeline(t *testing.T) {
 	notaryCommitteeInvoker.Invoke(t, false, "lockDepositUntil", util.Uint160{1, 2, 3}, int64(depositLock+3))
 	notaryCommitteeInvoker.Invoke(t, depositLock+2, "expirationOf", multisigHash)
 
-	// `lockDepositUntil`: bad `till` (less then the previous one)
+	// `lockDepositUntil`: bad `till` (less than the previous one)
 	notaryCommitteeInvoker.Invoke(t, false, "lockDepositUntil", multisigHash, int64(depositLock+1))
 	notaryCommitteeInvoker.Invoke(t, depositLock+2, "expirationOf", multisigHash)
 
-	// `lockDepositUntil`: bad `till` (less then the chain's height)
+	// `lockDepositUntil`: bad `till` (less than the chain's height)
 	notaryCommitteeInvoker.Invoke(t, false, "lockDepositUntil", multisigHash, int64(1))
 	notaryCommitteeInvoker.Invoke(t, depositLock+2, "expirationOf", multisigHash)
 
@@ -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,16 +232,16 @@ 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 {
+	if till < currentHeight+2 {
-		panic(fmt.Errorf("`till` shouldn't be less then the chain's height %d", currentHeight))
+		panic(fmt.Errorf("`till` shouldn't be less than 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))
+		panic(fmt.Errorf("`till` shouldn't be less than the previous value %d", deposit.Till))
 	}
 	feePerKey := n.GetNotaryServiceFeePerKey(ic.DAO)
 	if deposit == nil {
 		if amount.Cmp(big.NewInt(2*feePerKey)) < 0 {
-			panic(fmt.Errorf("first deposit can not be less then %d, got %d", 2*feePerKey, amount.Int64()))
+			panic(fmt.Errorf("first deposit can not be less than %d, got %d", 2*feePerKey, amount.Int64()))
 		}
 		deposit = &state.Deposit{
 			Amount: new(big.Int),
@@ -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)
 	}

pkg/services/rpcsrv/server.go

@@ -2998,7 +2998,7 @@ func (s *Server) blockHeightFromParam(param *params.Param) (uint32, *neorpc.Erro
 	}
 
 	if num < 0 || int64(num) > int64(s.chain.BlockHeight()) {
-		return 0, neorpc.WrapErrorWithData(neorpc.ErrUnknownHeight, fmt.Sprintf("param at index %d should be greater than or equal to 0 and less then or equal to current block height, got: %d", 0, num))
+		return 0, neorpc.WrapErrorWithData(neorpc.ErrUnknownHeight, fmt.Sprintf("param at index %d should be greater than or equal to 0 and less than or equal to current block height, got: %d", 0, num))
 	}
 	return uint32(num), nil
 }