Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add system fee refundable attribute #2905

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions pkg/core/blockchain.go
Original file line number Diff line number Diff line change
Expand Up @@ -1418,6 +1418,7 @@ func (bc *Blockchain) storeBlock(block *block.Block, txpool *mempool.Pool) error
cache = bc.dao.GetPrivate()
aerCache = bc.dao.GetPrivate()
appExecResults = make([]*state.AppExecResult, 0, 2+len(block.Transactions))
txesConsumed = make([]int64, len(block.Transactions))
aerchan = make(chan *state.AppExecResult, len(block.Transactions)/8) // Tested 8 and 4 with no practical difference, but feel free to test more and tune.
aerdone = make(chan error)
)
Expand Down Expand Up @@ -1500,7 +1501,7 @@ func (bc *Blockchain) storeBlock(block *block.Block, txpool *mempool.Pool) error
close(aerdone)
}()
_ = cache.GetItemCtx() // Prime serialization context cache (it'll be reused by upper layer DAOs).
aer, v, err := bc.runPersist(bc.contracts.GetPersistScript(), block, cache, trigger.OnPersist, nil)
aer, v, err := bc.runPersist(bc.contracts.GetPersistScript(), block, cache, trigger.OnPersist, nil, nil)
if err != nil {
// Release goroutines, don't care about errors, we already have one.
close(aerchan)
Expand All @@ -1510,7 +1511,7 @@ func (bc *Blockchain) storeBlock(block *block.Block, txpool *mempool.Pool) error
appExecResults = append(appExecResults, aer)
aerchan <- aer

for _, tx := range block.Transactions {
for i, tx := range block.Transactions {
systemInterop := bc.newInteropContext(trigger.Application, cache, block, tx)
systemInterop.ReuseVM(v)
v.LoadScriptWithFlags(tx.Script, callflag.All)
Expand All @@ -1533,6 +1534,7 @@ func (bc *Blockchain) storeBlock(block *block.Block, txpool *mempool.Pool) error
zap.Error(err))
faultException = err.Error()
}
txesConsumed[i] = v.GasConsumed()
aer := &state.AppExecResult{
Container: tx.Hash(),
Execution: state.Execution{
Expand All @@ -1548,7 +1550,7 @@ func (bc *Blockchain) storeBlock(block *block.Block, txpool *mempool.Pool) error
aerchan <- aer
}

aer, _, err = bc.runPersist(bc.contracts.GetPostPersistScript(), block, cache, trigger.PostPersist, v)
aer, _, err = bc.runPersist(bc.contracts.GetPostPersistScript(), block, cache, trigger.PostPersist, v, txesConsumed)
if err != nil {
// Release goroutines, don't care about errors, we already have one.
close(aerchan)
Expand Down Expand Up @@ -1683,13 +1685,14 @@ func (bc *Blockchain) IsExtensibleAllowed(u util.Uint160) bool {
return n < len(us)
}

func (bc *Blockchain) runPersist(script []byte, block *block.Block, cache *dao.Simple, trig trigger.Type, v *vm.VM) (*state.AppExecResult, *vm.VM, error) {
func (bc *Blockchain) runPersist(script []byte, block *block.Block, cache *dao.Simple, trig trigger.Type, v *vm.VM, txesConsumed []int64) (*state.AppExecResult, *vm.VM, error) {
systemInterop := bc.newInteropContext(trig, cache, block, nil)
if v == nil {
v = systemInterop.SpawnVM()
} else {
systemInterop.ReuseVM(v)
}
systemInterop.TxesConsumed = txesConsumed
v.LoadScriptWithFlags(script, callflag.All)
if err := systemInterop.Exec(); err != nil {
return nil, v, fmt.Errorf("VM has failed: %w", err)
Expand Down Expand Up @@ -2248,6 +2251,11 @@ func (bc *Blockchain) FeePerByte() int64 {
return bc.contracts.Policy.GetFeePerByteInternal(bc.dao)
}

// GasRefundFee returns extra fee for system fee refundable transaction
func (bc *Blockchain) SystemFeeRefundCost() int64 {
return bc.contracts.Policy.GetSystemFeeRefundCostInternal(bc.dao)
}

// GetMemPool returns the memory pool of the blockchain.
func (bc *Blockchain) GetMemPool() *mempool.Pool {
return bc.memPool
Expand Down Expand Up @@ -2367,6 +2375,9 @@ func (bc *Blockchain) verifyAndPoolTx(t *transaction.Transaction, pool *mempool.
needNetworkFee += (int64(na.NKeys) + 1) * bc.contracts.Notary.GetNotaryServiceFeePerKey(bc.dao)
}
}
if len(t.GetAttributes(transaction.RefundableSystemFeeT)) > 0 {
needNetworkFee += bc.SystemFeeRefundCost()
}
netFee := t.NetworkFee - needNetworkFee
if netFee < 0 {
return fmt.Errorf("%w: net fee is %v, need %v", ErrTxSmallNetworkFee, t.NetworkFee, needNetworkFee)
Expand Down Expand Up @@ -2480,6 +2491,11 @@ func (bc *Blockchain) verifyTxAttributes(d *dao.Simple, tx *transaction.Transact
if !tx.HasSigner(bc.contracts.Notary.Hash) {
return fmt.Errorf("%w: NotaryAssisted attribute was found, but transaction is not signed by the Notary native contract", ErrInvalidAttribute)
}
case transaction.RefundableSystemFeeT:
state := bc.GetContractState(tx.Sender())
if state != nil {
return fmt.Errorf("%w: RefundableSystemFee attribute was found, but transaction sender is contract", ErrInvalidAttribute)
}
AnnaShaleva marked this conversation as resolved.
Show resolved Hide resolved
default:
if !bc.config.ReservedAttributes && attrType >= transaction.ReservedLowerBound && attrType <= transaction.ReservedUpperBound {
return fmt.Errorf("%w: attribute of reserved type was found, but ReservedAttributes are disabled", ErrInvalidAttribute)
Expand Down
32 changes: 17 additions & 15 deletions pkg/core/interop/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,21 +45,23 @@ type Ledger interface {

// Context represents context in which interops are executed.
type Context struct {
Chain Ledger
Container hash.Hashable
Network uint32
Hardforks map[string]uint32
Natives []Contract
Trigger trigger.Type
Block *block.Block
NonceData [16]byte
Tx *transaction.Transaction
DAO *dao.Simple
Notifications []state.NotificationEvent
Log *zap.Logger
VM *vm.VM
Functions []Function
Invocations map[util.Uint160]int
Chain Ledger
Container hash.Hashable
Network uint32
Hardforks map[string]uint32
Natives []Contract
Trigger trigger.Type
Block *block.Block
NonceData [16]byte
Tx *transaction.Transaction
DAO *dao.Simple
Notifications []state.NotificationEvent
Log *zap.Logger
VM *vm.VM
Functions []Function
Invocations map[util.Uint160]int
TxesConsumed []int64

cancelFuncs []context.CancelFunc
getContract func(*dao.Simple, util.Uint160) (*state.Contract, error)
baseExecFee int64
Expand Down
9 changes: 9 additions & 0 deletions pkg/core/native/native_gas.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,15 @@ func (g *GAS) OnPersist(ic *interop.Context) error {

// PostPersist implements the Contract interface.
func (g *GAS) PostPersist(ic *interop.Context) error {
for i, tx := range ic.Block.Transactions {
attrs := tx.GetAttributes(transaction.RefundableSystemFeeT)
if len(attrs) != 0 {
consumed := ic.TxesConsumed[i]
if consumed < tx.SystemFee {
g.mint(ic, tx.Sender(), big.NewInt(tx.SystemFee-consumed), false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
return nil
}

Expand Down
41 changes: 37 additions & 4 deletions pkg/core/native/policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ import (
const (
policyContractID = -7

defaultExecFeeFactor = interop.DefaultBaseExecFee
defaultFeePerByte = 1000
defaultMaxVerificationGas = 1_50000000
defaultExecFeeFactor = interop.DefaultBaseExecFee
defaultFeePerByte = 1000
defaultMaxVerificationGas = 1_50000000
defaultSystemFeeRefundCost = 0_10000000
// DefaultStoragePrice is the price to pay for 1 byte of storage.
DefaultStoragePrice = 100000

Expand All @@ -33,7 +34,8 @@ const (
maxFeePerByte = 100_000_000
// maxStoragePrice is the maximum allowed price for a byte of storage.
maxStoragePrice = 10000000

// maxSystemFeeRefundCost is the maximun allowed extra fee for gas refundable transaction
maxSystemFeeRefundCost = 1_00000000
// blockedAccountPrefix is a prefix used to store blocked account.
blockedAccountPrefix = 15
)
Expand All @@ -46,6 +48,8 @@ var (
feePerByteKey = []byte{10}
// storagePriceKey is a key used to store storage price.
storagePriceKey = []byte{19}
// systemFeeRefundCostKey is a key usesd to store gas refund fee
systemFeeRefundCostKey = []byte{20}
)

// Policy represents Policy native contract.
Expand All @@ -59,6 +63,7 @@ type PolicyCache struct {
feePerByte int64
maxVerificationGas int64
storagePrice uint32
systemFeeRefundCost int64
blockedAccounts []util.Uint160
}

Expand Down Expand Up @@ -127,6 +132,11 @@ func newPolicy() *Policy {
md = newMethodAndPrice(p.unblockAccount, 1<<15, callflag.States)
p.AddMethod(md, desc)

desc = newDescriptor("setSystemFeeRefundCost", smartcontract.VoidType,
Copy link
Member

@AnnaShaleva AnnaShaleva Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you, please, add getSystemFeeRefundCost method to the Policy manifest as far?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you, please, update the native Policy interop wrapper (https://github.com/nspcc-dev/neo-go/blob/master/pkg/interop/native/policy/policy.go)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

manifest.NewParameter("value", smartcontract.IntegerType))
md = newMethodAndPrice(p.setSystemFeeRefundCost, 1<<15, callflag.States)
p.AddMethod(md, desc)

return p
}

Expand All @@ -140,12 +150,14 @@ func (p *Policy) Initialize(ic *interop.Context) error {
setIntWithKey(p.ID, ic.DAO, feePerByteKey, defaultFeePerByte)
setIntWithKey(p.ID, ic.DAO, execFeeFactorKey, defaultExecFeeFactor)
setIntWithKey(p.ID, ic.DAO, storagePriceKey, DefaultStoragePrice)
setIntWithKey(p.ID, ic.DAO, systemFeeRefundCostKey, defaultSystemFeeRefundCost)

cache := &PolicyCache{
execFeeFactor: defaultExecFeeFactor,
feePerByte: defaultFeePerByte,
maxVerificationGas: defaultMaxVerificationGas,
storagePrice: DefaultStoragePrice,
systemFeeRefundCost: defaultSystemFeeRefundCost,
blockedAccounts: make([]util.Uint160, 0),
}
ic.DAO.SetCache(p.ID, cache)
Expand All @@ -168,6 +180,7 @@ func (p *Policy) fillCacheFromDAO(cache *PolicyCache, d *dao.Simple) error {
cache.feePerByte = getIntWithKey(p.ID, d, feePerByteKey)
cache.maxVerificationGas = defaultMaxVerificationGas
cache.storagePrice = uint32(getIntWithKey(p.ID, d, storagePriceKey))
cache.systemFeeRefundCost = getIntWithKey(p.ID, d, systemFeeRefundCostKey)

cache.blockedAccounts = make([]util.Uint160, 0)
var fErr error
Expand Down Expand Up @@ -354,6 +367,26 @@ func (p *Policy) unblockAccount(ic *interop.Context, args []stackitem.Item) stac
return stackitem.NewBool(true)
}

func (p *Policy) GetSystemFeeRefundCostInternal(d *dao.Simple) int64 {
cache := d.GetROCache(p.ID).(*PolicyCache)
return cache.systemFeeRefundCost
}

// setSystemFeeRefundCost is a Policy contract method that set extra network fee for gas refundable transaction.
func (p *Policy) setSystemFeeRefundCost(ic *interop.Context, args []stackitem.Item) stackitem.Item {
value := toBigInt(args[0]).Int64()
if value < 0 || value > maxSystemFeeRefundCost {
panic(fmt.Errorf("SystemFeeRefundCost shouldn't be negative or greater than %d", maxSystemFeeRefundCost))
}
if !p.NEO.checkCommittee(ic) {
panic("invalid committee signature")
}
setIntWithKey(p.ID, ic.DAO, systemFeeRefundCostKey, value)
cache := ic.DAO.GetRWCache(p.ID).(*PolicyCache)
cache.systemFeeRefundCost = value
return stackitem.Null{}
}

// CheckPolicy checks whether a transaction conforms to the current policy restrictions,
// like not being signed by a blocked account or not exceeding the block-level system
// fee limit.
Expand Down
7 changes: 6 additions & 1 deletion pkg/core/transaction/attribute.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ func (attr *Attribute) DecodeBinary(br *io.BinReader) {
attr.Value = new(Conflicts)
case NotaryAssistedT:
attr.Value = new(NotaryAssisted)
case RefundableSystemFeeT:
attr.Value = new(RefundableSystemFee)
default:
if t >= ReservedLowerBound && t <= ReservedUpperBound {
attr.Value = new(Reserved)
Expand All @@ -57,7 +59,7 @@ func (attr *Attribute) EncodeBinary(bw *io.BinWriter) {
bw.WriteB(byte(attr.Type))
switch t := attr.Type; t {
case HighPriority:
case OracleResponseT, NotValidBeforeT, ConflictsT, NotaryAssistedT:
case OracleResponseT, NotValidBeforeT, ConflictsT, NotaryAssistedT, RefundableSystemFeeT:
attr.Value.EncodeBinary(bw)
default:
if t >= ReservedLowerBound && t <= ReservedUpperBound {
Expand Down Expand Up @@ -102,6 +104,9 @@ func (attr *Attribute) UnmarshalJSON(data []byte) error {
case NotaryAssistedT.String():
attr.Type = NotaryAssistedT
attr.Value = new(NotaryAssisted)
case RefundableSystemFeeT.String():
attr.Type = RefundableSystemFeeT
attr.Value = new(RefundableSystemFee)
default:
return errors.New("wrong Type")
}
Expand Down
11 changes: 6 additions & 5 deletions pkg/core/transaction/attrtype.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@ const (

// List of valid attribute types.
const (
HighPriority AttrType = 1
OracleResponseT AttrType = 0x11 // OracleResponse
NotValidBeforeT AttrType = 0x20 // NotValidBefore
ConflictsT AttrType = 0x21 // Conflicts
NotaryAssistedT AttrType = 0x22 // NotaryAssisted
HighPriority AttrType = 1
OracleResponseT AttrType = 0x11 // OracleResponse
NotValidBeforeT AttrType = 0x20 // NotValidBefore
ConflictsT AttrType = 0x21 // Conflicts
NotaryAssistedT AttrType = 0x22 // NotaryAssisted
RefundableSystemFeeT AttrType = 0x30 // RefundableSystemFee
AnnaShaleva marked this conversation as resolved.
Show resolved Hide resolved
)

func (a AttrType) allowMultiple() bool {
Expand Down
20 changes: 20 additions & 0 deletions pkg/core/transaction/refundablesystemfee.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package transaction

import (
"github.com/nspcc-dev/neo-go/pkg/io"
)

// Conflicts represents attribute for refund gas transaction.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Conflicts/RefundableSystemFee

type RefundableSystemFee struct {
}

// DecodeBinary implements the io.Serializable interface.
func (c *RefundableSystemFee) DecodeBinary(br *io.BinReader) {
}

// EncodeBinary implements the io.Serializable interface.
func (c *RefundableSystemFee) EncodeBinary(w *io.BinWriter) {
}

func (c *RefundableSystemFee) toJSONMap(m map[string]interface{}) {
}
4 changes: 4 additions & 0 deletions pkg/services/rpcsrv/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ type (
CalculateClaimable(h util.Uint160, endHeight uint32) (*big.Int, error)
CurrentBlockHash() util.Uint256
FeePerByte() int64
SystemFeeRefundCost() int64
ForEachNEP11Transfer(acc util.Uint160, newestTimestamp uint64, f func(*state.NEP11Transfer) (bool, error)) error
ForEachNEP17Transfer(acc util.Uint160, newestTimestamp uint64, f func(*state.NEP17Transfer) (bool, error)) error
GetAppExecResults(util.Uint256, trigger.Type) ([]state.AppExecResult, error)
Expand Down Expand Up @@ -847,6 +848,9 @@ func (s *Server) calculateNetworkFee(reqParams params.Params) (interface{}, *neo
netFee += (int64(na.NKeys) + 1) * s.chain.GetNotaryServiceFeePerKey()
}
}
if len(tx.GetAttributes(transaction.RefundableSystemFeeT)) > 0 {
netFee += s.chain.SystemFeeRefundCost()
}
Comment on lines +851 to +853
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless it goes away earlier than this PR gets in.

fee := s.chain.FeePerByte()
netFee += int64(size) * fee
return result.NetworkFee{Value: netFee}, nil
Expand Down