innerring: Add GAS pouring mechanism for a configurable list of wallets #128

Merged
fyrchik merged 1 commit from ironbee/frostfs-node:OBJECT-1145_wallet-pouring-for-ir into master 2023-03-20 12:50:07 +00:00
Contributor

Side chain is a powerful and reliable environment for distributed smart-contract code execution. Custom FrostFS deployments may use side chain infrastructure for it's own features outside of object storage scope. However smart-contract execution requires GAS which is controlled by the alphabet contracts and IR itself.

We can organize self-supporting GAS economy by expanding existing mechanism of GAS distribution with the set of predefined wallets. These wallets may be user's wallets, service's wallets or even proxy-like custom contract, so the GAS distribution logic will be delegated to the custom contract.

Side chain is a powerful and reliable environment for distributed smart-contract code execution. Custom FrostFS deployments may use side chain infrastructure for it's own features outside of object storage scope. However smart-contract execution requires GAS which is controlled by the alphabet contracts and IR itself. We can organize self-supporting GAS economy by expanding existing mechanism of GAS distribution with the set of predefined wallets. These wallets may be user's wallets, service's wallets or even proxy-like custom contract, so the GAS distribution logic will be delegated to the custom contract.
ironbee force-pushed OBJECT-1145_wallet-pouring-for-ir from fafca33d9d to d514b87f67 2023-03-09 13:22:10 +00:00 Compare
ironbee changed title from [#XXXX] innerring: Add GAS pouring mechanism for a configurable list of wallets to innerring: Add GAS pouring mechanism for a configurable list of wallets 2023-03-09 13:22:46 +00:00
ironbee requested review from alexvanin 2023-03-09 13:23:16 +00:00
ironbee force-pushed OBJECT-1145_wallet-pouring-for-ir from d514b87f67 to 0e5903594d 2023-03-09 13:25:03 +00:00 Compare
ironbee requested review from storage-core-committers 2023-03-09 13:29:45 +00:00
ironbee requested review from storage-core-developers 2023-03-09 13:29:46 +00:00
alexvanin requested changes 2023-03-09 14:09:44 +00:00
@ -78,6 +78,8 @@ func defaultConfiguration(cfg *viper.Viper) {
cfg.SetDefault("contracts.subnet", "")
cfg.SetDefault("contracts.proxy", "")
cfg.SetDefault("extra_wallets", make([]string, 0))
Owner

I think it would be better to hide things in separate section. I see we have emit section in config which is basically all about GAS emission. Let's put extra_wallets list there.

I think it would be better to hide things in separate section. I see we have `emit` section in config which is basically all about GAS emission. Let's put `extra_wallets` list there.
alexvanin marked this conversation as resolved
ironbee force-pushed OBJECT-1145_wallet-pouring-for-ir from 0e5903594d to 5e24eeefb8 2023-03-09 14:51:45 +00:00 Compare
ironbee requested review from alexvanin 2023-03-09 14:52:33 +00:00
alexvanin approved these changes 2023-03-09 14:59:46 +00:00
@ -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:
Owner

Some comment will be appreciated. Like wallet addresses that are included in gas emission process in equal share with network map nodes.

Some comment will be appreciated. Like `wallet addresses that are included in gas emission process in equal share with network map nodes`.
carpawell requested changes 2023-03-09 17:23:40 +00:00
carpawell left a comment
Contributor

While a can imagine that it can be useful, i see no PR description/comment/issue/etc that explains me (and any other reviewer) why do we need that and what descussiong led us to that PR.

While a can imagine that it can be useful, i see no PR description/comment/issue/etc that explains me (and any other reviewer) why do we need that and what descussiong led us to that PR.
@ -805,6 +809,7 @@ func New(ctx context.Context, log *logger.Logger, cfg *viper.Viper, errChan chan
// create alphabet processor
alphabetProcessor, err := alphabet.New(&alphabet.Params{
ExtraWallets: ExtraWallets{cfg: cfg},
Contributor

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)?

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)?
Owner

I guess it allows simpler SIGHUP reload implementation.

I guess it allows simpler SIGHUP reload implementation.
Owner

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).

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).
@ -11,3 +11,3 @@
const emitMethod = "emit"
func (ap *Processor) processEmit() {
func (ap *Processor) processEmit(extraWallets []string) {
Contributor

why do we pass wallets as a param? extraWallets is available inside that func the same way other fields (irList, storageEmission, morphClient, etc) are

why do we pass wallets as a param? `extraWallets` is available inside that func the same way other fields (`irList`, `storageEmission`, `morphClient`, etc) are
@ -81,1 +81,4 @@
}
for i := range extraWallets {
key, err := address.StringToUint160(extraWallets[i])
Contributor

the same operation every processEmit call?

the same operation every `processEmit` call?
Owner

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.
Owner

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.
Author
Contributor

@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.

@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.
@ -82,0 +89,4 @@
continue
}
err = ap.morphClient.TransferGas(key, gasPerNode)
Contributor

code duplication?

code duplication?
Author
Contributor

Nodes and wallets get processed separately. TransferGas is called in both batches.

Nodes and wallets get processed separately. TransferGas is called in both batches.
Contributor

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)

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)
fyrchik marked this conversation as resolved
@ -31,8 +31,16 @@ type (
GetByIndex(int) (util.Uint160, bool)
}
// ExtraWallets is in interface of configured
Contributor

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)?

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)?
fyrchik reviewed 2023-03-09 20:34:19 +00:00
@ -80,2 +80,4 @@
}
}
for i := range extraWallets {
Owner

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?
Author
Contributor

I am not sure what you mean. Could you elaborate, please?

I am not sure what you mean. Could you elaborate, please?
Owner

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 8dc5b38568/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.

`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.
fyrchik marked this conversation as resolved
Owner

While a can imagine that it can be useful, i see no PR description/comment/issue/etc that describes me (and any other reviewer) why do we need that and what discussion led us to that PR.

Agree with you. Let me clarify a bit of how we got here.

It is a part of the epic for on-premises feature, to establish self-supporting GAS economy in side chain for custom FrostFS installation. Such installations may use variety of custom smart-contracts in the sidechain besides FrostFS contracts.

So the issues should have been created after feature kick-off when we discussed details, including this one. And it did, but there was almost no syncrhonization between on-premises tasks and open-source tasks. I think today is a first day when sync robot is finally up and all issues and PRs are moved from previous organization and previous code source.

So I hope from now we can return to the process when:

  • issues are created during decomposition step with syncrhonization of public and private repos and contain rich description inside,
  • they are discussed synchronously on kick-offs and other meetings and asynchronously in issue comments.
> While a can imagine that it can be useful, i see no PR description/comment/issue/etc that describes me (and any other reviewer) why do we need that and what discussion led us to that PR. Agree with you. Let me clarify a bit of how we got here. It is a part of the epic for on-premises feature, to establish self-supporting GAS economy in side chain for custom FrostFS installation. Such installations may use variety of custom smart-contracts in the sidechain besides FrostFS contracts. So the issues should have been created after feature kick-off when we discussed details, including this one. And it did, but there was almost no syncrhonization between on-premises tasks and open-source tasks. I think today is a first day when sync robot is finally up and all issues and PRs are moved from previous organization and previous code source. So I hope from now we can return to the process when: - issues are created during decomposition step with syncrhonization of public and private repos and contain rich description inside, - they are discussed synchronously on kick-offs and other meetings and asynchronously in issue comments.
Contributor

@alexvanin, ok, i see.

Has anyone considered the idea that such a list can be a network setting? @TrueCloudLab/storage-core-committers
To me such a list is the way to say "the network not gonna work without GAS on that wallets". I could get the idea wrong.

@alexvanin, ok, i see. Has anyone considered the idea that such a list can be a network setting? @TrueCloudLab/storage-core-committers To me such a list is the way to say "the network not gonna work without GAS on _that_ wallets". I could get the idea wrong.
Owner

Has anyone considered the idea that such a list can be a network setting?

Yes. This may be the next step if this change shows its feasibility.

To me such a list is the way to say "the network not gonna work without GAS on that wallets". I could get the idea wrong.

Yes, you are right. Let's try the simple solution first and if it works well enough, move to the network settings option.

> Has anyone considered the idea that such a list can be a network setting? Yes. This may be the next step if this change shows its feasibility. > To me such a list is the way to say "the network not gonna work without GAS on _that_ wallets". I could get the idea wrong. Yes, you are right. Let's try the simple solution first and if it works well enough, move to the network settings option.
Contributor

@realloc, i do not mind. Just also do not understand why is it "the simple" solution. Someone has to know that wallets and add them to all the nodes' configs. At the same time network config is just a KV pair without any restrictions in our API: we can add it, we can test it, we can remove it; while config changes are not that simple IMO.

@realloc, i do not mind. Just also do not understand why is it "the simple" solution. Someone has to know that wallets and add them to _all_ the nodes' configs. At the same time network config is just a KV pair without any restrictions in our API: we can add it, we can test it, we can remove it; while config changes are not that simple IMO.
Owner

Someone has to know that wallets and add them to all the nodes' configs.

Yes, but operationally that's the simplest way to do it.

> Someone has to know that wallets and add them to _all_ the nodes' configs. Yes, but _operationally_ that's the simplest way to do it.
ironbee force-pushed OBJECT-1145_wallet-pouring-for-ir from 5e24eeefb8 to 91ad37affe 2023-03-14 12:55:46 +00:00 Compare
ironbee force-pushed OBJECT-1145_wallet-pouring-for-ir from 91ad37affe to c5319b19de 2023-03-14 12:58:42 +00:00 Compare
ironbee requested review from carpawell 2023-03-14 16:22:21 +00:00
fyrchik approved these changes 2023-03-15 05:20:08 +00:00
@ -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", make([]string, 0))
Owner

Isn't nil shorter and better?

Isn't `nil` shorter and better?
fyrchik marked this conversation as resolved
@ -1037,0 +1045,4 @@
// parseWalletAddressesFromStrings returns a slice of util.Uint160 from a slice
// of strings.
func parseWalletAddressesFromStrings(wallets []string) ([]util.Uint160, error) {
if len(wallets) > 0 {
Owner

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.

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.
fyrchik marked this conversation as resolved
fyrchik reviewed 2023-03-15 07:58:39 +00:00
@ -82,0 +83,4 @@
if err != nil {
receiversLog := make([]string, len(ap.parsedWallets))
for i, addr := range ap.parsedWallets {
receiversLog[i] = addr.StringLE()
Owner

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?
Author
Contributor

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.
Owner

I mean using address.Uint160ToString from neo-go instead of hex-string.

I mean using `address.Uint160ToString` from neo-go instead of hex-string.
ironbee force-pushed OBJECT-1145_wallet-pouring-for-ir from c5319b19de to ae45f31712 2023-03-15 09:36:42 +00:00 Compare
ironbee force-pushed OBJECT-1145_wallet-pouring-for-ir from ae45f31712 to f0b82feaba 2023-03-15 09:38:00 +00:00 Compare
ironbee force-pushed OBJECT-1145_wallet-pouring-for-ir from f0b82feaba to 69c496e65c 2023-03-15 15:05:02 +00:00 Compare
ironbee requested review from fyrchik 2023-03-18 08:15:43 +00:00
ironbee requested review from alexvanin 2023-03-18 08:15:44 +00:00
alexvanin approved these changes 2023-03-20 07:23:32 +00:00
fyrchik approved these changes 2023-03-20 12:35:11 +00:00
fyrchik reviewed 2023-03-20 12:46:07 +00:00
@ -2,7 +2,6 @@ package alphabet
import (
"crypto/elliptic"
Owner

What is this?

What is this?
ironbee force-pushed OBJECT-1145_wallet-pouring-for-ir from 69c496e65c to 5b00a79ff4 2023-03-20 12:48:43 +00:00 Compare
fyrchik merged commit db3ccd2762 into master 2023-03-20 12:50:07 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#128
No description provided.