Add system fee refundable attribute #2905
Conversation
ZhangTao1596
requested review from
AnnaShaleva,
roman-khimov and
fyrchik
as code owners
| if len(attrs) != 0 { | ||
| consumed := ic.TxesConsumed[tx.Hash()] | ||
| if consumed < tx.SystemFee { | ||
| g.mint(ic, tx.Sender(), big.NewInt(tx.SystemFee-consumed), false) |
There was a problem hiding this comment.
I'd still check tx.Sender() for being a contract address here. Contract addresses are predictable, one can precalculate it, send some GAS to it, use it as a sender in a transaction and deploy a contract in the same block (making regular address a contract address).
There was a problem hiding this comment.
Or actually this can't happen because to be a sender an account should not just have some GAS, but also pass a witness check. Contract scripts are very specific and not supposed to ever pass it if they're not deployed. When deployed, a previous check is already sufficient.
| "github.com/nspcc-dev/neo-go/pkg/io" | ||
| ) | ||
|
|
||
| // Conflicts represents attribute for refund gas transaction. |
There was a problem hiding this comment.
s/Conflicts/RefundableSystemFee
| if len(tx.GetAttributes(transaction.RefundableSystemFeeT)) > 0 { | ||
| netFee += s.chain.SystemFeeRefundCost() | ||
| } |
There was a problem hiding this comment.
We have deprecated (c *Client) AddNetworkFee method that is about to be removed, but still have to give a proper response. @roman-khimov, do we need to consider its updating?
There was a problem hiding this comment.
Unless it goes away earlier than this PR gets in.
| @@ -127,6 +132,11 @@ func newPolicy() *Policy { | |||
| md = newMethodAndPrice(p.unblockAccount, 1<<15, callflag.States) | |||
| p.AddMethod(md, desc) | |||
|
|
|||
| desc = newDescriptor("setSystemFeeRefundCost", smartcontract.VoidType, | |||
There was a problem hiding this comment.
Could you, please, add getSystemFeeRefundCost method to the Policy manifest as far?
| @@ -127,6 +132,11 @@ func newPolicy() *Policy { | |||
| md = newMethodAndPrice(p.unblockAccount, 1<<15, callflag.States) | |||
| p.AddMethod(md, desc) | |||
|
|
|||
| desc = newDescriptor("setSystemFeeRefundCost", smartcontract.VoidType, | |||
There was a problem hiding this comment.
Could you, please, update the native Policy interop wrapper (https://github.com/nspcc-dev/neo-go/blob/master/pkg/interop/native/policy/policy.go)?
| @@ -127,6 +132,11 @@ func newPolicy() *Policy { | |||
| md = newMethodAndPrice(p.unblockAccount, 1<<15, callflag.States) | |||
| p.AddMethod(md, desc) | |||
|
|
|||
| desc = newDescriptor("setSystemFeeRefundCost", smartcontract.VoidType, | |||
There was a problem hiding this comment.
Could you, please, update the Policy RPC client wrapper (https://github.com/nspcc-dev/neo-go/blob/master/pkg/rpcclient/policy/policy.go)? If I'm not mistaken, it can be done automatically with the help of RPC wrappers generator, @roman-khimov?
There was a problem hiding this comment.
Yes and no, those are hand-tuned l33t wrappers. Autogenerated ones can be used as a basis for new methods, but we need to have better comments (like other methods have now) anyway.
There was a problem hiding this comment.
Can we add a test for this attribute to ensure it works as intended? TestBlockchain_VerifyTx can be extended for verification code added here and then some test is needed to check two transactions executing some silly RET opcode with excessive system fee, one not using the attribute and another one using it.
Right now we're at the feasibility stage for this attribute/approach. It seems to be fine and if we can prove it with a test, then yes, it can be a very nice protocol change.
Will do. |
|
Still a nice example, but it's a protocol change that is not yet accepted upstream. |
Fix #2845