deploy: copy transaction expected to be changed #417
Conversation
| @@ -831,6 +831,8 @@ func makeUnsignedDesignateCommitteeNotaryTx(roleContract *rolemgmt.Contract, com | |||
| return nil, err | |||
| } | |||
|
|
|||
| tx = tx.Copy() | |||
There was a problem hiding this comment.
enough to copy and modify Signers if !tx.Signers[0].Account.Equals(sharedTxData.sender) only
There was a problem hiding this comment.
this is kinda hotfix to me, i do not agree with the code here, so something different can be done here in the future. do you really need one more if here?
There was a problem hiding this comment.
btw, this func is called only if !sharedTxDataMatches so suggested if is already done
There was a problem hiding this comment.
i do not agree with the code here, so something different can be done here in the future
then issue or to draft
There was a problem hiding this comment.
this is kinda hotfix to me
nvm, we dont modify other ref type fields, so copy only Signers
|
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
lets create an issue in Neo Go project to find the best solution |
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. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell
force-pushed
the
fix/role-setting-breaks-actor
branch
from
70357c6
to
a8d7fbc
Compare
carpawell
changed the title
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?
How should it sound? |
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
i'll do my best to create it |
#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. |
The whole
makeUnsignedDesignateCommitteeNotaryTxis 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.