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 {
client *client.Client
notaryEnabled bool
notaryDisabled bool
// TTL of Sidechain cached values. Non-positive value disables caching.
cacheTTL time.Duration

View file

@ -48,6 +48,7 @@ func initMorphComponents(ctx context.Context, c *cfg) {
}),
client.WithSwitchInterval(morphconfig.SwitchInterval(c.appCfg)),
client.WithMorphCacheMetrics(c.metricsCollector.MorphCacheMetrics()),
client.WithNotaryOptions(client.WithProxyContract(c.cfgMorph.proxyScriptHash)),
)
if err != nil {
c.log.Info(logs.FrostFSNodeFailedToCreateNeoRPCClient,
@ -68,21 +69,17 @@ func initMorphComponents(ctx context.Context, c *cfg) {
}
c.cfgMorph.client = cli
c.cfgMorph.notaryEnabled = cli.ProbeNotary()
c.cfgMorph.notaryDisabled = !cli.ProbeNotary()
lookupScriptHashesInNNS(c) // smart contract auto negotiation
if c.cfgMorph.notaryEnabled {
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.
err = c.cfgMorph.client.EnableNotarySupport(
client.WithProxyContract(
c.cfgMorph.proxyScriptHash,
),
)
if c.cfgMorph.notaryDisabled {
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.DisableNotarySupport()
fatalOnErr(err)
}
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())
@ -112,7 +109,7 @@ func initMorphComponents(ctx context.Context, c *cfg) {
func makeAndWaitNotaryDeposit(ctx context.Context, c *cfg) {
// skip notary deposit in non-notary environments
if !c.cfgMorph.notaryEnabled {
if c.cfgMorph.notaryDisabled {
return
}
@ -295,7 +292,7 @@ func lookupScriptHashesInNNS(c *cfg) {
)
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
}

View file

@ -202,16 +202,18 @@ func addNewEpochNotificationHandlers(c *cfg) {
c.handleLocalNodeInfo(ni)
})
if c.cfgMorph.notaryEnabled {
addNewEpochAsyncNotificationHandler(c, func(ev event.Event) {
_, err := makeNotaryDeposit(c)
if err != nil {
c.log.Error(logs.FrostFSNodeCouldNotMakeNotaryDeposit,
zap.String("error", err.Error()),
)
}
})
if c.cfgMorph.notaryDisabled {
return
}
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.

View file

@ -16,7 +16,6 @@ import (
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"
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"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/morph/client/container"
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
// 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 {
return err
}
@ -118,25 +117,10 @@ func (s *Server) initMainnet(ctx context.Context, cfg *viper.Viper, morphChain *
}
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)
if !s.mainNotaryConfig.disabled {
// enable notary support in the main client
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)
}
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.
}
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
@ -470,7 +454,7 @@ func (s *Server) initMorph(ctx context.Context, cfg *viper.Viper, errChan chan<-
}
// create morph client
s.morphClient, err = createClient(ctx, morphChain, errChan)
s.morphClient, err = createClient(ctx, morphChain, errChan, s.contracts.proxy, nil)
if err != nil {
return nil, err
}

View file

@ -444,7 +444,7 @@ func createListener(ctx context.Context, cli *client.Client, p *chainParams) (ev
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"
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)
}
notaryOpts := client.WithNotaryOptions(client.WithProxyContract(proxyHash))
if src != nil {
notaryOpts = client.WithNotaryOptions(
client.WithProxyContract(proxyHash),
client.WithAlphabetSource(src),
)
}
return client.New(
ctx,
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.WithMorphCacheMetrics(p.morphCacheMetric),
notaryOpts,
)
}

View file

@ -48,6 +48,8 @@ type cfg struct {
switchInterval time.Duration
morphCacheMetrics metrics.MorphCacheMetrics
notaryOpts []NotaryOption
}
const (
@ -74,8 +76,6 @@ func defaultConfig() *cfg {
// Notary support should be enabled with EnableNotarySupport client
// method separately.
//
// If private key is nil, it panics.
//
// Other values are set according to provided options, or by default:
// - client context: Background;
// - dial timeout: 5s;
@ -92,7 +92,7 @@ func defaultConfig() *cfg {
// If there are no healthy endpoint - returns ErrNoHealthyEndpoint.
func New(ctx context.Context, key *keys.PrivateKey, opts ...Option) (*Client, error) {
if key == nil {
panic("empty private key")
return nil, errors.New("empty private key")
}
acc := wallet.NewAccountFromPrivateKey(key)
@ -157,6 +157,10 @@ func New(ctx context.Context, key *keys.PrivateKey, opts ...Option) (*Client, er
}
cli.setActor(act)
if err = cli.enableNotarySupport(cfg.notaryOpts...); err != nil {
return nil, err
}
go cli.closeWaiter(ctx)
return cli, nil
@ -311,3 +315,9 @@ func WithMorphCacheMetrics(morphCacheMetrics metrics.MorphCacheMetrics) Option {
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"
setDesignateMethod = "designateAsRole"
notaryBalanceErrMsg = "can't fetch notary balance"
notaryNotEnabledPanicMsg = "notary support was not enabled on this client"
notaryBalanceErrMsg = "can't fetch notary balance"
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 {
return &notaryCfg{
@ -78,7 +81,7 @@ func defaultNotaryConfig(c *Client) *notaryCfg {
// EnableNotarySupport creates notary structure in client that provides
// ability for client to get alphabet keys from committee or provided source
// 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()
defer c.switchLock.RUnlock()
@ -114,6 +117,25 @@ func (c *Client) EnableNotarySupport(opts ...NotaryOption) error {
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
// called before.
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
// 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) {
c.switchLock.RLock()
defer c.switchLock.RUnlock()
@ -149,7 +171,7 @@ func (c *Client) DepositNotary(amount fixedn.Fixed8, delta uint32) (res util.Uin
}
if c.notary == nil {
panic(notaryNotEnabledPanicMsg)
return util.Uint256{}, fmt.Errorf(notaryDisabledErrMsg)
}
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 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) {
c.switchLock.RLock()
defer c.switchLock.RUnlock()
@ -184,7 +206,7 @@ func (c *Client) DepositEndlessNotary(amount fixedn.Fixed8) (res util.Uint256, e
}
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
@ -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.
// 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) {
c.switchLock.RLock()
defer c.switchLock.RUnlock()
@ -234,7 +256,7 @@ func (c *Client) GetNotaryDeposit() (res int64, err error) {
}
if c.notary == nil {
panic(notaryNotEnabledPanicMsg)
return 0, ErrNotaryDisabled
}
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
// 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 {
c.switchLock.RLock()
defer c.switchLock.RUnlock()
@ -286,7 +308,7 @@ func (c *Client) UpdateNotaryList(prm UpdateNotaryListPrm) error {
}
if c.notary == nil {
panic(notaryNotEnabledPanicMsg)
return ErrNotaryDisabled
}
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.
// 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 {
c.switchLock.RLock()
defer c.switchLock.RUnlock()
@ -334,7 +356,7 @@ func (c *Client) UpdateNeoFSAlphabetList(prm UpdateAlphabetListPrm) error {
}
if c.notary == nil {
panic(notaryNotEnabledPanicMsg)
return ErrNotaryDisabled
}
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.
func (c *Client) ReceiveNotaryRequests(txSigner util.Uint160, ch chan<- *result.NotaryRequestEvent) (string, error) {
if c.notary == nil {
panic(notaryNotEnabledPanicMsg)
return "", ErrNotaryDisabled
}
c.switchLock.Lock()