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.

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

Jump to bottom

Proper fix for gasUsed and fee in case of relayedV3 #6440

Merged
merged 4 commits into from
Sep 9, 2024
Merged

Proper fix for gasUsed and fee in case of relayedV3 #6440

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
506cd9f
proper fix for gasUsed and fee in case of relayedV3
sstanculeanu Sep 6, 2024
f3b4b13
fixes after review + extra test with multiple refunds
sstanculeanu Sep 6, 2024
51990e7
removed test
sstanculeanu Sep 6, 2024
398361a
simplify jsons
sstanculeanu Sep 6, 2024
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
26 changes: 18 additions & 8 deletions node/external/transactionAPI/gasUsedAndFeeProcessor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,16 @@ func (gfp *gasUsedAndFeeProcessor) computeAndAttachGasUsedAndFee(tx *transaction
tx.Fee = tx.InitiallyPaidFee
}

if tx.IsRelayed && isFeeFixActive {
totalFee, isRelayed := gfp.getFeeOfRelayed(tx)
if isRelayed {
tx.Fee = totalFee.String()
tx.InitiallyPaidFee = totalFee.String()
tx.GasUsed = big.NewInt(0).Div(totalFee, big.NewInt(0).SetUint64(tx.GasPrice)).Uint64()
}
initialTotalFee, isRelayed := gfp.getFeeOfRelayed(tx)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is basically the same as before right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

exactly, only removed one if

if isRelayed && isFeeFixActive {
tx.InitiallyPaidFee = initialTotalFee.String()
tx.Fee = initialTotalFee.String()
tx.GasUsed = big.NewInt(0).Div(initialTotalFee, big.NewInt(0).SetUint64(tx.GasPrice)).Uint64()
}

hasRefundForSender := false
totalRefunds := big.NewInt(0)
isRelayedV3 := len(tx.InnerTransactions) > 0
for _, scr := range tx.SmartContractResults {
if !scr.IsRefund || scr.RcvAddr != tx.Sender {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think line 67 is dead code right?
there was already the same check done on line 63 with a continue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

true, removed it

continue
Expand All @@ -69,7 +69,17 @@ func (gfp *gasUsedAndFeeProcessor) computeAndAttachGasUsedAndFee(tx *transaction

gfp.setGasUsedAndFeeBaseOnRefundValue(tx, scr.Value)
hasRefundForSender = true
break
totalRefunds.Add(totalRefunds, scr.Value)
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't this affect regular transactions with multiple refund scrs from other shards as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see.. it means this was a bug also for that transactions.. updated to iterate through all of them

if !isRelayedV3 {
break
}
}

if isRelayedV3 {
gasUsed, fee = gfp.feeComputer.ComputeGasUsedAndFeeBasedOnRefundValue(tx, totalRefunds)
tx.GasUsed = gasUsed
tx.Fee = fee.String()
return
}

gfp.prepareTxWithResultsBasedOnLogs(tx, hasRefundForSender)
Expand Down
15 changes: 13 additions & 2 deletions outport/process/transactionsfee/transactionsFeeProcessor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ func (tep *transactionsFeeProcessor) prepareNormalTxs(transactionsAndScrs *trans

func (tep *transactionsFeeProcessor) prepareTxWithResults(txHashHex string, txWithResults *transactionWithResults) {
hasRefund := false
totalRefunds := big.NewInt(0)
isRelayedV3 := len(txWithResults.GetTxHandler().GetUserTransactions()) > 0
for _, scrHandler := range txWithResults.scrs {
scr, ok := scrHandler.GetTxHandler().(*smartContractResult.SmartContractResult)
if !ok {
Expand All @@ -167,12 +169,21 @@ func (tep *transactionsFeeProcessor) prepareTxWithResults(txHashHex string, txWi
txWithResults.GetFeeInfo().SetGasUsed(gasUsed)
txWithResults.GetFeeInfo().SetFee(fee)
hasRefund = true
break
totalRefunds.Add(totalRefunds, scr.Value)
if !isRelayedV3 {
Copy link
Contributor

Choose a reason for hiding this comment

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

this will affect other transactions with multiple scrs in different shards as well, so I think it should iterate over all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see.. it means this was a bug also for that transactions.. updated to iterate through all of them

break
}
}
}

tep.prepareTxWithResultsBasedOnLogs(txHashHex, txWithResults, hasRefund)
if isRelayedV3 {
gasUsed, fee := tep.txFeeCalculator.ComputeGasUsedAndFeeBasedOnRefundValue(txWithResults.GetTxHandler(), totalRefunds)
txWithResults.GetFeeInfo().SetGasUsed(gasUsed)
txWithResults.GetFeeInfo().SetFee(fee)
return
}

tep.prepareTxWithResultsBasedOnLogs(txHashHex, txWithResults, hasRefund)
}

func (tep *transactionsFeeProcessor) getFeeOfRelayed(tx *transactionWithResults) (*big.Int, bool) {
Expand Down
103 changes: 103 additions & 0 deletions outport/process/transactionsfee/transactionsFeeProcessor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,13 @@ import (
outportcore "github.com/multiversx/mx-chain-core-go/data/outport"
"github.com/multiversx/mx-chain-core-go/data/smartContractResult"
"github.com/multiversx/mx-chain-core-go/data/transaction"
"github.com/multiversx/mx-chain-go/common"
"github.com/multiversx/mx-chain-go/outport/mock"
"github.com/multiversx/mx-chain-go/process"
"github.com/multiversx/mx-chain-go/process/economics"
"github.com/multiversx/mx-chain-go/testscommon"
"github.com/multiversx/mx-chain-go/testscommon/enableEpochsHandlerMock"
"github.com/multiversx/mx-chain-go/testscommon/epochNotifier"
"github.com/multiversx/mx-chain-go/testscommon/genericMocks"
"github.com/multiversx/mx-chain-go/testscommon/marshallerMock"
logger "github.com/multiversx/mx-chain-logger-go"
Expand Down Expand Up @@ -597,3 +600,103 @@ func TestMoveBalanceWithSignalError(t *testing.T) {
require.Nil(t, err)
require.Equal(t, uint64(225_500), initialTx.GetFeeInfo().GetGasUsed())
}

func TestPutFeeAndGasUsedRelayedTxV3WithRefunds(t *testing.T) {
t.Parallel()

txHash := []byte("af1581562830e36b0bfb12c618a4ee92d6b7f2e0aa84935432a44c9b63cc8daa")
scrHash1 := []byte("4c58801e77c57e88294f21018145662e2fb1698fd5f1a1cf7b6f81f073f5cd6c")
scrWithRefundHash := []byte("94e678f400192eeae3c84b3125c9d45301db619a3ecbf9e7f46266a81a85ef51")
refundValueBig, _ := big.NewInt(0).SetString("299005000000000", 10)
initialTx := &outportcore.TxInfo{
Transaction: &transaction.Transaction{
Nonce: 0,
SndAddr: []byte("erd1tp66n2lkhs2fm7elvh9lmzfajpg480v55sd8lf2lvu4fw92zsrasvn2wze"),
RcvAddr: []byte("erd1tp66n2lkhs2fm7elvh9lmzfajpg480v55sd8lf2lvu4fw92zsrasvn2wze"),
GasLimit: 99000000,
GasPrice: 1000000000,
Value: big.NewInt(0),
InnerTransactions: []*transaction.Transaction{
{
Nonce: 0,
SndAddr: []byte("erd1s89rm6mv6xyct38r3vqadj74rmqunamhwyz7c84a6u9thedj2wus5nlchg"),
RcvAddr: []byte("erd1qqqqqqqqqqqqqqqpqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqylllslmq6y6"),
RelayerAddr: []byte("erd1tp66n2lkhs2fm7elvh9lmzfajpg480v55sd8lf2lvu4fw92zsrasvn2wze"),
GasLimit: 85000000,
GasPrice: 1000000000,
Data: []byte("createNewDelegationContract@00@00"),
Value: big.NewInt(0),
},
},
},
FeeInfo: &outportcore.FeeInfo{Fee: big.NewInt(0)},
}

scr1 := &smartContractResult.SmartContractResult{
Nonce: 0,
GasPrice: 1000000000,
GasLimit: 84900500,
SndAddr: []byte("erd1s89rm6mv6xyct38r3vqadj74rmqunamhwyz7c84a6u9thedj2wus5nlchg"),
RcvAddr: []byte("erd1qqqqqqqqqqqqqqqpqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqylllslmq6y6"),
Data: []byte("createNewDelegationContract@00@00"),
PrevTxHash: txHash,
OriginalTxHash: txHash,
}
scrWithRefund := &smartContractResult.SmartContractResult{
Nonce: 1,
GasPrice: 1000000000,
GasLimit: 0,
Value: refundValueBig,
SndAddr: []byte("erd1qqqqqqqqqqqqqqqpqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqylllslmq6y6"),
RcvAddr: []byte("erd1tp66n2lkhs2fm7elvh9lmzfajpg480v55sd8lf2lvu4fw92zsrasvn2wze"),
PrevTxHash: scrHash1,
OriginalTxHash: txHash,
ReturnMessage: []byte("gas refund for relayer"),
}

pool := &outportcore.TransactionPool{
Transactions: map[string]*outportcore.TxInfo{
hex.EncodeToString(txHash): initialTx,
},
SmartContractResults: map[string]*outportcore.SCRInfo{
hex.EncodeToString(scrHash1): {
SmartContractResult: scr1,
FeeInfo: &outportcore.FeeInfo{
Fee: big.NewInt(0),
},
},
hex.EncodeToString(scrWithRefundHash): {
SmartContractResult: scrWithRefund,
FeeInfo: &outportcore.FeeInfo{
Fee: big.NewInt(0),
},
},
},
}

enableEpochsHandler := &enableEpochsHandlerMock.EnableEpochsHandlerStub{
IsFlagEnabledInEpochCalled: func(flag core.EnableEpochFlag, epoch uint32) bool {
return flag == common.GasPriceModifierFlag ||
flag == common.RelayedTransactionsV3Flag ||
flag == common.FixRelayedBaseCostFlag
},
}
arg := prepareMockArg()
arg.EnableEpochsHandler = enableEpochsHandler
economicsConfig := testscommon.GetEconomicsConfig()
arg.TxFeeCalculator, _ = economics.NewEconomicsData(economics.ArgsNewEconomicsData{
TxVersionChecker: &testscommon.TxVersionCheckerStub{},
Economics: &economicsConfig,
EpochNotifier: &epochNotifier.EpochNotifierStub{},
EnableEpochsHandler: enableEpochsHandler,
})
txsFeeProc, err := NewTransactionsFeeProcessor(arg)
require.NotNil(t, txsFeeProc)
require.Nil(t, err)

err = txsFeeProc.PutFeeAndGasUsed(pool, 0)
require.Nil(t, err)
require.Equal(t, big.NewInt(699500000000000), initialTx.GetFeeInfo().GetFee())
require.Equal(t, uint64(55149500), initialTx.GetFeeInfo().GetGasUsed())
require.Equal(t, "998505000000000", initialTx.GetFeeInfo().GetInitialPaidFee().String())
}
25 changes: 23 additions & 2 deletions process/economics/economicsData.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,8 +591,15 @@ func (ed *economicsData) ComputeGasUsedAndFeeBasedOnRefundValueInEpoch(tx data.T
txFee := ed.ComputeTxFeeInEpoch(tx, epoch)

if len(tx.GetUserTransactions()) > 0 {
gasUnitsUsed := big.NewInt(0).Div(txFee, big.NewInt(0).SetUint64(tx.GetGasPrice()))
return gasUnitsUsed.Uint64(), txFee
txFeeAfterRefund := txFee.Sub(txFee, refundValue)

gasPriceForProcessing := ed.GasPriceForProcessingInEpoch(tx, epoch)
gasUnitsRefunded := refundValue.Uint64() / gasPriceForProcessing

gasUnitsConsideredForInitialFee := ed.computeRelayedTxV3MinGasLimit(tx)
gasUnitsUsed := gasUnitsConsideredForInitialFee - gasUnitsRefunded

return gasUnitsUsed, txFeeAfterRefund
}

isPenalizedTooMuchGasFlagEnabled := ed.enableEpochsHandler.IsFlagEnabledInEpoch(common.PenalizedTooMuchGasFlag, epoch)
Expand Down Expand Up @@ -682,6 +689,20 @@ func (ed *economicsData) ComputeGasLimitBasedOnBalanceInEpoch(tx data.Transactio
return totalGasLimit, nil
}

func (ed *economicsData) computeRelayedTxV3MinGasLimit(tx data.TransactionWithFeeHandler) uint64 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Non-trivial function, should also be unit-tested (separately from "ComputeGasUsedAndFeeBasedOnRefundValueInEpoch").

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right, will add tests in a separate PR

relayedTxGasLimit := ed.ComputeGasLimit(tx)
relayedTxMinGasLimit := ed.MinGasLimit()
relayedTxGasLimitDiff := relayedTxGasLimit - relayedTxMinGasLimit // this may be positive if the relayed tx is guarded
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the case mentioned in comment tested using "TestEconomicsData_ComputeGasUsedAndFeeBasedOnRefundValueRelayedV3"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

indeed not tested here, covered by testing scenarios 100%

will add tests in a separate PR


innerTxs := tx.GetUserTransactions()
totalGasLimit := relayedTxGasLimitDiff + relayedTxMinGasLimit*uint64(len(innerTxs))
for _, innerTx := range innerTxs {
totalGasLimit += innerTx.GetGasLimit()
}

return totalGasLimit
}

// IsInterfaceNil returns true if there is no value under the interface
func (ed *economicsData) IsInterfaceNil() bool {
return ed == nil
Expand Down
33 changes: 33 additions & 0 deletions process/economics/economicsData_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1281,6 +1281,39 @@ func TestEconomicsData_ComputeGasUsedAndFeeBasedOnRefundValueSpecialBuiltInTooMu
require.Equal(t, expectedFee, fee)
}

func TestEconomicsData_ComputeGasUsedAndFeeBasedOnRefundValueRelayedV3(t *testing.T) {
t.Parallel()

economicData, _ := economics.NewEconomicsData(createArgsForEconomicsDataRealFees())
tx := &transaction.Transaction{
GasPrice: 1000000000,
GasLimit: 99000000,
InnerTransactions: []*transaction.Transaction{
{
GasPrice: 1000000000,
GasLimit: 85000000,
Data: []byte("createNewDelegationContract@00@00"),
},
{
GasPrice: 1000000000,
GasLimit: 50000,
},
{
GasPrice: 1000000000,
GasLimit: 50000,
},
},
}

expectedGasUsed := uint64(55349500)
expectedFee, _ := big.NewInt(0).SetString("899500000000000", 10)

refundValue, _ := big.NewInt(0).SetString("299005000000000", 10)
gasUsed, fee := economicData.ComputeGasUsedAndFeeBasedOnRefundValue(tx, refundValue)
require.Equal(t, expectedGasUsed, gasUsed)
require.Equal(t, expectedFee, fee)
}

func TestEconomicsData_ComputeGasLimitBasedOnBalance(t *testing.T) {
t.Parallel()

Expand Down
Loading