Support modifying unsigned transaction by `actor.Actor`

[cthulhu-rider]

Is your feature request related to a problem? Please describe.

NeoFS chain deployment procedure uses actor to make unsigned transaction designating Notary role. The essential goal is to get transaction which has particular data:

• first signer
• nonce
• ValidUntilBlock

as it turns out, this code leads to two problems:

1. (coding) actor's underlying slice of signer is mutated
2. (conceptual) txs prepared by actors should not be modified ((c) @AnnaShaleva)

1 can be easily fixed by memcpy like here or with separated actor

2, as more fundamental, can be fixed by manual tx formation similar to: https://github.com/nspcc-dev/neo-go/blob/4ff2063539430d18f499fea32c347f6396fadf38/pkg/rpcclient/rolemgmt/roles.go#L85. This requires having method name and having knowledge that https://pkg.go.dev/github.com/nspcc-dev/neo-go@v0.106.2/pkg/rpcclient/actor#Actor.CalculateNetworkFee should be done. While this is achievable, this is not very obvious and laconic as using actors

Describe the solution you'd like

support tx modiier by https://pkg.go.dev/github.com/nspcc-dev/neo-go@v0.106.2/pkg/rpcclient/actor#Actor.MakeUnsignedCall. There is https://pkg.go.dev/github.com/nspcc-dev/neo-go@v0.106.2/pkg/rpcclient/actor#Options but:

MakeUnsigned* methods do not run it.

seems like nothing stops methods creating unsigned txs to provide similar interceptor. For example, it can be called here https://github.com/nspcc-dev/neo-go/blob/4ff2063539430d18f499fea32c347f6396fadf38/pkg/rpcclient/actor/maker.go#L185 note that in this case method should not do useless work: if caller set its own ValidUntilBlock (with full responsibility), then there is no need to calculate it

Describe alternatives you've considered

more manual tx creation based on https://pkg.go.dev/github.com/nspcc-dev/neo-go@v0.106.2/pkg/rpcclient/invoker#Invoker.Call, e.g. make reader:

func (c *ContractReader) CallDesignateAsRole(role noderoles.Role, index uint32) (*result.Invoke, error)

[AnnaShaleva]

1. (coding) actor's underlying slice of signer is mutated

We need to extend actor's documentation to describe this behaviour. It should be explicitly prohibited to modify the signers passed to Actor's constructor. But I don't think we need to copy signers inside Actor's constructor since it adds resources consumption overhead to the constructor, and signers slice modification is not a hot use-case among Actor usages.

2. (conceptual) txs prepared by actors should not be modified ((c) @AnnaShaleva)

After a conversation with Roman this statement should be adjusted: signed transactions should not be modified by users, and unsigned transactions may be modified if there's a need (but wrt the issue 1). But it seems to me that introducing custom CheckerModifier to actor.MakeUnsignedCall and actor.MakeUnsignedRun slightly breaks actor's design because custom CheckerModifier supposed to be called only by those actor methods that sign transaction, i.e. by those methods that don't have other ways to customize transaction before signing. And if some further modification is needed, then user may use actor.MakeUnsignedUncheckedRun with additional modification.

So it seems to me that by design, it's supposed that it's a user responsibility to perform further modifications over unsigned transactions if it's needed. And existing API is sufficient for user's needs (consider using actor.MakeUnsignedUncheckedRun with further user'defined modification code) @roman-khimov, do we really need to introduce custom CheckerModifier for actor.MakeUnsigned* methods?

[cthulhu-rider]

I don't think we need to copy signers inside Actor

me neither. I should have clarified that for this problem I meant memcpy by the caller (like this)

[cthulhu-rider]

here is what can i do now:

    var a actor.Actor // with overridden signers
    const method = "designateAsRole"
    r, err := a.Call(rolemgmt.Hash, method, int(noderoles.P2PNotary), committee)
    if err != nil {
        return nil, fmt.Errorf("call %q method: %w", method, err)
    } else if r.State != vmstate.Halt.String() {
        // as actor.DefaultCheckerModifier
        return nil, fmt.Errorf("script failed (%s state) due to an error: %s", r.State, r.FaultException)
    }
    tx, err := a.MakeUnsignedUncheckedRun(r.Script, r.GasConsumed, nil)
    if err != nil {
        return nil, err
    }
    // MakeUnsignedUncheckedRun docs say:
    // > calculates proper ValidUntilBlock
    // and
    // > The resulting transaction can be changed in its Nonce, ValidUntilBlock, ...
    // so how to prevent double calculation? 
    tx.ValidUntilBlock = sharedTxData.validUntilBlock
    tx.Nonce = sharedTxData.nonce

well, it's not as huge as it could be tbh

---

i left one question in code ^. And im still requsting contract-specific helper (as in issue body)

r, err := roleContract.CallDesignateAsRole(noderoles.P2PNotary, committee)

is it possible to provide?

[AnnaShaleva]

// > The resulting transaction can be changed in its Nonce

Random nonce is embedded into transaction constructor, I doubt we need to change it and it doesn't cost a lot anyway: https://github.com/nspcc-dev/neo-go/blob/c207b9b194112825d4ca2b4c85ebfc991321af52/pkg/core/transaction/transaction.go#L96

, ValidUntilBlock,

May add an option to omit a call to (a *Actor) CalculateValidUntilBlock() but then you can't use this actor for instant signing anymore since it will always use this option and omit VUB calculation which makes this actor kind of flawed. I'm not in favor of it.

...

Which fields are you interested in besides these two? For NetworkFee it's supposed that you'll use the one calculated by Actor as a basis and add something to it if necessary.

// so how to prevent double calculation?

Given these facts double-calculation doesn't seem to be a problem for me.

r, err := roleContract.CallDesignateAsRole(noderoles.P2PNotary, committee)

But don't we have contract-specific wrapper for native designation contract? Check it here: https://github.com/nspcc-dev/neo-go/blob/c207b9b194112825d4ca2b4c85ebfc991321af52/pkg/rpcclient/rolemgmt/roles.go#L95-L100

[cthulhu-rider]

• see again mentioned problem code

Random nonce

not an option for my case, i have particular shared nonce

Which fields are you interested in besides these two?

none

double-calculation doesn't seem to be a problem

its not a problem if i code like this

But don't we have contract-specific wrapper for native designation contract?

it's used in the example code and conceptually not suitable for modifying a transaction. Im asking about a method that makes test invocation and returns result.Invoke (wrapper over invoker)

[roman-khimov]

Unsigned transaction can be modified (well, with some limitations of course). What you really want is to change VUB and nonce and that's absolutely feasible with the current code. I don't really understand why you want to change the first signer, you can have an actor with a proper signer for this case easily.

[cthulhu-rider]

i need actor to take my nonce and vub and not calc them himself. There is no way to do this

[AnnaShaleva]

But you may just change the resulting unsigned transaction. Calculation overhead is insignificant.

[roman-khimov]

Even modifiers we support for "signed" operations don't prevent actor from auto vub/nonce settings. They can override, but defaults are there anyway.

[cthulhu-rider]

Calculation overhead is insignificant.

this is not visible to a user like me, but ok i believe u