innerring: Add GAS pouring mechanism for a configurable list of wallets #128
|
@ -4,6 +4,7 @@ Changelog for FrostFS Node
|
|||
## [Unreleased]
|
||||
|
||||
### Added
|
||||
- Add GAS pouring mechanism for a configurable list of wallets (#128)
|
||||
- Separate batching for replicated operations over the same container in pilorama (#1621)
|
||||
- Doc for extended headers (#2128)
|
||||
- New `frostfs_node_object_container_size` metric for tracking size of reqular objects in a container (#2116)
|
||||
|
|
|
@ -102,6 +102,7 @@ func defaultConfiguration(cfg *viper.Viper) {
|
|||
cfg.SetDefault("emit.mint.threshold", 1)
|
||||
cfg.SetDefault("emit.mint.value", 20000000) // 0.2 Fixed8
|
||||
cfg.SetDefault("emit.gas.balance_threshold", 0)
|
||||
cfg.SetDefault("emit.extra_wallets", nil)
|
||||
fyrchik marked this conversation as resolved
Outdated
|
||||
|
||||
cfg.SetDefault("audit.task.exec_pool_size", 10)
|
||||
cfg.SetDefault("audit.task.queue_capacity", 100)
|
||||
|
|
|
@ -72,6 +72,10 @@ emit:
|
|||
threshold: 1 # Lifetime of records in LRU cache of all deposit receivers in FrostFS epochs
|
||||
gas:
|
||||
balance_threshold: 100000000000 # Fixed8 value of inner ring wallet balance threshold when GAS emission for deposit receivers is disabled; disabled by default
|
||||
extra_wallets: # wallet addresses that are included in gas emission process in equal share with network map nodes
|
||||
alexvanin
commented
Some comment will be appreciated. Like Some comment will be appreciated. Like `wallet addresses that are included in gas emission process in equal share with network map nodes`.
|
||||
- "NQcfMqU6pfXFwaaBN6KHcTpT63eMtzk6eH"
|
||||
- "NaSVC4xKySQBpKr1XRVYFCHjLhuYXnMBrP"
|
||||
- "NT9jL5XcxcDt2iTj67o2d5xNfDxquN3pPk"
|
||||
|
||||
workers:
|
||||
alphabet: 10 # Number of workers to process events from alphabet contract in parallel
|
||||
|
|
|
@ -49,7 +49,9 @@ import (
|
|||
"github.com/nspcc-dev/neo-go/pkg/core/block"
|
||||
"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/address"
|
||||
"github.com/nspcc-dev/neo-go/pkg/encoding/fixedn"
|
||||
"github.com/nspcc-dev/neo-go/pkg/util"
|
||||
"github.com/panjf2000/ants/v2"
|
||||
"github.com/spf13/viper"
|
||||
"go.uber.org/atomic"
|
||||
|
@ -803,8 +805,14 @@ func New(ctx context.Context, log *logger.Logger, cfg *viper.Viper, errChan chan
|
|||
}
|
||||
}
|
||||
|
||||
parsedWallets, err := parseWalletAddressesFromStrings(cfg.GetStringSlice("emit.extra_wallets"))
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
carpawell
commented
why do we have why do we have `viper` lib that deep? all the other usages are here in the `New` func (read values, store them in some var/field)?
alexvanin
commented
I guess it allows simpler SIGHUP reload implementation. I guess it allows simpler SIGHUP reload implementation.
fyrchik
commented
Let's consider SIGHUP in a separate task. Let's consider SIGHUP in a separate task. `Params` structure should not accept viper -- otherwise we cannot handle any logical errors during config reread (we will just stop working until the config is fixed).
|
||||
// create alphabet processor
|
||||
alphabetProcessor, err := alphabet.New(&alphabet.Params{
|
||||
ParsedWallets: parsedWallets,
|
||||
Log: log,
|
||||
PoolSize: cfg.GetInt("workers.alphabet"),
|
||||
AlphabetContracts: server.contracts.alphabet,
|
||||
|
@ -1034,6 +1042,24 @@ func ParsePublicKeysFromStrings(pubKeys []string) (keys.PublicKeys, error) {
|
|||
return publicKeys, nil
|
||||
}
|
||||
|
||||
// parseWalletAddressesFromStrings returns a slice of util.Uint160 from a slice
|
||||
// of strings.
|
||||
func parseWalletAddressesFromStrings(wallets []string) ([]util.Uint160, error) {
|
||||
if len(wallets) == 0 {
|
||||
fyrchik marked this conversation as resolved
Outdated
fyrchik
commented
Purely aesthetic: wouldn't it be better to have Purely aesthetic: wouldn't it be better to have `if len(wallets) == 0` first, to have less indendation in the biggest part of a function.
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
var err error
|
||||
extraWallets := make([]util.Uint160, len(wallets))
|
||||
for i := range wallets {
|
||||
extraWallets[i], err = address.StringToUint160(wallets[i])
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
}
|
||||
return extraWallets, nil
|
||||
}
|
||||
|
||||
func (s *Server) initConfigFromBlockchain() error {
|
||||
// get current epoch
|
||||
epoch, err := s.netmapClient.Epoch()
|
||||
|
|
|
@ -57,7 +57,7 @@ func (ap *Processor) processEmit() {
|
|||
return
|
||||
}
|
||||
|
||||
gasPerNode := fixedn.Fixed8(ap.storageEmission / uint64(ln))
|
||||
gasPerNode := fixedn.Fixed8(ap.storageEmission / uint64(ln+len(ap.parsedWallets)))
|
||||
|
||||
for i := range nmNodes {
|
||||
keyBytes := nmNodes[i].PublicKey()
|
||||
|
@ -79,4 +79,17 @@ func (ap *Processor) processEmit() {
|
|||
)
|
||||
}
|
||||
}
|
||||
|
||||
err = ap.morphClient.BatchTransferGas(ap.parsedWallets, gasPerNode)
|
||||
fyrchik marked this conversation as resolved
Outdated
fyrchik
commented
It would be nice to transfer everything in one transaction. Is it possible with the current API? It would be nice to transfer everything in one transaction. Is it possible with the current API?
ironbee
commented
I am not sure what you mean. Could you elaborate, please? I am not sure what you mean. Could you elaborate, please?
fyrchik
commented
The question was whether we could easily do the same here or it should be postponed because it requires some refactoring. `TransferGas` performs a contract call of the GAS contract and sends a transaction. However, there could be multiple contract calls in a single transaction -- it's just a byte-code after all. neo-go has a convenient API for this https://github.com/nspcc-dev/neo-go/blob/8dc5b38568fe6de0346f1a2deb1f3235ad7ff524/pkg/rpcclient/nep17/nep17.go#L117
The question was whether we could easily do the same here or it should be postponed because it requires some refactoring.
|
||||
if err != nil {
|
||||
carpawell
commented
the same operation every the same operation every `processEmit` call?
fyrchik
commented
Agree, we should parse it after we have read the configuration and before providing it to a constructor. This way we also avoid logging the same error again and again. Agree, we should parse it after we have read the configuration and before providing it to a constructor. This way we also avoid logging the same error again and again.
alexvanin
commented
We can parse it once on start and on every SIGHUP later, I think. We can parse it once on start and on every SIGHUP later, I think.
ironbee
commented
@carpawell @fyrchik if I add some
#1 happens every transfer, #2 happens only in case of an error. Both need a conversion code []Uint160 -> []string to log the data with The conversion should happen on both levels (process_emit.go and client.go) or we have to convert the data at process_emit.go and then pass it to new method Is this conversion acceptable? Does not look too well to me. @carpawell @fyrchik if I add some `client.BatchTransferGas` method which uses MultiTransfer, it still needs a []string of addresses for logging purposes.
1) TransferGas-like logging on debug level: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/morph/client/client.go#L239
2) Error logging at process_emit.go: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/innerring/processors/alphabet/process_emit.go#L74
#1 happens every transfer, #2 happens only in case of an error. Both need a conversion code []Uint160 -> []string to log the data with `zap.Strings()`.
The conversion should happen on both levels (process_emit.go and client.go) or we have to convert the data at process_emit.go and then pass it to new method `client.BatchTransferGas` so it could log the array.
Is this conversion acceptable? Does not look too well to me.
|
||||
receiversLog := make([]string, len(ap.parsedWallets))
|
||||
for i, addr := range ap.parsedWallets {
|
||||
fyrchik
commented
Is there a reason why we use addresses in config and hex format here? Is there a reason why we use addresses in config and hex format here?
ironbee
commented
Not sure if I understand the question fully. Previously, my code transformed addresses as strings into Uint160 every emit cycle. Following the comments I changed it so that addresses get transformed into Uint160 once on init phase. However, now the logging requires transforming them into strings. Not sure if I understand the question fully.
Previously, my code transformed addresses as strings into Uint160 every emit cycle. Following the comments I changed it so that addresses get transformed into Uint160 once on init phase. However, now the logging requires transforming them into strings.
fyrchik
commented
I mean using I mean using `address.Uint160ToString` from neo-go instead of hex-string.
|
||||
receiversLog[i] = addr.StringLE()
|
||||
}
|
||||
ap.log.Warn("can't transfer gas to wallet",
|
||||
zap.Strings("receivers", receiversLog),
|
||||
zap.Int64("amount", int64(gasPerNode)),
|
||||
zap.String("error", err.Error()),
|
||||
fyrchik marked this conversation as resolved
Outdated
carpawell
commented
code duplication? code duplication?
ironbee
commented
Nodes and wallets get processed separately. TransferGas is called in both batches. Nodes and wallets get processed separately. TransferGas is called in both batches.
carpawell
commented
i meant that we can handle both netmap nodes and extra wallets (public keys parsing for nodes and parsing/just appending slice for extra wallets) separately and just have one sending i meant that we can handle both netmap nodes and extra wallets (public keys parsing for nodes and parsing/just appending slice for extra wallets) separately and just have one sending `for` loop after that **OR** do that inside one transaction as said @fyrchik (if it is possible with the current API of course)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
|
|
@ -33,6 +33,7 @@ type (
|
|||
|
||||
// Processor of events produced for alphabet contracts in the sidechain.
|
||||
carpawell
commented
such desc says nothing about what that wallets are used for. do we need an interface here? why not just such desc says nothing about what that wallets are used for. do we need an interface here? why not just `[]string` or parsed wallet addresses (uint160)?
|
||||
Processor struct {
|
||||
parsedWallets []util.Uint160
|
||||
log *logger.Logger
|
||||
pool *ants.Pool
|
||||
alphabetContracts Contracts
|
||||
|
@ -44,6 +45,7 @@ type (
|
|||
|
||||
// Params of the processor constructor.
|
||||
Params struct {
|
||||
ParsedWallets []util.Uint160
|
||||
Log *logger.Logger
|
||||
PoolSize int
|
||||
AlphabetContracts Contracts
|
||||
|
@ -73,6 +75,7 @@ func New(p *Params) (*Processor, error) {
|
|||
}
|
||||
|
||||
return &Processor{
|
||||
parsedWallets: p.ParsedWallets,
|
||||
log: p.Log,
|
||||
pool: pool,
|
||||
alphabetContracts: p.AlphabetContracts,
|
||||
|
|
|
@ -244,6 +244,40 @@ func (c *Client) TransferGas(receiver util.Uint160, amount fixedn.Fixed8) error
|
|||
return nil
|
||||
}
|
||||
|
||||
func (c *Client) BatchTransferGas(receivers []util.Uint160, amount fixedn.Fixed8) error {
|
||||
c.switchLock.RLock()
|
||||
defer c.switchLock.RUnlock()
|
||||
|
||||
if c.inactive {
|
||||
return ErrConnectionLost
|
||||
}
|
||||
|
||||
transferParams := make([]nep17.TransferParameters, len(receivers))
|
||||
receiversLog := make([]string, len(receivers))
|
||||
|
||||
for i, receiver := range receivers {
|
||||
transferParams[i] = nep17.TransferParameters{
|
||||
From: c.accAddr,
|
||||
To: receiver,
|
||||
Amount: big.NewInt(int64(amount)),
|
||||
Data: nil,
|
||||
}
|
||||
receiversLog[i] = receiver.StringLE()
|
||||
}
|
||||
|
||||
txHash, vub, err := c.gasToken.MultiTransfer(transferParams)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
c.logger.Debug("batch gas transfer invoke",
|
||||
zap.Strings("to", receiversLog),
|
||||
zap.Stringer("tx_hash", txHash.Reverse()),
|
||||
zap.Uint32("vub", vub))
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// Wait function blocks routing execution until there
|
||||
// are `n` new blocks in the chain.
|
||||
//
|
||||
|
|
Isn't
nil
shorter and better?