Relayed v3 #5572

sstanculeanu · 2023-09-08T12:06:48Z

Reasoning behind the pull request

New approach for relayed txs, v3, where the user tx is set as a new field of the parent tx, which would simplify and lower the processing cost to the cost of 2 different txs, as there will be no more extra data field.

Proposed changes

implement the changes needed

Testing procedure

standard system test
relayed v3 specific tests

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?

codecov · 2023-09-08T12:36:03Z

Codecov Report

All modified lines are covered by tests ✅

❗ No coverage uploaded for pull request base (feat/relayedv3@5fe5258). Click here to learn what that means.

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

Additional details and impacted files

@@                Coverage Diff                @@
##             feat/relayedv3    #5572   +/-   ##
=================================================
  Coverage                  ?   80.05%           
=================================================
  Files                     ?      706           
  Lines                     ?    93789           
  Branches                  ?        0           
=================================================
  Hits                      ?    75081           
  Misses                    ?    13369           
  Partials                  ?     5339

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

…s + refactor on relayed tests

…into relayedV3 # Conflicts: # go.mod # go.sum

sasurobert · 2023-09-26T07:51:33Z

integrationTests/multiShard/relayedTx/relayedTxV2_test.go

-	"github.com/stretchr/testify/assert"
-)
-
-func TestRelayedTransactionV2InMultiShardEnvironmentWithSmartContractTX(t *testing.T) {


why do you delete this ? Until v1 and v2 features are not obsolete, like they are not taken out with an activation flag the tests have to stay.

They are somewhat duplicated - but sometimes these obsolete tests catch some issues.

the code is not lost, but moved here, in order to reuse it for different types of relayed

TestRelayedTransactionV2InMultiShardEnvironmentWithSmartContractTX is now TestRelayedTransactionInMultiShardEnvironmentWithSmartContractTX/relayed v2

sasurobert · 2023-09-26T07:53:40Z

node/node.go

@@ -54,7 +54,8 @@ var log = logger.GetOrCreate("node")
 var _ facade.NodeHandler = (*Node)(nil)

 // Option represents a functional configuration parameter that can operate
-//  over the None struct.
+//


indent. delete empty line

sasurobert · 2023-09-26T08:02:18Z

process/coordinator/transactionType.go

+		return false
+	}
+
+	return rtx.GetInnerTransaction() != nil


check.Ifnil

sasurobert · 2023-09-26T08:04:00Z

process/coordinator/transactionType.go

@@ -16,6 +17,10 @@ import (

 var _ process.TxTypeHandler = (*txTypeHandler)(nil)

+type relayedV3TransactionHandler interface {
+	GetInnerTransaction() *transaction.Transaction


can't you add this function to the data.TransactionHandler interface ? and have GetInnerTransaction() data.TransactionHandler as the actual function ? It is better if we do not have concrete structure here.

updated TransactionHandler interface

sasurobert · 2023-09-26T08:07:03Z

process/dataValidators/txValidator.go

@@ -115,6 +121,12 @@ func (txv *txValidator) getSenderUserAccount(
 }

 func (txv *txValidator) checkBalance(interceptedTx process.InterceptedTransactionHandler, account state.UserAccountHandler) error {
+	rTx, ok := interceptedTx.Transaction().(relayedV3TransactionHandler)
+	if ok && len(rTx.GetRelayerAddr()) > 0 {
+		// early return if this is a user tx of relayed v3, no need to check balance


check balance of relayer is a must.

Like the transaction sender is the relayer, if it has InnerTransaction - the inner TX should be for the user. First we execute the balance transfer from the relayer to the user, and execute the inner TX after.

GetRelayerAddr should be a field in the InnerTransaction and in the SmartContractResults.

it won't apply for the relayed transaction

this check is only valid for the inner transaction. Reason: only transaction is castable to this structure, then only an inner transaction would have the relayer address set

the reason why this bypass is needed is because now, at api level, we also create the inner transaction, which would fail in the following scenario:

User A with 0 egld signs a tx to move some ESDT

Relayer B will pay the fee for it

Since A does not have EGLD, the inner tx creation at API level would have failed

Ok. I get that this is for user tx only. so it is the check for inner tx

exactly, but does not work as expected.. on it

completely removed it, not needed

sasurobert · 2023-09-26T08:14:04Z

process/transaction/interceptedTransaction.go

+	if !bytes.Equal(innerTx.SndAddr, tx.RcvAddr) {
+		return process.ErrRelayedTxV3BeneficiaryDoesNotMatchReceiver
+	}
+	if len(innerTx.RelayerAddr) == 0 {


why do you need a relayerAddress in the innerTX ? it is not needed. Duplicate information. You have the relayerAddress from the tx.SndAddr

added it for an extra check, to be the same with (parent tx).SndAddr, as discussed with @AdoAdoAdo

that's good. As the user can choose the relayer.

But add the check

sasurobert · 2023-09-26T08:16:11Z

process/transactionEvaluator/transactionEvaluator.go

@@ -103,7 +103,7 @@ func (ate *apiTransactionEvaluator) ComputeTransactionGasLimit(tx *transaction.T
 	switch txTypeOnSender {
 	case process.SCDeployment, process.SCInvoking, process.BuiltInFunctionCall, process.MoveBalance:
 		return ate.simulateTransactionCost(tx, txTypeOnSender)
-	case process.RelayedTx, process.RelayedTxV2:
+	case process.RelayedTx, process.RelayedTxV2, process.RelayedTxV3:
 		// TODO implement in the next PR


let's add a task for this on Jira - a long forgotten TODO.

sasurobert · 2023-09-26T08:18:22Z

process/transaction/shardProcess.go

+	relayerAcnt, acntDst state.UserAccountHandler,
+) (vmcommon.ReturnCode, error) {
+	if !txProc.enableEpochsHandler.IsRelayedTransactionsV3FlagEnabled() {
+		return vmcommon.UserError, txProc.executingFailedTransaction(tx, relayerAcnt, process.ErrRelayedTxV3Disabled)


this has to be backward compatible. So let's make sure that TX is not accepted in the pool if it is of RelayedV3 and the flag is not active. This check should be done at transaction interceptor level.

The problem is nodes with different software will run at the same time in the first few days of the announcement of the new release.

sasurobert · 2023-09-26T08:19:35Z

process/transaction/shardProcess.go

+	if !bytes.Equal(tx.RcvAddr, userTx.SndAddr) {
+		return vmcommon.UserError, txProc.executingFailedTransaction(tx, relayerAcnt, process.ErrRelayedTxV3BeneficiaryDoesNotMatchReceiver)
+	}
+	if len(userTx.RelayerAddr) == 0 {


I do not see the need for the user to put a relayerAddress in his own TX. This was used internally, in case of relayedV1 and v2. It was not the user who field this value, but the protocol in case of processing.

added on inner txs only, for extra check on relayed v3, as mentioned above

add the full check here as well userTx.RelayerAddr = tx.SenderAddr

api/groups/transactionGroup.go

AdoAdoAdo · 2023-10-05T10:56:44Z

process/transaction/interceptedTransaction_test.go

+		Version:          minTxVersion,
+		InnerTransaction: innerTx,
+	}
+	txi, _ := createInterceptedTxFromPlainTxWithArgParser(tx)


this test is a bit long, could you create separate t.run for the cases tested ?

AdoAdoAdo · 2023-10-05T13:41:37Z

process/transaction/shardProcess_test.go

+		assert.Equal(t, process.ErrFailedTransaction, err)
+		assert.Equal(t, vmcommon.UserError, returnCode)
+	})
+	t.Run("value on relayed tx should error", func(t *testing.T) {


"value on relay ...tx"?
the relayed tx is the user tx

… to be the same with parent tx sender and removed skip for balance check as it is not needed anymore

new flag for relayed v3

18e8d70

sstanculeanu self-assigned this Sep 8, 2023

updaet mx-chain-core-go

96203d7

sstanculeanu added 4 commits September 20, 2023 14:44

added implementation on processing and interceptor + integration test…

a777fac

…s + refactor on relayed tests

Merge branch 'rc/v1.6.0' of https://github.com/multiversx/mx-chain-go …

913b9c3

…into relayedV3 # Conflicts: # go.mod # go.sum

removed unused method

8d4b90a

finished implementation + fixed all tests

124541c

sstanculeanu marked this pull request as ready for review September 25, 2023 06:02

sstanculeanu changed the base branch from rc/v1.6.0 to feat/relayedv3 September 25, 2023 07:10

sasurobert self-requested a review September 25, 2023 07:11

sstanculeanu added 2 commits September 25, 2023 12:24

fixed typo after self review

1608e13

added skip for user tx in tx validator balance check

ad459c0

AdoAdoAdo self-requested a review September 25, 2023 10:58

sasurobert requested changes Sep 26, 2023

View reviewed changes

sstanculeanu added 2 commits September 26, 2023 13:18

fixes after first review

f379f31

fixed failing tests

81603cc

AdoAdoAdo reviewed Oct 5, 2023

View reviewed changes

fixes after review

bd43a15

AdoAdoAdo previously approved these changes Oct 5, 2023

View reviewed changes

further fixes after review.. added check for inner tx relayer address…

075b746

… to be the same with parent tx sender and removed skip for balance check as it is not needed anymore

sstanculeanu dismissed AdoAdoAdo’s stale review via 075b746 October 6, 2023 08:19

added extra check for relayer address on interceptor + fixed tests

ca23f1b

AdoAdoAdo approved these changes Oct 6, 2023

View reviewed changes

sasurobert approved these changes Oct 11, 2023

View reviewed changes

sstanculeanu merged commit 34e8ba4 into feat/relayedv3 Oct 11, 2023
5 checks passed

sstanculeanu deleted the relayedV3 branch October 11, 2023 07:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relayed v3 #5572

Relayed v3 #5572

sstanculeanu commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading

sasurobert Sep 26, 2023

sasurobert Sep 26, 2023

sstanculeanu Sep 26, 2023 •

edited

Loading

sasurobert Sep 26, 2023

sstanculeanu Sep 26, 2023

sasurobert Sep 26, 2023

sasurobert Sep 26, 2023

sstanculeanu Sep 26, 2023

sasurobert Sep 26, 2023

sstanculeanu Sep 26, 2023

sasurobert Oct 6, 2023

sstanculeanu Oct 6, 2023

sstanculeanu Oct 6, 2023

sasurobert Sep 26, 2023

sstanculeanu Sep 26, 2023 •

edited

Loading

sasurobert Oct 6, 2023

sasurobert Oct 6, 2023

sstanculeanu Oct 6, 2023

sasurobert Sep 26, 2023

sstanculeanu Sep 26, 2023

sasurobert Sep 26, 2023

sstanculeanu Sep 26, 2023

sasurobert Sep 26, 2023

sstanculeanu Sep 26, 2023

sasurobert Oct 6, 2023

sstanculeanu Oct 6, 2023

AdoAdoAdo Oct 5, 2023

sstanculeanu Oct 5, 2023

AdoAdoAdo Oct 5, 2023

sstanculeanu Oct 5, 2023

Relayed v3 #5572

Relayed v3 #5572

Conversation

sstanculeanu commented Sep 8, 2023 • edited Loading

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstanculeanu Sep 26, 2023 • 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

sstanculeanu Sep 26, 2023 • 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

sstanculeanu commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading

sstanculeanu Sep 26, 2023 •

edited

Loading

sstanculeanu Sep 26, 2023 •

edited

Loading