deploy: transfer GAS in the first place if notaries are set #410
Conversation
deploy/deploy.go
Outdated
| @@ -645,3 +684,52 @@ func neoFSRuntimeTransactionModifier(getBlockchainHeight func() uint32) actor.Tr | |||
| return nil | |||
| } | |||
| } | |||
|
|
|||
| func rolesAreSet(b Blockchain, committee keys.PublicKeys, logger *zap.Logger) (bool, error) { | |||
There was a problem hiding this comment.
this essentially is a combination of same checks done in enableNotary and designateNeoFSAlphabet, i suggest to share and reuse the code
There was a problem hiding this comment.
they do almost the same things now, do you think enableNotary and designateNeoFSAlphabet need this check after this PR?
There was a problem hiding this comment.
ofc they need: each time (block) they attempt to set roles - they need to check if it worked or not
to be more precise, my discussion is not only about the code sharing but diff behavior. IIUC
rolesAsSetchecks exact equality of committee and role holdersenableNotaryanddesignateNeoFSAlphabetjust check committee occurrence by subset (ignoring situation when there are some role members besides the committee)
so, code sharing would prevent such diff
lets revise desired behavior. 2 is weaker, and deploy mechanics should work well in this case (never tested tbh). Can it cause any problems idk about? @roman-khimov @AnnaShaleva
There was a problem hiding this comment.
do you think enableNotary and designateNeoFSAlphabet need this check after this PR?
ofc they need
with this PR, roles are either set or not, enableNotary can be called if Notary role is not set and can be not called at all otherwise
each time (block) they attempt to set roles - they need to check if it worked or not
this code surprised me. my practice says that a correct TX acceptance is enough, otherwise our life is too paranoid here. neofs-adm does not do such additional checks, as i remember
enableNotary and designateNeoFSAlphabet just check committee occurrence by subset
do we really expect and are ready for committee < Notary/Alphabet case?
There was a problem hiding this comment.
adm has direct access to all committee and consensus accounts, ofc its behavior is simpler. Here only 1 account acts, i dont think logic can be easier than now
There was a problem hiding this comment.
i dont think logic can be easier than now
we have TX, we have VUB, why do we need a polling mechanism?
i do not say it is already done (or should be done) in this PR, i just want to make clear what exact code you want to share. currently, it seems to me like a wrong assumption that setting a role should be checked with getting a role (every block). if we agree that it is bad, i do not think sharing is worth it then, we just need to create an issue and this part will be removed
There was a problem hiding this comment.
do we really expect and are ready for committee < Notary/Alphabet case?
committee < alphabet is impossible to me, maybe unless you play with CommitteeHistory/ValidatorsHistory in some bad way, but even then validators are always a subset of committee and alphabet nodes are always validators. Committee and Notary role relations allow for a bit more freedom, one can technically have more notary nodes than we have them in committee, but I don't know any real use case for this. Either way, current code always sets both to the committee (kicking everyone else out), so on a properly configured system we can expect them to be in a perfect sync. Therefore, we can check for exact match everywhere.
we have TX, we have VUB, why do we need a polling mechanism?
You have to probe for configuration (read/compare lists) anyway to see if it needs to be changed. This covers for transaction execution side-effects perfectly which are more reliable than just waiting for successful transaction execution. Can't say that just waiting for the transaction outcome is wrong, it works as well (you know how contracts you're calling work, they will fail the transaction in case anything is wrong) and it's a bit more efficient (subscriptions!), but since you already have some code that reads configurations reusing it isn't that bad either.
There was a problem hiding this comment.
i do not understand. i have shared the code (that has not been shared b/w roles itself before this PR too) that i think should be different
There was a problem hiding this comment.
transaction is just a mechanism, routine wait for particular state, it does not care how this state was achieved. This covers two additional cases:
- if an earlier transaction has already been completed, there is no need to wait for this one, it will be no-op anyway
- if the transaction is successful, but the state still does not satisfying
so I see no reason to do different pre-checks and post-checks, the state condition should be checked uniformly
It is possible to start blockchain with required roles already set. In this case, there is no need to wait for NNS to be deployed for GAS transfers, since notary service (requires Notary roles to be set correctly) is ready. It allows all multi-signed operations to be applied at an earlier stage. Closes #409. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
c1bbe5c to
9095b3e
Compare
5317901 to
e5f8adb
Compare
e5f8adb to
03e403e
Compare
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
13e9684
03e403e to
13e9684
Compare
It is possible to start blockchain with required roles already set. In this case, there is no need to wait for NNS to be deployed for GAS transfers, since notary service (requires Notary roles to be set correctly) is ready. It allows all multi-signed operations to be applied at an earlier stage. Closes #409.