search

nspcc-dev / neofs-contract

NeoFS smart-contract
GNU General Public License v3.0
10 stars 17 forks source link

deploy: copy transaction expected to be changed #417

Closed carpawell closed 4 months ago

carpawell commented 4 months ago

The whole makeUnsignedDesignateCommitteeNotaryTx is designed to be called many times if something goes wrong. But currently neo-go's actor shares signers with TXs it produces, so the actor is broken immediately after this func is called. Can be seen only if transaction-to-sign is expired, in practice happened when deployment nodes were started with a huge delay.

cthulhu-rider commented 4 months ago

shared memory is not the root of the problem. According to the actor concept, the resulting tx should not be modified due to calculated fees. Therefore, we should use a special actor instead of modifying the tx

the closest approach could be TransactionModifier opt, but

MakeUnsigned* methods do not run it.

lets create an issue in Neo Go project to find the best solution

carpawell commented 4 months ago

shared memory is not the root of the problem

What do you mean? We are not allowed to run our tests in some cases. With this PR it can be done now. It either works or not. Do you want to change it to the actors in this PR? Why?

lets create an issue in Neo Go project to find the best solution

How should it sound?

cthulhu-rider commented 4 months ago

It either works or not.

definitely it should work. But proposed solution is a crutch to me: modifying actor-prepared txs is unsafe and can lead to incorrect behavior. Since in this case the crutch is working, we can merge it only with issue+FIXME

How should it sound?

i'll do my best to create it

carpawell commented 4 months ago

we can merge it only with issue+FIXME

https://github.com/nspcc-dev/neofs-contract/issues/418. I want to be sure any bigger change works and want to see if there are more places with similar problems. Currently, I have to concentrate on deployment and just want a fix in the master.