Relayed v3 with multiple transactions #6118

sstanculeanu · 2024-04-15T12:51:17Z

Reasoning behind the pull request

Relayed v3 should be able to support multiple inner transactions

Proposed changes

Added support for multiple inner transactions for relayed v3(senders should be in the same shard with the relayer)
Added proper fix for the relayed move balance fee
Added integration test with chain simulator for relayed v3

Testing procedure

Will be tested with feat branch

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

…actions + fixes on handling fee

…_txs Extra tests and coverage for relayed v3 with multiple inner transactions

codecov · 2024-04-30T14:20:45Z

Codecov Report

Attention: Patch coverage is 80.65934% with 88 lines in your changes are missing coverage. Please review.

Project coverage is 78.78%. Comparing base (d31d68b) to head (e3f3e38).

❗ Current head e3f3e38 differs from pull request most recent head 2bb0754. Consider uploading reports for the commit 2bb0754 to get more accurate results

Files	Patch %	Lines
api/groups/transactionGroup.go	61.40%	43 Missing and 1 partial ⚠️
process/transaction/shardProcess.go	73.18%	27 Missing and 10 partials ⚠️
...ode/chainSimulator/components/processComponents.go	0.00%	3 Missing ⚠️
factory/processing/processComponents.go	85.71%	1 Missing and 1 partial ⚠️
factory/processing/processComponentsHandler.go	75.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##           feat/relayedv3    #6118      +/-   ##
==================================================
+ Coverage           78.77%   78.78%   +0.01%     
==================================================
  Files                 752      753       +1     
  Lines               98137    98341     +204     
==================================================
+ Hits                77308    77481     +173     
- Misses              15604    15626      +22     
- Partials             5225     5234       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sasurobert

review still in works.

sasurobert · 2024-04-30T14:29:09Z

integrationTests/multiShard/relayedTx/common.go

@@ -14,7 +14,7 @@ import (
 )

 // CreateGeneralSetupForRelayTxTest will create the general setup for relayed transactions
-func CreateGeneralSetupForRelayTxTest() ([]*integrationTests.TestProcessorNode, []int, []*integrationTests.TestWalletAccount, *integrationTests.TestWalletAccount) {
+func CreateGeneralSetupForRelayTxTest(relayedV3Test bool) ([]*integrationTests.TestProcessorNode, []int, []*integrationTests.TestWalletAccount, *integrationTests.TestWalletAccount) {


this is not a good practice. Make a separate function for CreateGeneralSetupForRelayedV3. which calls the common code.

This can be done without the bool.

sasurobert · 2024-05-02T06:26:05Z

process/transaction/baseProcess.go

-			txFee = txProc.economicsFee.ComputeTxFee(tx)
+			moveBalanceGasLimit := txProc.economicsFee.ComputeGasLimit(tx)
+			gasToUse := tx.GetGasLimit() - moveBalanceGasLimit
+			moveBalanceUserFee := txProc.economicsFee.ComputeMoveBalanceFee(tx)


this will further increase the cost of relayed txs as they are now.

It can stay, but for relayed v1 and v2 the txData is computed twice. Once in the relayer and once when processing the underlying tx.

sasurobert · 2024-05-02T06:28:31Z

process/transaction/shardProcess.go

@@ -298,7 +304,14 @@ func (txProc *txProcessor) executingFailedTransaction(
 		return nil
 	}

-	txFee := txProc.economicsFee.ComputeTxFee(tx)
+	txFee := txProc.economicsFee.ComputeFeeForProcessing(tx, tx.GasLimit)
+	if txProc.enableEpochsHandler.IsFlagEnabled(common.FixRelayedMoveBalanceFlag) {


move the common code and use a common function. This is the exactly same lines of codes as in baseProcess.

move more code into that new function. activation flags as well.

sasurobert · 2024-05-02T06:28:47Z

process/transaction/shardProcess.go

@@ -391,7 +404,11 @@ func (txProc *txProcessor) processTxFee(
 	if isUserTxOfRelayed {
 		totalCost := txProc.economicsFee.ComputeFeeForProcessing(tx, tx.GasLimit)
 		if txProc.enableEpochsHandler.IsFlagEnabled(common.FixRelayedMoveBalanceFlag) {
-			totalCost = txProc.economicsFee.ComputeTxFee(tx)
+			moveBalanceGasLimit := txProc.economicsFee.ComputeGasLimit(tx)
+			gasToUse := tx.GetGasLimit() - moveBalanceGasLimit


duplicated code again.

sasurobert · 2024-05-02T06:30:05Z

process/transaction/shardProcess.go

 		}

 		txProc.txFeeHandler.ProcessTransactionFee(relayerFee, big.NewInt(0), txHash)
 	}

-	return txHash, nil
+	return nil


txHash was returned in order to not calculate that too many times. Actually we should create a task where we calculate txHash only once in the lifetime of a tx.

created new task MX-15423

sasurobert · 2024-05-02T17:05:16Z

api/groups/transactionGroup.go

+
+				newInnerTx, _, err := tg.createTransaction(innerTx, nil)
+				if err != nil {
+					// if one of the inner txs is invalid, break the loop and move to the next transaction received


why would you send the tx further if the inner tx is wrong? Why not error?

right, updated

sasurobert · 2024-05-02T17:08:44Z

process/transaction/baseProcess.go

@@ -176,6 +176,16 @@ func (txProc *baseTxProcessor) checkTxValues(
 	return nil
 }

+func (txProc *baseTxProcessor) computeTxFeeAfterMoveBalanceFix(tx *transaction.Transaction) *big.Int {
+	moveBalanceGasLimit := txProc.economicsFee.ComputeGasLimit(tx)


I think flag check and old computation can be moved inside this new function as well. It will look better. As in all the cases you have compute old way if flag is not activated.

sasurobert · 2024-05-02T17:15:44Z

process/transaction/shardProcess.go

@@ -298,7 +304,14 @@ func (txProc *txProcessor) executingFailedTransaction(
 		return nil
 	}

-	txFee := txProc.economicsFee.ComputeTxFee(tx)
+	txFee := txProc.economicsFee.ComputeFeeForProcessing(tx, tx.GasLimit)
+	if txProc.enableEpochsHandler.IsFlagEnabled(common.FixRelayedMoveBalanceFlag) {


move more code into that new function. activation flags as well.

sasurobert · 2024-05-02T17:18:14Z

process/transaction/shardProcess.go

+
+	allUserTxsSucceeded := len(executedUserTxs) == len(innerTxs) && innerTxErr == nil && innerTxRetCode == vmcommon.Ok
+	if !allUserTxsSucceeded {
+		log.Debug("failed to execute all inner transactions", "total", len(innerTxs), "executed transactions", len(executedUserTxs))


we do not need information like this in log.Debug - maybe in trace.

sasurobert · 2024-05-02T17:20:02Z

process/transaction/shardProcess.go

 ) (*smartContractResult.SmartContractResult, error) {
+	scrValue := tx.Value
+	if isRevertSCR {
+		scrValue = big.NewInt(0).Neg(tx.Value)


we have never created SCR with negative value. Why do you need this ?

this is not needed.

indeed not needed, I forgot to remove it after the last discussion

sasurobert · 2024-05-02T17:28:40Z

process/transaction/shardProcess.go

+	for acnt, prevBalance := range snapshot {
+		currentBalance := acnt.GetBalance()
+		diff := big.NewInt(0).Sub(currentBalance, prevBalance)
+		err := acnt.SubFromBalance(diff)


this is not good.

user accounts.RevertToSnapshot. it will resolve the issue. No need to manually do this.

sasurobert · 2024-05-02T17:30:24Z

process/transaction/shardProcess.go

+
+	uniqueSendersMap := txProc.relayedTxV3Processor.GetUniqueSendersRequiredFeesMap(tx.InnerTransactions)
+	uniqueSendersSlice := mapToSlice(uniqueSendersMap)
+	sendersBalancesSnapshot := make(map[state.UserAccountHandler]*big.Int, len(uniqueSendersMap))


you do not need to get the accounts balance like this - no need. revert should be done with accounts.RevertToSnapshot function. after taking the fees out from the relayer, you make a snapshot. accounts.Snapshot() -> will give you an ID. and in case of errors you return to that and all is good.

sasurobert · 2024-05-02T17:32:14Z

process/transaction/shardProcess.go

-	if userTx.GasLimit != remainingGasLimit {
-		return vmcommon.UserError, txProc.executingFailedTransaction(tx, relayerAcnt, process.ErrRelayedTxV3GasLimitMismatch)
+
+	if check.IfNil(acntSnd) {


we have discussed that acntSnd has to be in the same shard. If it is not, you should return error. it is a failed inner tx.

At the interceptor level we can simply stop these kind of txs. there is no reason at all to accept them to the network.

added return error as suggested

this check is already done at interceptor level, so this if should never ever be true

that is true. But it is ok to check here as well and return error.

sasurobert · 2024-05-02T17:34:23Z

process/transaction/relayedTxV3Processor.go

+	relayerMoveBalanceFee := proc.economicsFee.ComputeMoveBalanceFee(tx)
+	uniqueSenders := proc.GetUniqueSendersRequiredFeesMap(tx.InnerTransactions)
+
+	relayerFee := big.NewInt(0).Mul(relayerMoveBalanceFee, big.NewInt(int64(len(uniqueSenders))))


I do not think this should be by unique senders. every inner tx is a separate tx with separate move balance fee. independent if the sender is the same.

considered this a cost optimization, although it makes more sense to keep each tx independent including the move balance fee.. updated as suggested

sasurobert · 2024-05-02T17:34:47Z

process/transaction/relayedTxV3Processor.go

+
+const minTransactionsAllowed = 1
+
+type ArgRelayedTxV3Processor struct {


add comment

sasurobert · 2024-05-09T08:48:14Z

process/transaction/shardProcess.go

+	var innerTxErr error
+	executedUserTxs := make([]*transaction.Transaction, 0)
+	for _, innerTx := range innerTxs {
+		innerTxRetCode, innerTxErr = txProc.finishExecutionOfInnerTx(tx, innerTx)


maybe a better name is processInnerTx

sasurobert · 2024-05-09T08:50:22Z

process/transaction/shardProcess.go

 	}

-	return txProc.finishExecutionOfRelayedTx(relayerAcnt, acntDst, tx, userTx)
+	txFee := txProc.computeTxFee(innerTx)
+	err = txProc.addFeeAndValueToDest(acntSnd, tx, txFee)


is tx.value == big.NewInt(0) ? I think it would be better to have a localTX here, where we have value at 0 clearly.

we do have an early check for the tx value to be 0, but the suggestion from the next comment with the specific parameter should clarify.. applied it

sasurobert · 2024-05-09T08:52:00Z

process/transaction/shardProcess.go

 	}

-	return txProc.finishExecutionOfRelayedTx(relayerAcnt, acntDst, tx, userTx)
+	txFee := txProc.computeTxFee(innerTx)
+	err = txProc.addFeeAndValueToDest(acntSnd, tx, txFee)


you can change the addFeeAndValueToDest function and have 2 bigInts to be received there. it is much more clear. and here you could have big.newint(0), txFee.

sasurobert · 2024-05-09T08:55:26Z

process/transaction/shardProcess.go

 	}

-	return txProc.finishExecutionOfRelayedTx(relayerAcnt, acntDst, tx, userTx)
+	txFee := txProc.computeTxFee(innerTx)


we need another check - inveriant check Sum of TXFee == ComputeRelayedFee. you can sum all the txFees you have here, and at the end of processing to check it.

added check as discussed 🙏

sasurobert · 2024-05-09T09:01:04Z

process/transaction/shardProcess.go

+	for _, innerTx := range innerTxs {
+		innerTxRetCode, innerTxErr = txProc.finishExecutionOfInnerTx(tx, innerTx)
+		if innerTxErr != nil || innerTxRetCode != vmcommon.Ok {
+			continue


we should create a logEvent for failed innerTX, for those cases which is not caught by processUserTX - I think when the code reaches processUserTX a logEvent is done at least for that.

that's true, when the code reaches processUserTX it generates a logEvent.. will add one in case the processing fails before that point

sasurobert · 2024-05-09T09:01:35Z

process/transaction/shardProcess.go

+			continue
+		}
+
+		executedUserTxs = append(executedUserTxs, innerTx)


we have to check whether an additional logEvent would be needed here - I think it is better to have that innerTX was executed.

we do have a SCR. do we also a logEvent?

added draft implementation of relayed tx v3 with multiple inner trans…

d920fb8

…actions + fixes on handling fee

sstanculeanu added type:feature New feature or request area:processing impact:low interested:protocol complexity:medium labels Apr 15, 2024

sstanculeanu self-assigned this Apr 15, 2024

sstanculeanu added 2 commits April 15, 2024 16:00

removed commented code

dd31caa

fixes after clarifications and tests

94adf0d

sstanculeanu marked this pull request as ready for review April 30, 2024 11:14

sstanculeanu and others added 2 commits April 30, 2024 17:00

added extra tests and coverage

f2e097d

Merge pull request #6147 from multiversx/unittests_for_multiple_inner…

e3f3e38

…_txs Extra tests and coverage for relayed v3 with multiple inner transactions

sasurobert reviewed May 2, 2024

View reviewed changes

fixes after review part 1 + added limitation on the number of inner txs

404c3d3

sasurobert requested changes May 2, 2024

View reviewed changes

sstanculeanu added 3 commits May 7, 2024 13:58

fixes after review part 2

afbe7c5

fix tests

d23be9c

moved the test with chain simulator in the proper package

6975c19

sasurobert reviewed May 9, 2024

View reviewed changes

sstanculeanu added 4 commits May 13, 2024 15:18

further fixes after review

68a95a1

fix linter

253f920

improved integration test to check events as well

1da2106

added missing config

2bb0754


		const minTransactionsAllowed = 1

		type ArgRelayedTxV3Processor struct {

Relayed v3 with multiple transactions #6118

Are you sure you want to change the base?

Relayed v3 with multiple transactions #6118

Conversation

sstanculeanu commented Apr 15, 2024 • edited

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov bot commented Apr 30, 2024 • edited

Codecov Report

sasurobert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu commented Apr 15, 2024 •

edited

codecov bot commented Apr 30, 2024 •

edited

sstanculeanu May 7, 2024 •

edited

sstanculeanu May 7, 2024 •

edited