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 missing coverage. Please review.

Project coverage is 78.78%. Comparing base (2afa256) to head (e3f3e38).
Report is 219 commits behind head on feat/relayedv3.

❗ Current head e3f3e38 differs from pull request most recent head 06328f8

Please upload reports for the commit 06328f8 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?

now I understand.. it would make sense to also have events for the succeeded inner txs

added in the next pr 🙏

…in-go into multiple_inner_transactions # Conflicts: # errors/errors.go # factory/interface.go # factory/mock/processComponentsStub.go # factory/processing/export_test.go # factory/processing/processComponents.go # factory/processing/processComponentsHandler.go # factory/processing/processComponentsHandler_test.go # go.mod # go.sum # integrationTests/mock/processComponentsStub.go # integrationTests/multiShard/relayedTx/common.go # node/chainSimulator/components/processComponents.go # node/chainSimulator/components/processComponents_test.go

sasurobert · 2024-05-28T11:49:45Z

process/transaction/shardProcess.go

 	if txProc.enableEpochsHandler.IsFlagEnabled(common.FixRelayedMoveBalanceFlag) {
-		totalFee = txProc.economicsFee.ComputeTxFee(userTx)
+		moveBalanceUserFee := txProc.economicsFee.ComputeMoveBalanceFee(userTx)


can't you reuse the common code for this?

some of the above variables are used in further ifs

api/groups/transactionGroup.go

factory/processing/processComponents.go

integrationTests/multiShard/relayedTx/common.go

process/transaction/relayedTxV3Processor.go

process/transaction/shardProcess.go

AdoAdoAdo · 2024-06-07T12:58:29Z

process/transaction/shardProcess.go

 	}

-	return txProc.finishExecutionOfRelayedTx(relayerAcnt, acntDst, tx, userTx)
+	err = txProc.addFeeAndValueToDest(acntSnd, big.NewInt(0), txFee)


maybe as an optimization, we can skip the addition of fee to the destination account if we are anyway going to remove it afterwards.

can be done on a different PR, as we need to check also with v1 & v2, and see if everything is backwards compatible (should be).

AdoAdoAdo · 2024-06-07T12:59:53Z

process/transaction/shardProcess.go

@@ -832,20 +935,14 @@ func (txProc *txProcessor) processUserTx(
 	userTx *transaction.Transaction,
 	relayedTxValue *big.Int,
 	relayedNonce uint64,
-	txHash []byte,
+	originalTxHash []byte,


if implementing the optimization with the fee, then line 950 would simply need to remove the value from user.

AdoAdoAdo · 2024-06-07T13:28:15Z

process/transaction/shardProcess.go

@@ -832,20 +935,14 @@ func (txProc *txProcessor) processUserTx(
 	userTx *transaction.Transaction,
 	relayedTxValue *big.Int,
 	relayedNonce uint64,
-	txHash []byte,
+	originalTxHash []byte,
 ) (vmcommon.ReturnCode, error) {

 	acntSnd, acntDst, err := txProc.getAccounts(userTx.SndAddr, userTx.RcvAddr)


is it possible that during the execution of relayed tx we reach this point and we can get an error here?
if so we need extra cleanup.

handled the situation. should not happen though

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

update-core-go after merge

3da7174

fix simulator tests after merge

9980a4f

sasurobert reviewed May 28, 2024

View reviewed changes

sasurobert previously approved these changes May 29, 2024

View reviewed changes

updated core-go

b810441

sstanculeanu dismissed sasurobert’s stale review via b810441 June 4, 2024 07:56

AdoAdoAdo requested changes Jun 5, 2024

View reviewed changes

fixes after review

78831c6

AdoAdoAdo reviewed Jun 7, 2024

View reviewed changes

todo + fixes after review

06328f8

sasurobert approved these changes Jun 13, 2024

View reviewed changes

AdoAdoAdo approved these changes Jun 13, 2024

View reviewed changes

sstanculeanu merged commit 3b45189 into feat/relayedv3 Jun 13, 2024
4 checks passed

sstanculeanu deleted the multiple_inner_transactions branch June 13, 2024 14:27


		const minTransactionsAllowed = 1

		type ArgRelayedTxV3Processor struct {

Relayed v3 with multiple transactions #6118

Relayed v3 with multiple transactions #6118

Conversation

sstanculeanu commented Apr 15, 2024 • edited Loading

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov bot commented Apr 30, 2024 • edited Loading

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 Loading

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 Loading

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

Choose a reason for hiding this comment

sstanculeanu commented Apr 15, 2024 •

edited

Loading

codecov bot commented Apr 30, 2024 •

edited

Loading

sstanculeanu May 7, 2024 •

edited

Loading

sstanculeanu May 7, 2024 •

edited

Loading