New relayed v3 #6570

sstanculeanu · 2024-10-29T14:04:57Z

Reasoning behind the pull request

new implementation for relayed transactions v3 was needed, in order to simplify the initial implementation

Proposed changes

new approach, with relayer signature on the relayed transaction

Testing procedure

scenarios + system test

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?

…n-go into relayedv3 # Conflicts: # go.mod # go.sum

github-actions · 2024-10-29T14:10:02Z

⚠️ No report was generated due to an error or cancellation of the process.
Please checkout gh action logs for details

github-actions · 2024-10-29T15:05:31Z

⚠️ No report was generated due to an error or cancellation of the process.
Please checkout gh action logs for details

github-actions · 2024-10-30T09:34:05Z

⚠️ No report was generated due to an error or cancellation of the process.
Please checkout gh action logs for details

github-actions · 2024-10-30T12:12:32Z

⚠️ No report was generated due to an error or cancellation of the process.
Please checkout gh action logs for details

sasurobert · 2024-11-04T10:55:57Z

outport/process/transactionsfee/transactionsFeeProcessor.go

-	if !check.IfNil(userTx) && tep.enableEpochsHandler.IsFlagEnabledInEpoch(common.FixRelayedBaseCostFlag, epoch) {
+	tx := txWithResults.GetTxHandler()
+	relayedTx, isRelayedV3 := tx.(data.RelayedTransactionHandler)
+	if isRelayedV3 {


I have seen this code a few times. can't you make a common function for this in a core file and call from everywhere ?

applied the suggestion

sasurobert · 2024-11-04T11:00:35Z

process/coordinator/transactionType.go

@@ -83,6 +83,16 @@ func (tth *txTypeHandler) ComputeTransactionType(tx data.TransactionHandler) (pr
 		return process.InvalidTransaction, process.InvalidTransaction
 	}

+	relayedTxV3, ok := tx.(data.RelayedTransactionHandler)


same as above - we need a core function for all the checks.

sasurobert · 2024-11-04T11:07:27Z

process/dataValidators/txValidator.go

+	tx := interceptedTx.Transaction()
+	relayedTx, ok := tx.(data.RelayedTransactionHandler)
+	if ok {
+		hasValidRelayer := len(relayedTx.GetRelayerAddr()) == len(tx.GetSndAddr()) && len(relayedTx.GetRelayerAddr()) > 0


duplicated code again.

sasurobert · 2024-11-04T11:10:54Z

process/economics/economicsData.go

+		hasValidRelayerSignature := len(txInstance.GetRelayerSignature()) == len(txInstance.GetSignature()) && len(txInstance.GetRelayerSignature()) > 0
+		isRelayedV3 := hasValidRelayer && hasValidRelayerSignature
+		if isRelayedV3 {
+			gasLimit += ed.MinGasLimitInEpoch(epoch)


why not a function like getExtraGasLimitRelayedTx(epoch) and than you can have the implementation there. As is in case of guarded txs

extracted in a new method

sasurobert · 2024-11-04T11:12:11Z

process/economics/economicsData.go

+			gasLimit += ed.getExtraGasLimitGuardedTx(epoch)
+		}
+
+		hasValidRelayer := len(txInstance.GetRelayerAddr()) == len(txInstance.GetSndAddr()) && len(txInstance.GetRelayerAddr()) > 0


duplicated code here.

sasurobert · 2024-11-04T12:09:36Z

process/transaction/baseProcess.go

+	}
+
+	// early exit for relayed v3, the cost will be compared with the sender balance
+	if isRelayedV3 {


Why do you need early exit here? it could go on and get its fee verified on the next lines.

it should be few lines later, indeed

the reason why it is needed is because this method is called from multiple places:

for full relayed tx, before moving the funds from relayer to the sender. In this case, we need the early exit as the feePayer will be the relayer, but the txValue will be taken from the sender. So we don't want to check if the relayer also has the tx.Value.

only for user tx, when the tx fee was already moved into the relayer account. At this point, the sender of the user tx will be verified if it has both the fee(moved from relayer) and the tx.Value (which is here defined as cost)

it is too hard to understand the code and a lot of mistakes can happen because of it.

Can you have separate functions called as it should be ?

sasurobert · 2024-11-04T12:12:28Z

process/transaction/interceptedTransaction.go

@@ -223,6 +232,36 @@ func isRelayedTx(funcName string) bool {
 		core.RelayedTransactionV2 == funcName
 }

+func isRelayedTxV3(tx *transaction.Transaction) bool {


I think it can be considered as relayed TX V3 if one of the 2 new fields is not nil. And if it is not nil and but data is inconsistent than it is a failed tx.

removed the method and reused the common one

the reason why I've added the extended check was to avoid treating txs that only have relayer address or relayer signature as relayed v3.. of course, these txs would have failed later

sasurobert · 2024-11-04T12:31:46Z

process/transaction/shardProcess.go

+	relayedNonce := tx.Nonce
+	relayerAddr := tx.SndAddr
+	if isUserTxOfRelayedV3 && !check.IfNil(relayerAcnt) {
+		relayedNonce = relayerAcnt.GetNonce() - 1 // nonce already increased inside processTxAtRelayer


yeah. but it is increased for the relayer. which is not really good.

In case of relayedV3 increase nonce should happen only at the user - and it has to be exactly as its transaction nonce has.

updated the code to avoid relayer nonce increase, thanks for clarification

sasurobert · 2024-11-04T12:34:32Z

process/transaction/shardProcess.go

+
+	userTx := *tx
+	// remove relayer addr and signature for tx type handler
+	userTx.RelayerAddr = nil


it could work without these 2 lines.

with the current implementation of ProcessTransactionType from txTypeHandler(first check if is relayed v3), if these fields won't be removed, the user tx will still be considered of type relayed v3

at this point, we want to get the proper type of the user tx for its processing

yeah. but this is not ok at all. somebody make a hash for the USER TX - and it gets completely different, links will be lost.

You may create a new function down the line - which works a little bit differently.

I would also separate into new function because we want to get rid of relayedV1 and relayedV2.

pushed the suggestions + improved the comment of mentioned area

…n-go into relayedv3 # Conflicts: # go.mod # go.sum

github-actions · 2024-11-11T11:25:05Z

📊 MultiversX Automated Test Report: View Report

🔄 Build Details:

mx-chain-go Commit Hash: 531aad5fefe9ca88daf115aebc40136b44cba949
Current Branch: relayedv3
mx-chain-go Target Branch: rc/v1.7.next1
mx-chain-simulator-go Target Branch: rc/v1.7.next1
mx-chain-testing-suite Target Branch: rc/v1.7.next1

🚀 Environment Variables:

TIMESTAMP: 11112024-112502
PYTEST_EXIT_CODE: 0
🎉 MultiversX CI/CD Workflow Complete!

sasurobert

also - I think the target should be rc/barnard

sasurobert · 2024-11-13T07:57:03Z

common/common.go

+import "github.com/multiversx/mx-chain-core-go/data"
+
+// IsRelayedTxV3 returns true if the provided transaction is of type relayed v3
+func IsRelayedTxV3(tx data.TransactionHandler) bool {


it returns true only if it is a valid relayed v3 - thus the fields are filled correctly. Maybe a slight rename ?

sasurobert · 2024-11-13T07:58:23Z

integrationTests/multiShard/relayedTx/common.go

+
+	_, err := txDispatcherNode.SendTransaction(relayedTx)
+	if err != nil {
+		fmt.Println(err.Error())


log.error - instead of this.

sasurobert · 2024-11-13T07:58:59Z

outport/process/transactionsfee/transactionsFeeProcessor.go

@@ -271,7 +271,8 @@ func (tep *transactionsFeeProcessor) setGasUsedAndFeeBasedOnRefundValue(
 	refund *big.Int,
 	epoch uint32,
 ) {
-	if !check.IfNil(userTx) && tep.enableEpochsHandler.IsFlagEnabledInEpoch(common.FixRelayedBaseCostFlag, epoch) {
+	isValidUserTxAfterBaseCostActivation := !check.IfNilReflect(userTx) && tep.enableEpochsHandler.IsFlagEnabledInEpoch(common.FixRelayedBaseCostFlag, epoch)


no reflection, change to ifNil.

sasurobert · 2024-11-13T09:09:48Z

process/transaction/baseProcess.go

@@ -166,14 +171,35 @@ func (txProc *baseTxProcessor) checkTxValues(
 		txFee = core.SafeMul(tx.GasLimit, tx.GasPrice)
 	}

+	// early exit for relayed v3, the cost will be compared with the sender balance
+	if isRelayedV3 {
+		return nil


I get the code now. Still a little hard to understand.

sasurobert · 2024-11-13T09:19:25Z

process/transaction/shardProcess.go

+
+	userTx := *tx
+	// remove relayer addr and signature for tx type handler
+	userTx.RelayerAddr = nil


yeah. but this is not ok at all. somebody make a hash for the USER TX - and it gets completely different, links will be lost.

You may create a new function down the line - which works a little bit differently.

I would also separate into new function because we want to get rid of relayedV1 and relayedV2.

github-actions · 2024-11-13T15:57:22Z

📊 MultiversX Automated Test Report: View Report

🔄 Build Details:

mx-chain-go Commit Hash: 6c02c942752af944fe5d97363be7139e05e61a28
Current Branch: relayedv3
mx-chain-go Target Branch: rc/v1.7.next1
mx-chain-simulator-go Target Branch: rc/v1.7.next1
mx-chain-testing-suite Target Branch: rc/v1.7.next1

🚀 Environment Variables:

TIMESTAMP: 13112024-155719
PYTEST_EXIT_CODE: 0
🎉 MultiversX CI/CD Workflow Complete!

sstanculeanu · 2024-11-19T09:53:01Z

Run Tests:
mx-chain-simulator-go: main
mx-chain-testing-suite: main

github-actions · 2024-11-19T10:20:36Z

📊 MultiversX Automated Test Report: View Report

🔄 Build Details:

mx-chain-go Commit Hash: ea799ab74103af94b86408aecb4e938cb05bb6ee
Current Branch: relayedv3
mx-chain-go Target Branch: rc/spica-patch-mempool
mx-chain-simulator-go Target Branch: main
mx-chain-testing-suite Target Branch: main

🚀 Environment Variables:

TIMESTAMP: 19112024-102035
PYTEST_EXIT_CODE: 0
🎉 MultiversX CI/CD Workflow Complete!

sasurobert · 2024-11-25T09:34:54Z

process/transaction/shardProcess.go

@@ -306,7 +308,7 @@ func (txProc *txProcessor) executingFailedTransaction(
 		return err
 	}

-	acntSnd.IncreaseNonce(1)
+	relayerAccount.IncreaseNonce(1)


is this correct ?

With the new model the increase of the nonce has to be Tx.Sender, not tx.RelayerAddress.

indeed this was wrong.. reverted the changes done for this method and created a new one for relayed v3, thus further separating the code

sasurobert · 2024-11-25T10:03:01Z

process/transaction/shardProcess.go

+	userTx *transaction.Transaction,
+) (vmcommon.ReturnCode, error) {
+	computedFees := txProc.computeRelayedTxV3Fees(tx, userTx)
+	txHash, err := txProc.processTxV3AtRelayer(


this is not necessary processing, but taking out fees from relayer. maybe rename the function.

renamed to consumeFeeFromRelayer

sasurobert · 2024-11-25T10:04:19Z

process/transaction/shardProcess.go

+			return nil, err
+		}
+
+		txProc.txFeeHandler.ProcessTransactionFee(relayerFee, big.NewInt(0), txHash)


isn't the fee the Total Fee in this case ?

I think it should stay relayerFee, same as relayed v1 and v2, as the cost of execution is added to txFeeHandler as part of sc processor

sstanculeanu · 2024-11-25T10:49:14Z

Run Tests:
mx-chain-simulator-go: main
mx-chain-testing-suite: main

github-actions · 2024-11-25T11:01:20Z

📊 MultiversX Automated Test Report: View Report

🔄 Build Details:

mx-chain-go Commit Hash: 4c24359ab89654bee5b184d2506d31cd959165bf
Current Branch: master
mx-chain-go Target Branch: ``
mx-chain-simulator-go Target Branch: main
mx-chain-testing-suite Target Branch: main

🚀 Environment Variables:

TIMESTAMP: 25112024-110118
PYTEST_EXIT_CODE: 0
🎉 MultiversX CI/CD Workflow Complete!

new relayed v3

57301ae

sstanculeanu added type:feature New feature or request area:processing impact:low interested:protocol complexity:medium go Pull requests that update Go code labels Oct 29, 2024

sstanculeanu self-assigned this Oct 29, 2024

sstanculeanu added 2 commits October 29, 2024 16:06

Merge branch 'rc/v1.7.next1' of https://github.com/multiversx/mx-chai…

937f10c

…n-go into relayedv3 # Conflicts: # go.mod # go.sum

update deps after merge

905bd78

small fix + fix tests

a36e68c

revert config values modified for local tests

ab06f0b

guarded integration tests

09a2751

cristure previously approved these changes Oct 31, 2024

View reviewed changes

sasurobert self-requested a review November 4, 2024 07:12

sasurobert requested changes Nov 4, 2024

View reviewed changes

fixes after review

68dbf89

sstanculeanu dismissed cristure’s stale review via 68dbf89 November 11, 2024 11:10

sstanculeanu added 2 commits November 11, 2024 13:10

Merge branch 'rc/v1.7.next1' of https://github.com/multiversx/mx-chai…

bb8ee0f

…n-go into relayedv3 # Conflicts: # go.mod # go.sum

deps update

531aad5

sasurobert requested changes Nov 13, 2024

View reviewed changes

further fixes after review

6c02c94

sstanculeanu changed the base branch from rc/v1.7.next1 to rc/spica-patch-mempool November 19, 2024 08:29

Merge branch 'rc/spica-patch-mempool' into relayedv3

232874b

updated indexer

ea799ab

multiversx deleted a comment from github-actions bot Nov 19, 2024

sstanculeanu changed the base branch from rc/spica-patch-mempool to feat/relayedv3 November 21, 2024 19:08

proper name for EnableEpoch epoch

dab1093

sasurobert reviewed Nov 25, 2024

View reviewed changes

fixes after review

95fac83

sasurobert approved these changes Nov 25, 2024

View reviewed changes

cristure approved these changes Nov 25, 2024

View reviewed changes

sstanculeanu merged commit b8487af into feat/relayedv3 Nov 25, 2024
4 checks passed

sstanculeanu deleted the relayedv3 branch November 25, 2024 11:28

New relayed v3 #6570

New relayed v3 #6570

Conversation

sstanculeanu commented Oct 29, 2024

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 30, 2024

github-actions bot commented Oct 30, 2024

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 Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 11, 2024

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

github-actions bot commented Nov 13, 2024

sstanculeanu commented Nov 19, 2024 • edited Loading

github-actions bot commented Nov 19, 2024

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 Nov 25, 2024

github-actions bot commented Nov 25, 2024

sstanculeanu Nov 11, 2024 •

edited

Loading

sstanculeanu commented Nov 19, 2024 •

edited

Loading