Skip to content

Add system fee refundable attribute #2905

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add system fee refundable attribute #2905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
18b7900
core: fix #2845 system fee refund attribute
Feb 9, 2023
238f254
core: fix #2845 Roman's feedback
Feb 13, 2023
d8bcb25
core: fix #2845 attribute type string
Feb 15, 2023
f9fb12a
core: anna's feedback (fix #2845)
Mar 1, 2023
1cb4dda
core: system fee refund ut (fix #2845)
Mar 2, 2023
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
View file
Open in desktop
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)
}
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
57 changes: 56 additions & 1 deletion pkg/core/blockchain_neotest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,52 @@ func TestBlockchain_AddBadBlock(t *testing.T) {
})
}

func TestBlockchain_RefundSystemFee(t *testing.T) {
bc, acc := chain.NewSingle(t)
e := neotest.NewExecutor(t, bc, acc, acc)
neoHash := e.NativeHash(t, nativenames.Neo)

bal0 := bc.GetUtilityTokenBalance(acc.ScriptHash())

// tx without sysfee refund
tx1 := e.NewUnsignedTx(t, neoHash, "symbol")
e.SignTx(t, tx1, 100000000, acc)
vm, err := neotest.TestInvoke(bc, tx1)
actualSysfee1 := vm.GasConsumed()
require.NoError(t, err)

block1 := e.NewUnsignedBlock(t, tx1)
e.SignBlock(block1)
err = bc.AddBlock(block1)
require.NoError(t, err)
bal1 := bc.GetUtilityTokenBalance(acc.ScriptHash())

// tx with sysfee refund
tx2 := e.NewUnsignedTx(t, neoHash, "symbol")
attr := transaction.Attribute{Type: transaction.RefundableSystemFeeT, Value: &transaction.RefundableSystemFee{}}
tx2.Attributes = append(tx2.Attributes, attr)
e.SignTx(t, tx2, 100000000, acc)
tx2.NetworkFee += bc.SystemFeeRefundCost()
vm, err = neotest.TestInvoke(bc, tx1)
actualSysfee2 := vm.GasConsumed()
require.NoError(t, err)
require.Equal(t, actualSysfee1, actualSysfee2)

block2 := e.NewUnsignedBlock(t, tx2)
e.SignBlock(block2)
err = bc.AddBlock(block2)
require.NoError(t, err)
bal2 := bc.GetUtilityTokenBalance(acc.ScriptHash())

gas1 := new(big.Int).Sub(bal0, bal1)
gas2 := new(big.Int).Sub(bal1, bal2)
extraNetfee := big.NewInt(int64(io.GetVarSize(&attr)) * int64(bc.FeePerByte()))
extraNetfee.Add(extraNetfee, big.NewInt(bc.SystemFeeRefundCost()))
expect := new(big.Int).Add(gas1, extraNetfee)
expect.Sub(expect, big.NewInt(100000000-actualSysfee1))
require.Equal(t, expect, gas2)
}

func TestBlockchain_GetHeader(t *testing.T) {
bc, acc := chain.NewSingle(t)
e := neotest.NewExecutor(t, bc, acc, acc)
Expand Down Expand Up @@ -1109,14 +1155,15 @@ func TestBlockchain_VerifyTx(t *testing.T) {
}

testScript := []byte{byte(opcode.PUSH1)}
newTestTx := func(t *testing.T, signer util.Uint160, script []byte) *transaction.Transaction {
newTestTx := func(t *testing.T, signer util.Uint160, script []byte, attrs ...transaction.Attribute) *transaction.Transaction {
tx := transaction.New(script, 1_000_000)
tx.Nonce = neotest.Nonce()
tx.ValidUntilBlock = e.Chain.BlockHeight() + 5
tx.Signers = []transaction.Signer{{
Account: signer,
Scopes: transaction.CalledByEntry,
}}
tx.Attributes = attrs
tx.NetworkFee = int64(io.GetVarSize(tx)+200 /* witness */) * bc.FeePerByte()
tx.NetworkFee += 1_000_000 // verification cost
return tx
Expand Down Expand Up @@ -1338,6 +1385,14 @@ func TestBlockchain_VerifyTx(t *testing.T) {
}}
require.NoError(t, bc.VerifyTx(tx))
})
t.Run("SystemFeeRefundableAttribute", func(t *testing.T) {
tx := newTestTx(t, h, testScript, transaction.Attribute{Type: transaction.RefundableSystemFeeT, Value: &transaction.RefundableSystemFee{}})
require.NoError(t, accs[0].SignTx(netmode.UnitTestNet, tx))
checkErr(t, core.ErrTxSmallNetworkFee, tx)
tx.NetworkFee += bc.SystemFeeRefundCost()
require.Nil(t, bc.VerifyTx(tx))
t.Log("success")
})
t.Run("Oracle", func(t *testing.T) {
cs := contracts.GetOracleContractState(t, pathToInternalContracts, validator.ScriptHash(), 0)
e.DeployContract(t, &neotest.Contract{
Expand Down
32 changes: 17 additions & 15 deletions pkg/core/interop/context.go
View file
Open in desktop
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
View file
Open in desktop
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
70 changes: 56 additions & 14 deletions pkg/core/native/policy.go
View file
Open in desktop
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 @@ -55,11 +59,12 @@ type Policy struct {
}

type PolicyCache struct {
execFeeFactor uint32
feePerByte int64
maxVerificationGas int64
storagePrice uint32
blockedAccounts []util.Uint160
execFeeFactor uint32
feePerByte int64
maxVerificationGas int64
storagePrice uint32
systemFeeRefundCost int64
blockedAccounts []util.Uint160
}

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

desc = newDescriptor("getSystemFeeRefundCost", smartcontract.IntegerType,
manifest.NewParameter("value", smartcontract.IntegerType))
md = newMethodAndPrice(p.GetSystemFeeRefundCost, 1<<15, callflag.ReadStates)
p.AddMethod(md, desc)

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

@AnnaShaleva AnnaShaleva Feb 16, 2023 •
edited
Loading

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,13 +155,15 @@ 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,
blockedAccounts: make([]util.Uint160, 0),
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 +185,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 +372,30 @@ func (p *Policy) unblockAccount(ic *interop.Context, args []stackitem.Item) stac
return stackitem.NewBool(true)
}

func (p *Policy) GetSystemFeeRefundCost(ic *interop.Context, args []stackitem.Item) stackitem.Item {
return stackitem.NewBigInteger(big.NewInt(p.GetSystemFeeRefundCostInternal(ic.DAO)))
}

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
View file
Open in desktop
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
Loading