node: Break notary deposit wait after VUB #1467

Merged
fyrchik merged 1 commit from dstepanov-yadro/frostfs-node:fix/deposit_vub into master 2024-11-05 13:05:55 +00:00
6 changed files with 77 additions and 41 deletions

View file

@ -17,15 +17,16 @@ import (
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/util/rand"
"github.com/nspcc-dev/neo-go/pkg/core/block"
"github.com/nspcc-dev/neo-go/pkg/core/state"
"github.com/nspcc-dev/neo-go/pkg/neorpc/result"
"github.com/nspcc-dev/neo-go/pkg/rpcclient/waiter"
"github.com/nspcc-dev/neo-go/pkg/smartcontract/trigger"
"github.com/nspcc-dev/neo-go/pkg/util"
"github.com/nspcc-dev/neo-go/pkg/vm/vmstate"
"go.uber.org/zap"
)
const (
newEpochNotification = "NewEpoch"
// amount of tries(blocks) before notary deposit timeout.
notaryDepositRetriesAmount = 300
)
func (c *cfg) initMorphComponents(ctx context.Context) {
@ -128,7 +129,7 @@ func makeAndWaitNotaryDeposit(ctx context.Context, c *cfg) {
return
}
tx, err := makeNotaryDeposit(c)
tx, vub, err := makeNotaryDeposit(c)
fatalOnErr(err)
if tx.Equals(util.Uint256{}) {
@ -139,11 +140,11 @@ func makeAndWaitNotaryDeposit(ctx context.Context, c *cfg) {
return
}
err = waitNotaryDeposit(ctx, c, tx)
err = waitNotaryDeposit(ctx, c, tx, vub)
fatalOnErr(err)
}
func makeNotaryDeposit(c *cfg) (util.Uint256, error) {
func makeNotaryDeposit(c *cfg) (util.Uint256, uint32, error) {
const (
// gasMultiplier defines how many times more the notary
// balance must be compared to the GAS balance of the node:
@ -157,7 +158,7 @@ func makeNotaryDeposit(c *cfg) (util.Uint256, error) {
depositAmount, err := client.CalculateNotaryDepositAmount(c.cfgMorph.client, gasMultiplier, gasDivisor)
if err != nil {
return util.Uint256{}, fmt.Errorf("could not calculate notary deposit: %w", err)
return util.Uint256{}, 0, fmt.Errorf("could not calculate notary deposit: %w", err)
}
return c.cfgMorph.client.DepositEndlessNotary(depositAmount)
@ -168,32 +169,43 @@ var (
errNotaryDepositTimeout = errors.New("notary deposit tx has not appeared in the network")
)
func waitNotaryDeposit(ctx context.Context, c *cfg, tx util.Uint256) error {
for range notaryDepositRetriesAmount {
c.log.Debug(logs.ClientAttemptToWaitForNotaryDepositTransactionToGetPersisted)
select {
case <-ctx.Done():
return ctx.Err()
default:
}
type waiterClient struct {
c *client.Client
}
ok, err := c.cfgMorph.client.TxHalt(tx)
if err == nil {
if ok {
func (w *waiterClient) Context() context.Context {
return context.Background()
}
func (w *waiterClient) GetApplicationLog(hash util.Uint256, trig *trigger.Type) (*result.ApplicationLog, error) {
return w.c.GetApplicationLog(hash, trig)
}
func (w *waiterClient) GetBlockCount() (uint32, error) {
return w.c.BlockCount()
}
func (w *waiterClient) GetVersion() (*result.Version, error) {
return w.c.GetVersion()
}
func waitNotaryDeposit(ctx context.Context, c *cfg, tx util.Uint256, vub uint32) error {
w, err := waiter.NewPollingBased(&waiterClient{c: c.cfgMorph.client})
if err != nil {
return fmt.Errorf("could not create notary deposit waiter: %w", err)

This is tricky, because there is some time between TxHalt() and BlockCount()
Consider this scenario:

  1. Check for TxHalt() at Height=VUB-1
  2. New block is accepted, tx is in it.
  3. Wait for one more block
  4. Height = VUB + 1, so we exit.

I think we must make yet another TxHalt request in case currHeight > vub.

Still we may switch endpoint, but this is a more rare event.
Another option would be to make WaitTxPersist(ctx, tx) and do everything under switch mutex.

This is tricky, because there is some time between `TxHalt()` and `BlockCount()` Consider this scenario: 1. Check for `TxHalt()` at Height=VUB-1 2. New block is accepted, tx is in it. 3. Wait for _one more_ block 4. Height = VUB + 1, so we exit. I think we must make yet another `TxHalt` request in case `currHeight > vub`. Still we may switch endpoint, but this is a more rare event. Another option would be to make `WaitTxPersist(ctx, tx)` and do everything under switch mutex.
Also, can we reuse something from https://github.com/nspcc-dev/neo-go/blob/master/pkg/rpcclient/waiter/waiter.go ?

Done: make yet another TxHalt request in case currHeight > vub

Done: make yet another `TxHalt` request in case `currHeight > vub`

Also, can we reuse something from https://github.com/nspcc-dev/neo-go/blob/master/pkg/rpcclient/waiter/waiter.go ?

Don't think we need one more dependency from neo-go.

> Also, can we reuse something from https://github.com/nspcc-dev/neo-go/blob/master/pkg/rpcclient/waiter/waiter.go ? Don't think we need one more dependency from neo-go.

We already depend on the module (and it is external).
And that package canonically solves exactly our problem (I have given 1 error, who knows what else can go wrong).

We already depend on the module (and it is external). And that package canonically solves exactly our problem (I have given 1 error, who knows what else can go wrong).

Then why do you ask, if in fact you insist on using it?

Then why do you ask, if in fact you insist on using it?

fixed

fixed

That seemed like an obvious choice, and I wanted to know if there was any reason not to make it.
I find yet-another-dependency (which in fact is already is in go.mod) argument weak.

That seemed like an obvious choice, and I wanted to know if there was any reason not to make it. I find yet-another-dependency (which in fact is already is in `go.mod`) argument weak.

obvious choice

I don't see an obvious choice: to use this interface, which I didn't even know about (and not only me), I had to add a structure and two methods, and also add a dependency on a package that can be changed at any time (do you know how neo-go supports backward compatibility?)
The implementation itself also raises questions:

  • the context should not be stored in the structure:
RPCPollingBased interface {
		Context() context.Context
  • there is no check for the length of the array Execution: res.Executions[0]
  • the number of retries const PollingBasedRetryCount = 3 is not configured
  • why are retries used only to request the number of blocks, but not for application log
  • ...
> obvious choice I don't see an obvious choice: to use this interface, which I didn't even know about (and not only me), I had to add a structure and two methods, and also add a dependency on a package that can be changed at any time (do you know how neo-go supports backward compatibility?) The implementation itself also raises questions: - the context should not be stored in the structure: ``` RPCPollingBased interface { Context() context.Context ``` - there is no check for the length of the array `Execution: res.Executions[0]` - the number of retries `const PollingBasedRetryCount = 3` is not configured - why are retries used only to request the number of blocks, but not for application log - ...
}
res, err := w.WaitAny(ctx, vub, tx)
if err != nil {
if errors.Is(err, waiter.ErrTxNotAccepted) {
return errNotaryDepositTimeout
}
return fmt.Errorf("could not wait for notary deposit persists in chain: %w", err)
}
if res.Execution.VMState.HasFlag(vmstate.Halt) {

if res.Execution.VMState.HasFlag(vmstate.Halt) {?

`if res.Execution.VMState.HasFlag(vmstate.Halt) {`?

ok, fixed

ok, fixed
c.log.Info(logs.ClientNotaryDepositTransactionWasSuccessfullyPersisted)
return nil
}
return errNotaryDepositFail
}
err = c.cfgMorph.client.Wait(ctx, 1)
if err != nil {
return fmt.Errorf("could not wait for one block in chain: %w", err)
}
}
return errNotaryDepositTimeout
}
func listenMorphNotifications(ctx context.Context, c *cfg) {

View file

@ -192,7 +192,7 @@ func addNewEpochNotificationHandlers(c *cfg) {
if c.cfgMorph.notaryEnabled {
addNewEpochAsyncNotificationHandler(c, func(_ event.Event) {
_, err := makeNotaryDeposit(c)
_, _, err := makeNotaryDeposit(c)
if err != nil {
c.log.Error(logs.FrostFSNodeCouldNotMakeNotaryDeposit,
zap.String("error", err.Error()),

View file

@ -142,7 +142,6 @@ const (
ClientNotaryRequestWithPreparedMainTXInvoked = "notary request with prepared main TX invoked"
ClientNotaryRequestInvoked = "notary request invoked"
ClientNotaryDepositTransactionWasSuccessfullyPersisted = "notary deposit transaction was successfully persisted"
ClientAttemptToWaitForNotaryDepositTransactionToGetPersisted = "attempt to wait for notary deposit transaction to get persisted"
ClientNeoClientInvoke = "neo client invoke"
ClientNativeGasTransferInvoke = "native gas transfer invoke"
ClientBatchGasTransferInvoke = "batch gas transfer invoke"

View file

@ -40,13 +40,14 @@ func (s *Server) depositMainNotary() (tx util.Uint256, err error) {
)
}
func (s *Server) depositSideNotary() (tx util.Uint256, err error) {
func (s *Server) depositSideNotary() (util.Uint256, error) {
depositAmount, err := client.CalculateNotaryDepositAmount(s.morphClient, gasMultiplier, gasDivisor)
if err != nil {
return util.Uint256{}, fmt.Errorf("could not calculate side notary deposit amount: %w", err)
}
return s.morphClient.DepositEndlessNotary(depositAmount)
tx, _, err := s.morphClient.DepositEndlessNotary(depositAmount)
return tx, err
}
func (s *Server) notaryHandler(_ event.Event) {

View file

@ -19,6 +19,7 @@ import (
"github.com/nspcc-dev/neo-go/pkg/core/transaction"
"github.com/nspcc-dev/neo-go/pkg/crypto/keys"
"github.com/nspcc-dev/neo-go/pkg/encoding/fixedn"
"github.com/nspcc-dev/neo-go/pkg/neorpc/result"
"github.com/nspcc-dev/neo-go/pkg/rpcclient"
"github.com/nspcc-dev/neo-go/pkg/rpcclient/actor"
"github.com/nspcc-dev/neo-go/pkg/rpcclient/gas"
@ -461,6 +462,28 @@ func (c *Client) TxHalt(h util.Uint256) (res bool, err error) {
return len(aer.Executions) > 0 && aer.Executions[0].VMState.HasFlag(vmstate.Halt), nil
}
func (c *Client) GetApplicationLog(hash util.Uint256, trig *trigger.Type) (*result.ApplicationLog, error) {
c.switchLock.RLock()
defer c.switchLock.RUnlock()
if c.inactive {
return nil, ErrConnectionLost
}
return c.client.GetApplicationLog(hash, trig)
}
func (c *Client) GetVersion() (*result.Version, error) {
c.switchLock.RLock()
defer c.switchLock.RUnlock()
if c.inactive {
return nil, ErrConnectionLost
}
return c.client.GetVersion()
}
// TxHeight returns true if transaction has been successfully executed and persisted.
func (c *Client) TxHeight(h util.Uint256) (res uint32, err error) {
c.switchLock.RLock()

View file

@ -140,7 +140,7 @@ func (c *Client) ProbeNotary() (res bool) {
// use this function.
//
// This function must be invoked with notary enabled otherwise it throws panic.
func (c *Client) DepositNotary(amount fixedn.Fixed8, delta uint32) (res util.Uint256, err error) {
func (c *Client) DepositNotary(amount fixedn.Fixed8, delta uint32) (util.Uint256, error) {
c.switchLock.RLock()
defer c.switchLock.RUnlock()
@ -163,7 +163,8 @@ func (c *Client) DepositNotary(amount fixedn.Fixed8, delta uint32) (res util.Uin
}
till := max(int64(bc+delta), currentTill)
return c.depositNotary(amount, till)
res, _, err := c.depositNotary(amount, till)
return res, err
}
// DepositEndlessNotary calls notary deposit method. Unlike `DepositNotary`,
@ -171,12 +172,12 @@ func (c *Client) DepositNotary(amount fixedn.Fixed8, delta uint32) (res util.Uin
// This allows to avoid ValidAfterDeposit failures.
//
// This function must be invoked with notary enabled otherwise it throws panic.
func (c *Client) DepositEndlessNotary(amount fixedn.Fixed8) (res util.Uint256, err error) {
func (c *Client) DepositEndlessNotary(amount fixedn.Fixed8) (util.Uint256, uint32, error) {
c.switchLock.RLock()
defer c.switchLock.RUnlock()
if c.inactive {
return util.Uint256{}, ErrConnectionLost
return util.Uint256{}, 0, ErrConnectionLost
}
if c.notary == nil {
@ -187,7 +188,7 @@ func (c *Client) DepositEndlessNotary(amount fixedn.Fixed8) (res util.Uint256, e
return c.depositNotary(amount, math.MaxUint32)
}
func (c *Client) depositNotary(amount fixedn.Fixed8, till int64) (res util.Uint256, err error) {
func (c *Client) depositNotary(amount fixedn.Fixed8, till int64) (util.Uint256, uint32, error) {
txHash, vub, err := c.gasToken.Transfer(
c.accAddr,
c.notary.notary,
@ -195,7 +196,7 @@ func (c *Client) depositNotary(amount fixedn.Fixed8, till int64) (res util.Uint2
[]any{c.acc.PrivateKey().GetScriptHash(), till})
if err != nil {
if !errors.Is(err, neorpc.ErrAlreadyExists) {
return util.Uint256{}, fmt.Errorf("can't make notary deposit: %w", err)
return util.Uint256{}, 0, fmt.Errorf("can't make notary deposit: %w", err)
}
// Transaction is already in mempool waiting to be processed.
@ -205,7 +206,7 @@ func (c *Client) depositNotary(amount fixedn.Fixed8, till int64) (res util.Uint2
zap.Int64("expire_at", till),
zap.Uint32("vub", vub),
zap.Error(err))
return util.Uint256{}, nil
return util.Uint256{}, 0, nil
}
c.logger.Info(logs.ClientNotaryDepositInvoke,
@ -214,7 +215,7 @@ func (c *Client) depositNotary(amount fixedn.Fixed8, till int64) (res util.Uint2
zap.Uint32("vub", vub),
zap.Stringer("tx_hash", txHash.Reverse()))
return txHash, nil
return txHash, vub, nil
}
// GetNotaryDeposit returns deposit of client's account in notary contract.