Skip to content
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

Introduce procedure for the NeoFS Sidechain deployment #2402

Merged
merged 2 commits into from
Jun 27, 2023

Conversation

cthulhu-rider
Copy link
Contributor

@cthulhu-rider cthulhu-rider commented Jun 22, 2023

i plan to add contracts' deployment in the next PR to reduce the amount of changes at a time (although there are a lot of current ones)

i've tested changes manually with NNS contract built from https://github.com/nspcc-dev/neofs-contract/tree/6e9b183b219353bd07a5d985f2ee38473f4f6a28 on x4 node cluster from neo-go project modified with Notary service

i guess it's possible to write unit tests using neotest framework, but it'll take too much time i bet

i also plan to describe the procedure in detail in docs/ after approval (next PR i think)

@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Merging #2402 (72052c4) into master (1f835b8) will decrease coverage by 0.90%.
The diff coverage is 2.73%.

❗ Current head 72052c4 differs from pull request most recent head 253fe2b. Consider uploading reports for the commit 253fe2b to get more accurate results

@@            Coverage Diff             @@
##           master    nspcc-dev/neofs-node#2402      +/-   ##
==========================================
- Coverage   30.12%   29.23%   -0.90%     
==========================================
  Files         399      406       +7     
  Lines       29850    30863    +1013     
==========================================
+ Hits         8993     9023      +30     
- Misses      20127    21098     +971     
- Partials      730      742      +12     
Impacted Files Coverage Δ
...md/neofs-adm/internal/modules/morph/dump_hashes.go 0.00% <0.00%> (ø)
cmd/neofs-adm/internal/modules/morph/dump_names.go 0.00% <0.00%> (ø)
...d/neofs-adm/internal/modules/morph/renew_domain.go 0.00% <0.00%> (ø)
pkg/morph/deploy/deploy.go 0.00% <0.00%> (ø)
pkg/morph/deploy/nns.go 0.00% <0.00%> (ø)
pkg/morph/deploy/notary.go 0.00% <0.00%> (ø)
pkg/morph/deploy/util.go 0.00% <0.00%> (ø)
pkg/morph/deploy/group.go 11.62% <11.62%> (ø)
cmd/neofs-adm/internal/modules/morph/root.go 50.59% <53.33%> (+0.26%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main logic seems to be correct though.

pkg/morph/deploy/deploy.go Outdated Show resolved Hide resolved
pkg/morph/deploy/deploy.go Outdated Show resolved Hide resolved
pkg/morph/deploy/notary.go Outdated Show resolved Hide resolved
pkg/morph/deploy/notary.go Outdated Show resolved Hide resolved
pkg/morph/deploy/notary.go Outdated Show resolved Hide resolved
pkg/morph/deploy/notary.go Show resolved Hide resolved
pkg/morph/deploy/notary.go Outdated Show resolved Hide resolved
pkg/morph/deploy/notary.go Outdated Show resolved Hide resolved
pkg/morph/deploy/notary.go Outdated Show resolved Hide resolved
pkg/morph/deploy/group.go Show resolved Hide resolved
@cthulhu-rider
Copy link
Contributor Author

tested after bunch of changes, everything still works ok

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only looked at the commented changes and GH can't easily compare old/new PR state, but it should be OK otherwise.

contracts/00-nns.manifest.json Outdated Show resolved Hide resolved
Copy link
Contributor

@notimetoname notimetoname left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, sometimes it is too info.

pkg/morph/deploy/nns.go Outdated Show resolved Hide resolved
pkg/morph/deploy/nns.go Outdated Show resolved Hide resolved
pkg/morph/deploy/nns.go Outdated Show resolved Hide resolved
pkg/morph/deploy/notary.go Show resolved Hide resolved
pkg/morph/deploy/notary.go Show resolved Hide resolved
pkg/morph/deploy/notary.go Show resolved Hide resolved
}

// KeyStorage represents storage of the private keys.
type KeyStorage interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it an interface? you have literally described its (the only) implementation in a few lines

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cuz package doesn't care how exactly key is stored (in file, in db, in memory, remotely)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have to store it? i understand your point but it is not about storing only but about generating too. why?

Copy link
Contributor Author

@cthulhu-rider cthulhu-rider Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have to store it because we must not loose it between stages (which is really possible). This also relates to neo-project/proposals#161 that will simplify this stuff with distributed single private key

well, u right that we could narrow interface to just key storage w/o randomizing or even binary storage, but i dont think it's worth really much

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have to store it because we must not loose it between stages

do you mean that initialization should be idempotent? if no, can just a local variable save us?

Copy link
Contributor Author

@cthulhu-rider cthulhu-rider Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, as much as possible ofc. File can dissapear and there is nothing else we could do with this. But in 97% cases this will work

didn't got ur point about local variable. The main goal is to create single key, and save it until it'll be persisted in the Sidechain. We could start the app, init key, use it for NNS deployment, but then fail just before key is saved on the chain (it's saved in the NNS). Then we restart application and desire to continue working with the same key. If it accidentally dissapear (e.g. file purged), there is nothing else we can do but init new key

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is nothing else we can do but init new key

is it a problem? if no key has persisted, i would just init the next one? but if a key has persisted successfully, is it possible to just read it? mb i got the idea wrong

@roman-khimov

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the key is on the chain, that's where the key is. If it's not, it doesn't exist and we don't care which one is gonna be used for the next attempt. The only problematic case probably is if you have an in-flight transaction that sets the key, but you restart before it's processed. This can be solved by always waiting for a new block initially (so that it'll bring this transaction with it), when the leader goes into this routine. It's not precisely accurate, but a sufficiently good way.

@cthulhu-rider
Copy link
Contributor Author

IMO, sometimes it is too info.

what exactly do u mean? iiuc u suggest to write more messages with debug severity?

@notimetoname
Copy link
Contributor

iiuc u suggest to write more messages with debug severity?

Yes, I've found 43 Info logs. Are all of them necessary for admins? Many of them contain specific words ("committee", "NNS", "transaction").

@cthulhu-rider
Copy link
Contributor Author

cthulhu-rider commented Jun 27, 2023

Yes, I've found 43 Info logs. Are all of them necessary for admins? Many of them contain specific words ("committee", "NNS", "transaction").

yes, i'd say it's necessary cuz this is an automatic decentralized scheme which also awaits for background fixes which could be necessary in some situations. Normally there will be not so many messages, but if smth will stuck u'll need to realize what's goin on and how we've come to this: here log will help u

overall i agree with u that sometimes logs just bring us to tears, but overall it's better to have some message and not read it than miss some meaningful one

There is a need to initialize running Neo network intended to be NeoFS
Sidechain in automatic mode. To do this, a group of committee nodes
working in the NeoFS Inner Ring should conduct a set of operations on
the blockchain.

This commit introduces partially implemented deployment procedure incl.
NNS deployment, Notary service and committee group initialization.
Deployment of NeoFS-system and custom smart contracts will be
implemented in the future. Since procedure is incomplete, it's not yet
used in the Inner Ring application.

Refs nspcc-dev#2195.

Signed-off-by: Leonard Lyubich <[email protected]>
Copy link
Contributor

@notimetoname notimetoname left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only looked at the commented changes and GH can't easily compare old/new PR state, but it should be OK otherwise.

Too.

@cthulhu-rider cthulhu-rider merged commit 5ca26a8 into nspcc-dev:master Jun 27, 2023
@cthulhu-rider cthulhu-rider deleted the feature/nns-autodeploy branch June 27, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants