Support modifying unsigned transaction by `actor.Actor`

sami commented

2025-12-28 17:16:04 +00:00

Owner

Originally created by @cthulhu-rider on GitHub (Jul 15, 2024).

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:

(coding) actor's underlying slice of signer is mutated
(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:
nspcc-dev/neo-go@4ff2063539/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 nspcc-dev/neo-go@4ff2063539/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)

Originally created by @cthulhu-rider on GitHub (Jul 15, 2024). ## Is your feature request related to a problem? Please describe. NeoFS chain deployment procedure uses actor to [make](https://github.com/nspcc-dev/neofs-contract/blob/3072d7baa88a050882e3070269c6ca8d439ba686/deploy/notary.go#L821-L839) 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](https://github.com/nspcc-dev/neofs-contract/pull/417) 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](https://pkg.go.dev/github.com/nspcc-dev/neo-go@v0.106.2/pkg/rpcclient/rolemgmt#ContractReader): ```go func (c *ContractReader) CallDesignateAsRole(role noderoles.Role, index uint32) (*result.Invoke, error) ```

sami added the

labels

2025-12-28 17:16:04 +00:00

sami commented

2025-12-28 17:16:04 +00:00

Author

Owner

@AnnaShaleva commented on GitHub (Jul 15, 2024):

(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.

(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?

@AnnaShaleva commented on GitHub (Jul 15, 2024): > 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?

sami commented

2025-12-28 17:16:04 +00:00

Author

Owner

@cthulhu-rider commented on GitHub (Jul 15, 2024):

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 commented on GitHub (Jul 15, 2024): > 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](https://github.com/nspcc-dev/neofs-contract/pull/417))

sami commented

2025-12-28 17:16:04 +00:00

Author

Owner

@cthulhu-rider commented on GitHub (Jul 15, 2024):

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?

@cthulhu-rider commented on GitHub (Jul 15, 2024): here is what can i do now: ```go 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) ```go r, err := roleContract.CallDesignateAsRole(noderoles.P2PNotary, committee) ``` is it possible to provide?

sami commented

2025-12-28 17:16:04 +00:00

Author

Owner

@AnnaShaleva commented on GitHub (Jul 16, 2024):

// > 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:
nspcc-dev/neo-go@c207b9b194/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:
nspcc-dev/neo-go@c207b9b194/pkg/rpcclient/rolemgmt/roles.go (L95-L100)

@AnnaShaleva commented on GitHub (Jul 16, 2024): > // > 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

sami commented

2025-12-28 17:16:04 +00:00

Author

Owner

@cthulhu-rider commented on GitHub (Jul 16, 2024):

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)

@cthulhu-rider commented on GitHub (Jul 16, 2024): * see again mentioned problem [code](https://github.com/nspcc-dev/neofs-contract/blob/3072d7baa88a050882e3070269c6ca8d439ba686/deploy/notary.go#L821-L839) > 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](https://github.com/nspcc-dev/neo-go/issues/3512#issuecomment-2228421239) > 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](https://pkg.go.dev/github.com/nspcc-dev/neo-go@v0.106.2/pkg/rpcclient/invoker#Invoker.Call))

sami commented

2025-12-28 17:16:04 +00:00

Author

Owner

@roman-khimov commented on GitHub (Jul 16, 2024):

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.

@roman-khimov commented on GitHub (Jul 16, 2024): 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.

sami commented

2025-12-28 17:16:04 +00:00

Author

Owner

@cthulhu-rider commented on GitHub (Jul 16, 2024):

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

@cthulhu-rider commented on GitHub (Jul 16, 2024): i need actor to take my nonce and vub and not calc them himself. There is no way to do this

sami commented

2025-12-28 17:16:04 +00:00

Author

Owner

@AnnaShaleva commented on GitHub (Jul 16, 2024):

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

@AnnaShaleva commented on GitHub (Jul 16, 2024): But you may just change the resulting unsigned transaction. Calculation overhead is insignificant.

sami commented

2025-12-28 17:16:04 +00:00

Author

Owner

@roman-khimov commented on GitHub (Jul 16, 2024):

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

@roman-khimov commented on GitHub (Jul 16, 2024): Even modifiers we support for "signed" operations don't prevent actor from auto vub/nonce settings. They can override, but defaults are there anyway.

sami commented

2025-12-28 17:16:05 +00:00

Author

Owner

@cthulhu-rider commented on GitHub (Jul 16, 2024):

Calculation overhead is insignificant.

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

@cthulhu-rider commented on GitHub (Jul 16, 2024): > Calculation overhead is insignificant. this is not visible to a user like me, but ok i believe u

Rows
Columns

Support modifying unsigned transaction by actor.Actor #1332

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

Describe the solution you'd like

Describe alternatives you've considered

Support modifying unsigned transaction by `actor.Actor` #1332