innerring: Add GAS pouring mechanism for a configurable list of wallets #128
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#128
Loading…
Reference in a new issue
No description provided.
Delete branch "ironbee/frostfs-node:OBJECT-1145_wallet-pouring-for-ir"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
fafca33d9d
tod514b87f67
[#XXXX] innerring: Add GAS pouring mechanism for a configurable list of walletsto innerring: Add GAS pouring mechanism for a configurable list of walletsd514b87f67
to0e5903594d
@ -78,6 +78,8 @@ func defaultConfiguration(cfg *viper.Viper) {
cfg.SetDefault("contracts.subnet", "")
cfg.SetDefault("contracts.proxy", "")
cfg.SetDefault("extra_wallets", make([]string, 0))
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 putextra_wallets
list there.0e5903594d
to5e24eeefb8
@ -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:
Some comment will be appreciated. Like
wallet addresses that are included in gas emission process in equal share with network map nodes
.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},
why do we have
viper
lib that deep? all the other usages are here in theNew
func (read values, store them in some var/field)?I guess it allows simpler SIGHUP reload implementation.
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) {
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])
the same operation every
processEmit
call?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.
We can parse it once on start and on every SIGHUP later, I think.
@carpawell @fyrchik if I add some
client.BatchTransferGas
method which uses MultiTransfer, it still needs a []string of addresses for logging purposes.#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)
code duplication?
Nodes and wallets get processed separately. TransferGas is called in both batches.
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)@ -31,8 +31,16 @@ type (
GetByIndex(int) (util.Uint160, bool)
}
// ExtraWallets is in interface of configured
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)?@ -80,2 +80,4 @@
}
}
for i := range extraWallets {
It would be nice to transfer everything in one transaction. Is it possible with the current API?
I am not sure what you mean. Could you elaborate, please?
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 this8dc5b38568/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.
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:
@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.
Yes. This may be the next step if this change shows its feasibility.
Yes, you are right. Let's try the simple solution first and if it works well enough, move to the network settings option.
@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.
Yes, but operationally that's the simplest way to do it.
5e24eeefb8
to91ad37affe
91ad37affe
toc5319b19de
@ -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))
Isn't
nil
shorter and better?@ -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 {
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.@ -82,0 +83,4 @@
if err != nil {
receiversLog := make([]string, len(ap.parsedWallets))
for i, addr := range ap.parsedWallets {
receiversLog[i] = addr.StringLE()
Is there a reason why we use addresses in config and hex format here?
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.
I mean using
address.Uint160ToString
from neo-go instead of hex-string.c5319b19de
toae45f31712
ae45f31712
tof0b82feaba
f0b82feaba
to69c496e65c
@ -2,7 +2,6 @@ package alphabet
import (
"crypto/elliptic"
What is this?
69c496e65c
to5b00a79ff4