-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deploy/update NeoFS contracts automatically within Sidechain deployment procedure #2444
Deploy/update NeoFS contracts automatically within Sidechain deployment procedure #2444
Conversation
3dd9463
to
6365616
Compare
Codecov Report
@@ Coverage Diff @@
## master #2444 +/- ##
==========================================
- Coverage 29.21% 28.53% -0.68%
==========================================
Files 416 421 +5
Lines 31330 32481 +1151
==========================================
+ Hits 9154 9270 +116
- Misses 21377 22402 +1025
- Partials 799 809 +10
... and 1 file with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
6365616
to
e8b560d
Compare
pkg/morph/deploy/contracts.go
Outdated
|
||
var managementContract *management.Contract | ||
if prm.committeeDeployRequired { | ||
deployCommitteeActor, err := newCommitteeNotaryActorWithScope(prm.blockchain, prm.localAcc, prm.committee, transaction.Global) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need Global here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
container contract is deployed with global only during testing. What scope do u expect to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CalledByEntry
, at least if it's not doing some calls in _deploy()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Container contract calls NNS one on _deploy
https://github.com/nspcc-dev/neofs-contract/blob/84ff5d244f6913987c2fb7dc3e2adfb5dfa81381/container/container_contract.go#L170
should CalledByEntry
be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tried, got
failed to send Notary request deploying the contract, will try again later {"contract": "NeoFS Container", "domain": "container.neofs", "error": "script failed (FAULT state) due to an error: at instruction 8 (SYSCALL): failed native call: at instruction 5043 (THROW): unhandled exception: \"not witnessed by committee\""}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be done with CustomContracts
or Rules
, but OK to be a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CustomContracts
works, changed
didn't check Rules
pkg/morph/deploy/contracts.go
Outdated
|
||
l.Error("could not read on-chain state of the contract by NNS domain name, trying by pre-calculated address...") | ||
|
||
preCalculatedAddr := state.CreateContractHash(prm.localAcc.ScriptHash(), prm.localNEF.Checksum, prm.localManifest.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes you deploy from committee, is it correct always using localAcc
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think yes: transaction sender should always be the same acc (to keep hash predictable), so, even if committee multisig is required, we calculate hash from simple acc. What do u think?
btw Actor
provides Sender()
interface, so it may be useful here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw Actor provides Sender() interface, so it may be useful here
Yes, use it here, it'll be helpful if we're to change the signers in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/morph/deploy/deploy.go
Outdated
syncPrm.localManifest = prm.AlphabetContract.Common.Manifest | ||
syncPrm.anyValidExtraDeployArgs = []interface{}{false, util.Uint160{}, util.Uint160{}, "az", 0, 0} | ||
syncPrm.buildVersionedExtraUpdateArgs = func(versionOnChain contractVersion) ([]interface{}, error) { | ||
if versionOnChain.equals(0, 17, 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you care about on-chain version if _deploy
will be called after the update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iiuc ur question: because on-update _deploy
callback is executed with on-chain version which may require some args (like this one). If _deploy
throws exception then contract isn't updated, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on-update _deploy callback is executed with on-chain version
Why? It's called after the update. If you mean arguments, then yes, that's what our update methods do (add on-chain version to control upgrades), but the _deploy
implementation is going to be from the new contract and you need to provide arguments valid for this new version, not the one you have before the upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a suprise to me, will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/morph/deploy/deploy.go
Outdated
} | ||
|
||
// we attempt to deploy contracts other than Alphabet ones by single committee | ||
// member (1st for simplicity) to reduce the likelihood of contract duplication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's a good approach. Old IR nodes can break with some new contracts, so we better have a BFT number of new ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this statement is about "prevent multiple instances of contracts of the same version". Not sure i got ur point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it affect deployments only or upgrades as well? It's OK to have az
leading during deployment, but for upgrades it's a little more dangerous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For upgrades we've ended up on a definite round-robin solution (ref. #2417 (comment)), but I'm not sure that this solution is implemented here. @cthulhu-rider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
affects deployment only, upgrades are parallel
[]byte(netmap.IrCandidateFeeConfig), encodeUintConfig(prm.NetmapContract.Config.IRCandidateFee), | ||
[]byte(netmap.WithdrawFeeConfig), encodeUintConfig(prm.NetmapContract.Config.WithdrawalFee), | ||
[]byte(netmap.HomomorphicHashingDisabledKey), encodeBoolConfig(prm.NetmapContract.Config.HomomorphicHashingDisabled), | ||
[]byte(netmap.MaintenanceModeAllowedConfig), encodeBoolConfig(prm.NetmapContract.Config.MaintenanceModeAllowed), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we get default values from for these? How about preserving them on update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought we'll get exact values (incl. defaults) from neofs-ir app config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it doesn't have them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be implemented with integration in app
// | ||
// Returns address of the on-chain contract synchronized with the record of the | ||
// NNS domain with parameterized name. | ||
func syncNeoFSContract(ctx context.Context, prm syncNeoFSContractPrm) (util.Uint160, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
300+ lines long func, up to 7 level nesting. can it be simplified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont wanna do this right now, otherwise there will be more funcs called once accepting a lot of parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more funcs called once
have nothing against it. there are a lot of linters that ask to keep functions easier does not matter they may be called once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepting a lot of parameters
without a context, i would say smth bad is happening in general then. and functions are not the key reasons for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have nothing agains synthesize funcs that are well documented and their purpose is more than just to reduce the size of other functions. But, at the same time, i prefer to do refactor when behavior is approved and stable, and everything is well-tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and tbh this func isnt too big: there are a lot of logs+whitespace, all actions are done in for
loop and there aint many of them. Ofc linter says function is ugly, but any fella will figure it out i bet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
up to 7 level nesting
lie, up to 4-5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
behavior is approved and stable
it is hard to approve smth that hard to keep in mind
any fella will figure it out i bet
any code (that can be compiled) can be understood. that is the reading/time question
lie, up to 4-5
you pushed code 7+ times since, why the numbers are so necessary? "lie"? 6 tabs, the behavior changing keyword (continue
) is the seventh in the func's context
41384e9
to
039e0f2
Compare
pkg/morph/client/netmap/config.go
Outdated
EpochDurationConfig = "EpochDuration" | ||
ContainerFeeConfig = "ContainerFee" | ||
ContainerAliasFeeConfig = "ContainerAliasFee" | ||
EtIterationsConfig = "EigenTrustIterations" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/EtIterationsConfig
/ETIterationsConfig
pkg/morph/client/netmap/config.go
Outdated
ContainerFeeConfig = "ContainerFee" | ||
ContainerAliasFeeConfig = "ContainerAliasFee" | ||
EtIterationsConfig = "EigenTrustIterations" | ||
EtAlphaConfig = "EigenTrustAlpha" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/EtAlphaConfig
/ETAlphaConfig
pkg/morph/client/netmap/config.go
Outdated
ContainerAliasFeeConfig = "ContainerAliasFee" | ||
EtIterationsConfig = "EigenTrustIterations" | ||
EtAlphaConfig = "EigenTrustAlpha" | ||
IrCandidateFeeConfig = "InnerRingCandidateFee" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/IrCandidateFeeConfig
/IRCandidateFeeConfig
continue | ||
} | ||
|
||
someoneWithoutRole := len(accsWithAlphabetRole) < len(prm.committee) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we check the precise members match? I.e. we can have the exact len(ptm.committee)
number of designated alphabet nodes, but the pubs may differ from the desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we check this in the next condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this one can be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaik designateAsRole
sets member list not adds ones. So, when this condition is true, we don't actually need to know what exact keys are missing and we skip loop above
pkg/morph/deploy/alphabet.go
Outdated
tx, err := roleContract.DesignateAsRoleTransaction(noderoles.NeoFSAlphabet, prm.committee) | ||
if err != nil { | ||
prm.logger.Error("failed to make transaction designating NeoFS Alphabet role to the committee, will try again later", zap.Error(err)) | ||
continue | ||
} | ||
|
||
mainTxID, fallbackTxID, vub, err := committeeActor.Notarize(tx, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use committeeActor.Notarize(roleContract.DesignateAsRoleTransaction(noderoles.NeoFSAlphabet, prm.committee))
, and if error happens, then it will be properly formatted anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/morph/deploy/contracts.go
Outdated
|
||
l.Info("sending new transaction registering domain in the NNS...") | ||
|
||
txID, vub, err := localActor.SendCall(prm.nnsContract, methodNNSRegister, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can at least register and transfer to alphabet address immediately (in the same transaction)?
LGTM, this change is atomic and allows not to worry about the domain owner in future. However, if committee is updated, then we have to transfer the domain from the old committee address to the new one (IINM, currently this code is absent from the node implementation).
pkg/morph/deploy/contracts.go
Outdated
|
||
l.Info("sending new transaction registering domain in the NNS...") | ||
|
||
txID, vub, err := localActor.SendCall(prm.nnsContract, methodNNSRegister, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And BTW, can't we include the domain record addition into the same transaction? Currently it's done in a separate transaction.
if err != nil { | ||
// contract may be deployed but not registered in the NNS yet | ||
if localAccLeads && (errors.Is(err, errMissingDomain) || errors.Is(err, errMissingDomainRecord)) { | ||
return state.CreateContractHash(prm.LocalAccount.ScriptHash(), commonPrm.NEF.Checksum, commonPrm.Manifest.Name), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what if it's not the local account who was deployed the contract? What if it's another committee member was managed to deploy it earlier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can suggest only one alternative: iterate over all contracts, meet first with the same name in manifest and that's it. Btw i've already done this before, but, taking into account that we expect fixed committee during initial deployment and that searching by manifest name isn't protected from the race, I abandoned this approach
i have no ideas about other alternatives
thoughts?
pkg/morph/deploy/deploy.go
Outdated
for ind := 0; ind < len(committee) && ind < glagolitsaSize; ind++ { | ||
syncPrm.tryDeploy = ind == localAccCommitteeIndex // each member deploys its own Alphabet contract | ||
syncPrm.domainName = calculateAlphabetContractAddressDomain(ind) | ||
syncPrm.buildExtraDeployArgs = func() ([]interface{}, error) { | ||
netmapContractAddress, err := resolveContractAddressDynamically(prm.NetmapContract.Common, domainNetmap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why are we trying to deploy Alphabet from every member? Why can't we preserve a single deployment initiator for a cirtain time period like it is done for the rest of the contracts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we have N per-member Alphabet contracts that are completely the same, so afaik to reach N entries we must vary deployer, right?
pkg/morph/deploy/deploy.go
Outdated
} | ||
|
||
// we attempt to deploy contracts other than Alphabet ones by single committee | ||
// member (1st for simplicity) to reduce the likelihood of contract duplication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For upgrades we've ended up on a definite round-robin solution (ref. #2417 (comment)), but I'm not sure that this solution is implemented here. @cthulhu-rider?
95ba5b1
to
6b57741
Compare
13e19c4
to
2316623
Compare
cfba68b
to
ac4bf9d
Compare
updated, still works (checked on 4x node neo-go env) |
It's worth to make these constants reusable. Signed-off-by: Leonard Lyubich <[email protected]>
Previously, `readContractLocalVersion` didn't pass extra arguments to deploy method. This was normal for the NNS contract, but some other system contracts would fail without the args. The function accepts optional data from now. Another drawback: if contract already exists (deployed earlier), `deploy` method throws `contract already exists` exception. To avoid this, dummy (zero) signer is used as a sender which almost definitely doesn't collide with the real one. One more disadvantage: function didn't add committee signers to deploy invocation. For some contracts that require committee witness for deployment (e.g. Container) version reader could not execute. To cover this, the function now accepts committee members and, for simplicity, always attach them as deployent transaction's signers. Signed-off-by: Leonard Lyubich <[email protected]>
Extend Sidechain deployment procedure with Alphabet initialization and deployment/update of all system NeoFS contracts except NNS (implemented earlier). Glagolitic script is moved to shared package in order to reuse it between different applications. Refs nspcc-dev#2195. Signed-off-by: Leonard Lyubich <[email protected]>
Some actions of committee group distribution routine are similar for all domains, so it's redundant to repeat same failed ops for other domains. Signed-off-by: Leonard Lyubich <[email protected]>
Drop additional pre-checks of contract checksums and versions, and perform active `update` method call which can throw `already updated` exception. Signed-off-by: Leonard Lyubich <[email protected]>
Signed-off-by: Leonard Lyubich <[email protected]>
In order to prevent duplication of update transactions, there is a need to sync nonce and ValidUntilBlock values in a distributed manner. Since Sidechain is tied to the particular running NeoFS network, and update procedure is performed during its lifetime, it makes sense to base these variables to the network state. For all update transactions, set: * nonce to the current NeoFS epoch; * ValidUntilBlock to E+100, where E is a height of the Sidechain at which the current epoch began. Signed-off-by: Leonard Lyubich <[email protected]>
Previously, update procedure calculated extra update arguments according to the on-chain version for each contract. This didn't make any sense because update callback (`_deploy` function in contract sources) is known in advance and called around new version of the contract (in this context, on-chain version is previous). Drop `contractVersion` parameter from update data constructors. Signed-off-by: Leonard Lyubich <[email protected]>
Previously, NeoFS contracts deployed in free order. In order to avoid potential dependency or stability issues, it's worth to make deployment in a fixed and clearly defined sequence. Signed-off-by: Leonard Lyubich <[email protected]>
Previously, Sidechain deployment procedure registered contract domain in the NNS and set its record in two separate transactions. According to `register` method that doesn't fail if requested domain already exists, nothing prevents us to stick both actions together into single transaction. Make `syncNeoFSContract` to concatenate scripts of `register` and `addRecord` calls and send single transaction with the result. Signed-off-by: Leonard Lyubich <[email protected]>
Make Sidechain deployment procedure to register 'group.neofs' domain with public key of the committee group. Register functionality is almost the same (different domain name and record format) as for contract addresses, so it's shared in new helper function. Signed-off-by: Leonard Lyubich <[email protected]>
Initially, all Sidechain GAS is owned by the validator multi-sig account. We need to distribute this GAS between the committee (multi-sig account and single ones) for initial deployment. It doesn't make sense to leave GAS on validator account. Add stage following Notary service initialization that: * preserves at least 150GAS (initially 300GAS) on each committee member's account * preserves (almost) all remaining GAS on the committee multi-sig account (keeps 10GAS on validator acc to pay transfer fees) * preserves all available NEO on the committee multi-sig acc Notary requests with transactions to be signed by the validator multi-sig account are also supported in the listening routine. Signed-off-by: Leonard Lyubich <[email protected]>
Proxy contact pays for main transactions sent using Notary service, so it needs a lot of GAS to do so. Transfer 90% of GAS from the committee multi-sig account to the Proxy contract if it has no GAS. Later replenishments are done by NeoFS GAS emission cycles. Signed-off-by: Leonard Lyubich <[email protected]>
Prevent double processing. Support different number and order of signers. Skip transactions that have already been signed by the local account. Log main transaction hash. Signed-off-by: Leonard Lyubich <[email protected]>
All NeoFS Alphabet (i.e. committee) members must be registered as candidates to Sidechain validators. This is vital for NeoFS network processing. Extended Sidechain deployment procedure with registration stage. It sends one transaction witnessed by committee multi-sig (to temporary minimize registration price) and each individual Alphabet member (required by Neo contract). Signed-off-by: Leonard Lyubich <[email protected]>
…acts Make Sidechain deployment procedure to evenly distribute all available NEO tokens between deployed NeoFS Alphabet contracts. Signed-off-by: Leonard Lyubich <[email protected]>
ac4bf9d
to
19fdf98
Compare
Make NeoFS Inner Ring application to perform Sidechain auto-deployment (incl. contracts' update) on each run in local consensus mode. Embedded contracts are built from v0.18.0 revision of NeoFS Contracts with updated NNS behavior (TLDs owned by the committee). Refs nspcc-dev#2195. Signed-off-by: Leonard Lyubich <[email protected]>
Previously, floating point numbers were successfully parsed into integers: mantissa were silently dropped. This behavior could lead to false-positive acceptance of the incorrect configuration. Check floating point separately as a workaround for missing check in `viper` library. Signed-off-by: Leonard Lyubich <[email protected]>
Despite the fact that the committee consists of one member, the transaction must be witnessed by a multi-sig 1:1 account. Signed-off-by: Leonard Lyubich <[email protected]>
Simple local and committee multi-sig notary actors are used in all `syncNeoFSContract` calls, so it's worth to create them once and share. Signed-off-by: Leonard Lyubich <[email protected]>
Signed-off-by: Leonard Lyubich <[email protected]>
Previously, updating transactions of the NeoFS smart contracts (calling `update` method) were sent with ValidUntilBlock set to last epoch block + 100 by the sidechain auto-deployment procedure. This could cause the update idleness when epoch duration was more than 100 Sidechain blocks. To prevent this, the transaction should be valid at least one epoch time. Refs nspcc-dev#2195. Signed-off-by: Leonard Lyubich <[email protected]>
19fdf98
to
3dad944
Compare
rebased and checked again on neo-go and neofs dev env |
thoughts/ideas not implemented in the current PR:
bootstrap
domain exists in the NNS: that is not done in existing networks, so we should adapt to this situationNeoFSAlphabet
role is designated to all committee members, i don't any reason why not (at least at the deployment stage)on all points, I propose to leave the issue and decide separately