WIP: Make notary support enabled by default in morph client #961

Closed
elebedeva wants to merge 2 commits from elebedeva/frostfs-node:feat/morph-default-notary-support into master
8 changed files with 83 additions and 58 deletions

View file

@ -552,7 +552,7 @@ func (c *cfgGRPC) dropConnection(endpoint string) {
type cfgMorph struct { type cfgMorph struct {
client *client.Client client *client.Client
notaryEnabled bool notaryDisabled bool
// TTL of Sidechain cached values. Non-positive value disables caching. // TTL of Sidechain cached values. Non-positive value disables caching.
cacheTTL time.Duration cacheTTL time.Duration

View file

@ -48,6 +48,7 @@ func initMorphComponents(ctx context.Context, c *cfg) {
}), }),
client.WithSwitchInterval(morphconfig.SwitchInterval(c.appCfg)), client.WithSwitchInterval(morphconfig.SwitchInterval(c.appCfg)),
client.WithMorphCacheMetrics(c.metricsCollector.MorphCacheMetrics()), client.WithMorphCacheMetrics(c.metricsCollector.MorphCacheMetrics()),
client.WithNotaryOptions(client.WithProxyContract(c.cfgMorph.proxyScriptHash)),
) )
if err != nil { if err != nil {
c.log.Info(logs.FrostFSNodeFailedToCreateNeoRPCClient, c.log.Info(logs.FrostFSNodeFailedToCreateNeoRPCClient,
@ -68,21 +69,17 @@ func initMorphComponents(ctx context.Context, c *cfg) {
} }
c.cfgMorph.client = cli c.cfgMorph.client = cli
c.cfgMorph.notaryEnabled = cli.ProbeNotary() c.cfgMorph.notaryDisabled = !cli.ProbeNotary()
lookupScriptHashesInNNS(c) // smart contract auto negotiation lookupScriptHashesInNNS(c) // smart contract auto negotiation
if c.cfgMorph.notaryEnabled { if c.cfgMorph.notaryDisabled {
dstepanov-yadro marked this conversation as resolved Outdated

c.cfgMorph.notaryEnabled is used after this? Can be deleted?

`c.cfgMorph.notaryEnabled` is used after this? Can be deleted?

replaced c.cfgMorph.notaryEnabled with c.cfgMorph.notaryDisabled, it is used to skip notary-related steps in non-notary environments.

replaced `c.cfgMorph.notaryEnabled` with `c.cfgMorph.notaryDisabled`, it is used to skip notary-related steps in non-notary environments.
acid-ant marked this conversation as resolved Outdated

Why not to rename sidechain_enabled too?

Why not to rename `sidechain_enabled` too?

fixed.

fixed.
err = c.cfgMorph.client.EnableNotarySupport( err := c.cfgMorph.client.DisableNotarySupport()
client.WithProxyContract(
c.cfgMorph.proxyScriptHash,
),
)
fatalOnErr(err) fatalOnErr(err)
} }
c.log.Info(logs.FrostFSNodeNotarySupport, c.log.Info(logs.FrostFSNodeNotarySupport,
zap.Bool("sidechain_enabled", c.cfgMorph.notaryEnabled), zap.Bool("sidechain_disabled", c.cfgMorph.notaryDisabled),
) )
wrap, err := nmClient.NewFromMorph(c.cfgMorph.client, c.cfgNetmap.scriptHash, 0, nmClient.TryNotary()) wrap, err := nmClient.NewFromMorph(c.cfgMorph.client, c.cfgNetmap.scriptHash, 0, nmClient.TryNotary())
@ -112,7 +109,7 @@ func initMorphComponents(ctx context.Context, c *cfg) {
func makeAndWaitNotaryDeposit(ctx context.Context, c *cfg) { func makeAndWaitNotaryDeposit(ctx context.Context, c *cfg) {
// skip notary deposit in non-notary environments // skip notary deposit in non-notary environments
if !c.cfgMorph.notaryEnabled { if c.cfgMorph.notaryDisabled {
return return
} }
@ -295,7 +292,7 @@ func lookupScriptHashesInNNS(c *cfg) {
) )
for _, t := range targets { for _, t := range targets {
if t.nnsName == client.NNSProxyContractName && !c.cfgMorph.notaryEnabled { if t.nnsName == client.NNSProxyContractName && c.cfgMorph.notaryDisabled {
continue // ignore proxy contract if notary disabled continue // ignore proxy contract if notary disabled
} }

View file

@ -202,16 +202,18 @@ func addNewEpochNotificationHandlers(c *cfg) {
c.handleLocalNodeInfo(ni) c.handleLocalNodeInfo(ni)
}) })
if c.cfgMorph.notaryEnabled { if c.cfgMorph.notaryDisabled {
addNewEpochAsyncNotificationHandler(c, func(ev event.Event) { return
_, err := makeNotaryDeposit(c)
if err != nil {
c.log.Error(logs.FrostFSNodeCouldNotMakeNotaryDeposit,
zap.String("error", err.Error()),
)
}
})
} }
addNewEpochAsyncNotificationHandler(c, func(ev event.Event) {
_, err := makeNotaryDeposit(c)
if err != nil {
c.log.Error(logs.FrostFSNodeCouldNotMakeNotaryDeposit,
zap.String("error", err.Error()),
)
}
})
} }
// bootstrapNode adds current node to the Network map. // bootstrapNode adds current node to the Network map.

View file

@ -16,7 +16,6 @@ import (
nodevalidator "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/innerring/processors/netmap/nodevalidation" nodevalidator "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/innerring/processors/netmap/nodevalidation"
addrvalidator "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/innerring/processors/netmap/nodevalidation/maddress" addrvalidator "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/innerring/processors/netmap/nodevalidation/maddress"
statevalidation "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/innerring/processors/netmap/nodevalidation/state" statevalidation "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/innerring/processors/netmap/nodevalidation/state"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/morph/client"
balanceClient "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/morph/client/balance" balanceClient "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/morph/client/balance"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/morph/client/container" "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/morph/client/container"
frostfsClient "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/morph/client/frostfs" frostfsClient "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/morph/client/frostfs"
@ -107,7 +106,7 @@ func (s *Server) initMainnet(ctx context.Context, cfg *viper.Viper, morphChain *
mainnetChain.from = fromMainChainBlock mainnetChain.from = fromMainChainBlock
// create mainnet client // create mainnet client
s.mainnetClient, err = createClient(ctx, mainnetChain, errChan) s.mainnetClient, err = createClient(ctx, mainnetChain, errChan, s.contracts.processing, s.morphClient.Committee)
if err != nil { if err != nil {
return err return err
} }
@ -118,25 +117,10 @@ func (s *Server) initMainnet(ctx context.Context, cfg *viper.Viper, morphChain *
} }
func (s *Server) enableNotarySupport() error { func (s *Server) enableNotarySupport() error {
// enable notary support in the side client
err := s.morphClient.EnableNotarySupport(
client.WithProxyContract(s.contracts.proxy),
)
if err != nil {
return fmt.Errorf("could not enable side chain notary support: %w", err)
}
s.morphListener.EnableNotarySupport(s.contracts.proxy, s.morphClient.Committee, s.morphClient) s.morphListener.EnableNotarySupport(s.contracts.proxy, s.morphClient.Committee, s.morphClient)
if !s.mainNotaryConfig.disabled { if !s.mainNotaryConfig.disabled {
// enable notary support in the main client return fmt.Errorf("main chain notary support is disabled")
Review

Either the condition or the message is wrong, in this branch notary is enabled, but the message says it is disabled, why so?

Also, we could use errors.New here.

Either the condition or the message is wrong, in this branch notary is enabled, but the message says it is disabled, why so? Also, we could use `errors.New` here.
err := s.mainnetClient.EnableNotarySupport(
client.WithProxyContract(s.contracts.processing),
client.WithAlphabetSource(s.morphClient.Committee),
)
if err != nil {
return fmt.Errorf("could not enable main chain notary support: %w", err)
}
} }
fyrchik marked this conversation as resolved Outdated

What is the problem here?

What is the problem here?

I thought it was an appropriate case of using EnableNotarySupport() even if it's deprecated. Now it seems illogical (having in mind that if notary support is disabled - it must be on purpose) so i decided to return an error here instead.

I thought it was an appropriate case of using `EnableNotarySupport()` even if it's deprecated. Now it seems illogical (having in mind that if notary support is disabled - it must be on purpose) so i decided to return an `error` here instead.
return nil return nil
@ -470,7 +454,7 @@ func (s *Server) initMorph(ctx context.Context, cfg *viper.Viper, errChan chan<-
} }
// create morph client // create morph client
s.morphClient, err = createClient(ctx, morphChain, errChan) s.morphClient, err = createClient(ctx, morphChain, errChan, s.contracts.proxy, nil)
if err != nil { if err != nil {
return nil, err return nil, err
} }

View file

@ -444,7 +444,7 @@ func createListener(ctx context.Context, cli *client.Client, p *chainParams) (ev
return listener, err return listener, err
} }
func createClient(ctx context.Context, p *chainParams, errChan chan<- error) (*client.Client, error) { func createClient(ctx context.Context, p *chainParams, errChan chan<- error, proxyHash util.Uint160, src client.AlphabetKeys) (*client.Client, error) {
// config name left unchanged for compatibility, may be its better to rename it to "endpoints" or "clients" // config name left unchanged for compatibility, may be its better to rename it to "endpoints" or "clients"
var endpoints []client.Endpoint var endpoints []client.Endpoint
@ -473,6 +473,15 @@ func createClient(ctx context.Context, p *chainParams, errChan chan<- error) (*c
return nil, fmt.Errorf("%s chain client endpoints not provided", p.name) return nil, fmt.Errorf("%s chain client endpoints not provided", p.name)
} }
notaryOpts := client.WithNotaryOptions(client.WithProxyContract(proxyHash))
if src != nil {
notaryOpts = client.WithNotaryOptions(
client.WithProxyContract(proxyHash),
client.WithAlphabetSource(src),
)
}
return client.New( return client.New(
ctx, ctx,
p.key, p.key,
@ -485,6 +494,7 @@ func createClient(ctx context.Context, p *chainParams, errChan chan<- error) (*c
}), }),
client.WithSwitchInterval(p.cfg.GetDuration(p.name+".switch_interval")), client.WithSwitchInterval(p.cfg.GetDuration(p.name+".switch_interval")),
client.WithMorphCacheMetrics(p.morphCacheMetric), client.WithMorphCacheMetrics(p.morphCacheMetric),
notaryOpts,
) )
} }

View file

@ -48,6 +48,8 @@ type cfg struct {
switchInterval time.Duration switchInterval time.Duration
morphCacheMetrics metrics.MorphCacheMetrics morphCacheMetrics metrics.MorphCacheMetrics
notaryOpts []NotaryOption
} }
const ( const (
@ -74,8 +76,6 @@ func defaultConfig() *cfg {
// Notary support should be enabled with EnableNotarySupport client // Notary support should be enabled with EnableNotarySupport client
// method separately. // method separately.
// //
// If private key is nil, it panics.
//
// Other values are set according to provided options, or by default: // Other values are set according to provided options, or by default:
// - client context: Background; // - client context: Background;
// - dial timeout: 5s; // - dial timeout: 5s;
@ -92,7 +92,7 @@ func defaultConfig() *cfg {
// If there are no healthy endpoint - returns ErrNoHealthyEndpoint. // If there are no healthy endpoint - returns ErrNoHealthyEndpoint.
func New(ctx context.Context, key *keys.PrivateKey, opts ...Option) (*Client, error) { func New(ctx context.Context, key *keys.PrivateKey, opts ...Option) (*Client, error) {
if key == nil { if key == nil {
panic("empty private key") return nil, errors.New("empty private key")
} }
acc := wallet.NewAccountFromPrivateKey(key) acc := wallet.NewAccountFromPrivateKey(key)
@ -157,6 +157,10 @@ func New(ctx context.Context, key *keys.PrivateKey, opts ...Option) (*Client, er
} }
cli.setActor(act) cli.setActor(act)
if err = cli.enableNotarySupport(cfg.notaryOpts...); err != nil {
return nil, err
}
go cli.closeWaiter(ctx) go cli.closeWaiter(ctx)
return cli, nil return cli, nil
@ -311,3 +315,9 @@ func WithMorphCacheMetrics(morphCacheMetrics metrics.MorphCacheMetrics) Option {
c.morphCacheMetrics = morphCacheMetrics c.morphCacheMetrics = morphCacheMetrics
} }
} }
func WithNotaryOptions(opts ...NotaryOption) Option {
return func(c *cfg) {
c.notaryOpts = append(c.notaryOpts, opts...)
}
}

View file

@ -61,11 +61,14 @@ const (
notaryExpirationOfMethod = "expirationOf" notaryExpirationOfMethod = "expirationOf"
setDesignateMethod = "designateAsRole" setDesignateMethod = "designateAsRole"
notaryBalanceErrMsg = "can't fetch notary balance" notaryBalanceErrMsg = "can't fetch notary balance"
notaryNotEnabledPanicMsg = "notary support was not enabled on this client" notaryDisabledErrMsg = "notary support was not enabled on this client"
) )
var errUnexpectedItems = errors.New("invalid number of NEO VM arguments on stack") var (
errUnexpectedItems = errors.New("invalid number of NEO VM arguments on stack")
ErrNotaryDisabled = errors.New(notaryDisabledErrMsg)
)
func defaultNotaryConfig(c *Client) *notaryCfg { func defaultNotaryConfig(c *Client) *notaryCfg {
return &notaryCfg{ return &notaryCfg{
@ -78,7 +81,7 @@ func defaultNotaryConfig(c *Client) *notaryCfg {
// EnableNotarySupport creates notary structure in client that provides // EnableNotarySupport creates notary structure in client that provides
// ability for client to get alphabet keys from committee or provided source // ability for client to get alphabet keys from committee or provided source
// and use proxy contract script hash to create tx for notary contract. // and use proxy contract script hash to create tx for notary contract.
func (c *Client) EnableNotarySupport(opts ...NotaryOption) error { func (c *Client) enableNotarySupport(opts ...NotaryOption) error {
acid-ant marked this conversation as resolved Outdated

What the difference with EnableNotarySupport?

What the difference with `EnableNotarySupport`?

It's the same EnableNotarySupport() but now it's called from Client constructor therefore there is no need to make it public.

It's the same `EnableNotarySupport()` but now it's called from `Client constructor` therefore there is no need to make it public.

Why not to remove it or call private from public? Why we need to duplicate code?

Why not to remove it or call private from public? Why we need to duplicate code?

@aarifullin mentions it here:

To [enable notary support by default in morph client constructor], we need to invoke EnableNotarySupport in constructor, but at the same it would be better to make this method deprecated to notify client to stop using this invocation explictly.

I suggest to create enableNotarySupport, copy the definition to there

@aarifullin mentions it [here](https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/905): > To [enable notary support by default in morph client constructor], we need to invoke `EnableNotarySupport` in constructor, but at the same it would be better to make this method `deprecated` to notify client to stop using this invocation explictly. > I suggest to create `enableNotarySupport`, copy the definition to there

Let's do like @aarifullin suggested:

func (c *Client) EnableNotarySupport(opts ...NotaryOption) error {
  return c.enableNotarySupport(opts...) // also use in the constructor
}
Let's do like @aarifullin suggested: ``` func (c *Client) EnableNotarySupport(opts ...NotaryOption) error { return c.enableNotarySupport(opts...) // also use in the constructor } ```

fixed.

fixed.
c.switchLock.RLock() c.switchLock.RLock()
defer c.switchLock.RUnlock() defer c.switchLock.RUnlock()
@ -114,6 +117,25 @@ func (c *Client) EnableNotarySupport(opts ...NotaryOption) error {
return nil return nil
} }
// Deprecated: Notary support is enabled by default during client construction.
func (c *Client) EnableNotarySupport(opts ...NotaryOption) error {
return c.enableNotarySupport(opts...)
}
// DisableNotarySupport removes notary structure in client.
func (c *Client) DisableNotarySupport() error {
c.switchLock.Lock()
defer c.switchLock.Unlock()
if c.inactive {
return ErrConnectionLost
}
c.notary = nil
return nil
}
// IsNotaryEnabled returns true if EnableNotarySupport has been successfully // IsNotaryEnabled returns true if EnableNotarySupport has been successfully
// called before. // called before.
func (c *Client) IsNotaryEnabled() bool { func (c *Client) IsNotaryEnabled() bool {
@ -139,7 +161,7 @@ func (c *Client) ProbeNotary() (res bool) {
// be called periodically. Notary support should be enabled in client to // be called periodically. Notary support should be enabled in client to
// use this function. // use this function.
// //
// This function must be invoked with notary enabled otherwise it throws panic. // This function must be invoked with notary enabled.
func (c *Client) DepositNotary(amount fixedn.Fixed8, delta uint32) (res util.Uint256, err error) { func (c *Client) DepositNotary(amount fixedn.Fixed8, delta uint32) (res util.Uint256, err error) {
c.switchLock.RLock() c.switchLock.RLock()
defer c.switchLock.RUnlock() defer c.switchLock.RUnlock()
@ -149,7 +171,7 @@ func (c *Client) DepositNotary(amount fixedn.Fixed8, delta uint32) (res util.Uin
} }
if c.notary == nil { if c.notary == nil {
panic(notaryNotEnabledPanicMsg) return util.Uint256{}, fmt.Errorf(notaryDisabledErrMsg)
} }
bc, err := c.rpcActor.GetBlockCount() bc, err := c.rpcActor.GetBlockCount()
@ -174,7 +196,7 @@ func (c *Client) DepositNotary(amount fixedn.Fixed8, delta uint32) (res util.Uin
// this method sets notary deposit till parameter to a maximum possible value. // this method sets notary deposit till parameter to a maximum possible value.
// This allows to avoid ValidAfterDeposit failures. // This allows to avoid ValidAfterDeposit failures.
// //
// This function must be invoked with notary enabled otherwise it throws panic. // This function must be invoked with notary enabled.
func (c *Client) DepositEndlessNotary(amount fixedn.Fixed8) (res util.Uint256, err error) { func (c *Client) DepositEndlessNotary(amount fixedn.Fixed8) (res util.Uint256, err error) {
c.switchLock.RLock() c.switchLock.RLock()
defer c.switchLock.RUnlock() defer c.switchLock.RUnlock()
@ -184,7 +206,7 @@ func (c *Client) DepositEndlessNotary(amount fixedn.Fixed8) (res util.Uint256, e
} }
if c.notary == nil { if c.notary == nil {
panic(notaryNotEnabledPanicMsg) return util.Uint256{}, ErrNotaryDisabled
} }
// till value refers to a block height and it is uint32 value in neo-go // till value refers to a block height and it is uint32 value in neo-go
@ -224,7 +246,7 @@ func (c *Client) depositNotary(amount fixedn.Fixed8, till int64) (res util.Uint2
// GetNotaryDeposit returns deposit of client's account in notary contract. // GetNotaryDeposit returns deposit of client's account in notary contract.
// Notary support should be enabled in client to use this function. // Notary support should be enabled in client to use this function.
// //
// This function must be invoked with notary enabled otherwise it throws panic. // This function must be invoked with notary enabled.
func (c *Client) GetNotaryDeposit() (res int64, err error) { func (c *Client) GetNotaryDeposit() (res int64, err error) {
c.switchLock.RLock() c.switchLock.RLock()
defer c.switchLock.RUnlock() defer c.switchLock.RUnlock()
@ -234,7 +256,7 @@ func (c *Client) GetNotaryDeposit() (res int64, err error) {
} }
if c.notary == nil { if c.notary == nil {
panic(notaryNotEnabledPanicMsg) return 0, ErrNotaryDisabled
} }
sh := c.acc.PrivateKey().PublicKey().GetScriptHash() sh := c.acc.PrivateKey().PublicKey().GetScriptHash()
@ -276,7 +298,7 @@ func (u *UpdateNotaryListPrm) SetHash(hash util.Uint256) {
// UpdateNotaryList updates list of notary nodes in designate contract. Requires // UpdateNotaryList updates list of notary nodes in designate contract. Requires
// committee multi signature. // committee multi signature.
// //
// This function must be invoked with notary enabled otherwise it throws panic. // This function must be invoked with notary enabled.
func (c *Client) UpdateNotaryList(prm UpdateNotaryListPrm) error { func (c *Client) UpdateNotaryList(prm UpdateNotaryListPrm) error {
c.switchLock.RLock() c.switchLock.RLock()
defer c.switchLock.RUnlock() defer c.switchLock.RUnlock()
@ -286,7 +308,7 @@ func (c *Client) UpdateNotaryList(prm UpdateNotaryListPrm) error {
} }
if c.notary == nil { if c.notary == nil {
panic(notaryNotEnabledPanicMsg) return ErrNotaryDisabled
} }
nonce, vub, err := c.CalculateNonceAndVUB(&prm.hash) nonce, vub, err := c.CalculateNonceAndVUB(&prm.hash)
@ -324,7 +346,7 @@ func (u *UpdateAlphabetListPrm) SetHash(hash util.Uint256) {
// As for sidechain list should contain all inner ring nodes. // As for sidechain list should contain all inner ring nodes.
// Requires committee multi signature. // Requires committee multi signature.
// //
// This function must be invoked with notary enabled otherwise it throws panic. // This function must be invoked with notary enabled.
func (c *Client) UpdateNeoFSAlphabetList(prm UpdateAlphabetListPrm) error { func (c *Client) UpdateNeoFSAlphabetList(prm UpdateAlphabetListPrm) error {
c.switchLock.RLock() c.switchLock.RLock()
defer c.switchLock.RUnlock() defer c.switchLock.RUnlock()
@ -334,7 +356,7 @@ func (c *Client) UpdateNeoFSAlphabetList(prm UpdateAlphabetListPrm) error {
} }
if c.notary == nil { if c.notary == nil {
panic(notaryNotEnabledPanicMsg) return ErrNotaryDisabled
} }
nonce, vub, err := c.CalculateNonceAndVUB(&prm.hash) nonce, vub, err := c.CalculateNonceAndVUB(&prm.hash)

View file

@ -63,7 +63,7 @@ func (c *Client) ReceiveBlocks(ch chan<- *block.Block) (string, error) {
// connection to any of passed RPC endpoints. // connection to any of passed RPC endpoints.
func (c *Client) ReceiveNotaryRequests(txSigner util.Uint160, ch chan<- *result.NotaryRequestEvent) (string, error) { func (c *Client) ReceiveNotaryRequests(txSigner util.Uint160, ch chan<- *result.NotaryRequestEvent) (string, error) {
if c.notary == nil { if c.notary == nil {
panic(notaryNotEnabledPanicMsg) return "", ErrNotaryDisabled
} }
c.switchLock.Lock() c.switchLock.Lock()