Append log events for all inner transactions failure #6176

sstanculeanu · 2024-05-16T13:46:20Z

Reasoning behind the pull request

only the last failed transaction was seen in the log events of a relayed v3 with multiple failed inner txs, because the original tx has was used

Proposed changes

now logs processor appends new logs to the old ones

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?

…ompletedTxEventIdentifier for completed inner move balance of v3

sasurobert · 2024-05-29T10:00:25Z

process/transaction/shardProcess.go

@@ -662,7 +662,12 @@ func (txProc *txProcessor) processRelayedTxV3(
 	if check.IfNil(relayerAcnt) {
 		return vmcommon.UserError, txProc.executingFailedTransaction(tx, relayerAcnt, process.ErrNilRelayerAccount)
 	}
-	err := txProc.relayedTxV3Processor.CheckRelayedTx(tx)
+	funcName, _, err := txProc.argsParser.ParseCallData(string(tx.Data))


can't you add this check inside the relayedTxV3Process.CheckRelayedTX function?

sasurobert · 2024-05-29T10:05:12Z

process/transaction/shardProcess.go

@@ -970,6 +982,10 @@ func (txProc *txProcessor) processUserTx(
 	switch txType {
 	case process.MoveBalance:
 		err = txProc.processMoveBalance(userTx, acntSnd, acntDst, dstShardTxType, originalTxHash, true)
+		isUserTxOfRelayedV3 := len(originalTx.InnerTransactions) > 0


this if can be deleted and add a complete event log for each such processing.

But actually the completeEventLog should be done only if destination shard is the current shard.

otherwise you may have execution on the destination shard and create another log event.

so add log event is this is intra shard only. otherwise, completeLogEvent is added by scProcessor. plus add an integration test and verify how many completeLogEvents are generated.

updated as suggested, only for intra shard inner tx of type move balance

sasurobert · 2024-05-29T10:10:37Z

process/transactionLog/process.go

+	if oldLogs.Address == nil {
+		oldLogs.Address = newLog.Address
+	}
+	oldLogs.Events = append(oldLogs.Events, newLog.Events...)


this is super inefficient. until now for a hash we added the logEvent only once when all the execution was completed. Please do that here as well.

In the current format this PR is not ok.

pushed

updated the flow a bit so this only happens for failed inner intra shard transactions. The append functionality is still needed, as for failed intra shard inner transactions we don't have scrs, but only events on the big relayed tx(key = relayed tx hash).

but you could accumulate the logs at the transaction processor level and push here once.

pushed.. added a new component able to accumulate all logs during execution, then at the end only one SaveLog is done

also removed AppendLogs functionality

update tx unmarshaller to support relayed v3 as well

…iversx/mx-chain-go into append_log_events # Conflicts: # factory/processing/processComponents.go # go.mod # go.sum # process/errors.go

…iversx/mx-chain-go into append_log_events

…nent that accumulates logs during execution of relayed tx from all failed inner txs

ssd04 · 2024-06-18T13:45:57Z

process/transactionLog/failedTxLogsAccumulator.go

+
+type failedTxLogsAccumulator struct {
+	mut     sync.RWMutex
+	logsMap map[string]*logData


how this map will be used? is it possible to use a lot of memory?

maybe evaluating using LRU mechanism to cleanup old entries

it should not use a lot of memory. This is how it works: during the execution of a relayed tx v3, all inner tx scrs will be pushed here, then at the end of the execution, they all acumulated scrs will be forwarded to txLogsProcessor, in order to write into that component only once. Once they are forwarded, the entry for the current tx will be deleted from the map.

ssd04 · 2024-06-18T13:58:43Z

process/smartContract/processorV2/processV2.go

+	if isRelayed {
+		ignorableError = sc.failedTxLogsAccumulator.SaveLogs(logsTxHash, tx, processIfErrorLogs)
+	} else {
+		ignorableError = sc.txLogsProcessor.SaveLog(logsTxHash, tx, processIfErrorLogs)


before newly added isRelayed check, the relayedTx logs were saved to txLogsProcessor but now they will be saved only to failedTxLogsAccumulator, is it ok to save only to failedTxLogsAccumulator?

failedTxLogsAccumulator only accumulates them during the execution of the tx. At the end, the accumulated logs are forwarded to the txLogsProcessor. Should be no difference for relayedV1 and relayedV2

ssd04 · 2024-06-18T14:04:40Z

process/transaction/interceptedTransaction.go


+	if len(tx.InnerTransactions) > 0 {


should we check here also for relayed v2 not allowed? or is this already checked?

should be imposible to have v1 and v2 on the same tx, as they both are part of the data field. Thus no need for check

ssd04 · 2024-06-18T14:07:17Z

process/transaction/shardProcess.go

+	if err != nil {
+		scrHash = originalTxHash


do we need a log message for this error?

ssd04 · 2024-06-18T14:20:59Z

process/transactionLog/failedTxLogsAccumulator.go

+}
+
+// SaveLogs saves the logs into the internal map
+func (accumulator *failedTxLogsAccumulator) SaveLogs(txHash []byte, tx data.TransactionHandler, logs []*vmcommon.LogEntry) error {


since we are not saving the logs to persistent storage, is it possible that we lose unintended failed tx logs in case of a restart? or the logs will be saved later via tx logs processor if needed?

I don’t think we can lose logs. If something fails during tx execution and we somehow lose this, the tx won’t be seen as executed too.

codecov · 2024-06-18T16:37:53Z

Codecov Report

Attention: Patch coverage is 83.44227% with 76 lines in your changes missing coverage. Please review.

Project coverage is 78.76%. Comparing base (e3f3e38) to head (4fdf47d).
Report is 342 commits behind head on feat/relayedv3.

Files	Patch %	Lines
node/external/transactionAPI/unmarshaller.go	48.64%	37 Missing and 1 partial ⚠️
...ess/smartContract/processProxy/testProcessProxy.go	0.00%	24 Missing ⚠️
process/transaction/shardProcess.go	73.80%	5 Missing and 6 partials ⚠️
process/transaction/interceptedTransaction.go	71.42%	1 Missing and 1 partial ⚠️
process/smartContract/processorV2/processV2.go	97.36%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##           feat/relayedv3    #6176      +/-   ##
==================================================
- Coverage           78.78%   78.76%   -0.03%     
==================================================
  Files                 753      755       +2     
  Lines               98341    99289     +948     
==================================================
+ Hits                77481    78207     +726     
- Misses              15626    15825     +199     
- Partials             5234     5257      +23

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

append log events + new chain simulator integration test

0b00ad1

sstanculeanu added low prio not critical - to be solved with low prio area:processing impact:low interested:protocol type:improvement complexity:medium labels May 16, 2024

sstanculeanu self-assigned this May 16, 2024

sstanculeanu added 4 commits May 17, 2024 09:50

added missing file

9a3d0a2

fix indentation

77a77ca

added extra check for multiple relayed types in the same tx + added C…

c2da37d

…ompletedTxEventIdentifier for completed inner move balance of v3

display inner transactions on response of transaction endpoint

89e9fad

sasurobert requested changes May 29, 2024

View reviewed changes

sstanculeanu added 5 commits May 29, 2024 19:38

fixes after first review

f3a89fc

update tx unmarshaller to support relayed v3 as well

do not add relayed v3 to the bad tx forwarder

e347114

Merge branch 'multiple_inner_transactions' of https://github.com/mult…

d2cf3c5

…iversx/mx-chain-go into append_log_events # Conflicts: # factory/processing/processComponents.go # go.mod # go.sum # process/errors.go

fixes after branch update

6e3ff41

Merge branch 'multiple_inner_transactions' of https://github.com/mult…

19079e6

…iversx/mx-chain-go into append_log_events

Base automatically changed from multiple_inner_transactions to feat/relayedv3 June 13, 2024 14:27

sstanculeanu marked this pull request as ready for review June 13, 2024 14:28

sstanculeanu and others added 3 commits June 13, 2024 17:28

Merge branch 'feat/relayedv3' into append_log_events

4d8c8a4

fixes after review: replaced AppendLog functionality with a new compo…

edaa9a5

…nent that accumulates logs during execution of relayed tx from all failed inner txs

reverted change not needed

b52afea

sasurobert previously approved these changes Jun 17, 2024

View reviewed changes

Merge branch 'feat/relayedv3' into append_log_events

22fb0bb

sstanculeanu dismissed sasurobert’s stale review via 22fb0bb June 18, 2024 12:27

ssd04 self-requested a review June 18, 2024 13:05

ssd04 reviewed Jun 18, 2024

View reviewed changes

ssd04 previously approved these changes Jun 18, 2024

View reviewed changes

fix test after merge

4fdf47d

sstanculeanu dismissed ssd04’s stale review via 4fdf47d June 18, 2024 16:28

sasurobert approved these changes Jun 19, 2024

View reviewed changes

ssd04 approved these changes Jun 19, 2024

View reviewed changes

sstanculeanu merged commit 65251ab into feat/relayedv3 Jun 19, 2024
5 of 6 checks passed

sstanculeanu deleted the append_log_events branch June 19, 2024 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Append log events for all inner transactions failure #6176

Append log events for all inner transactions failure #6176

sstanculeanu commented May 16, 2024

sasurobert May 29, 2024

sasurobert May 29, 2024

sasurobert May 29, 2024

sstanculeanu May 29, 2024

sasurobert May 29, 2024

sstanculeanu May 29, 2024

sasurobert Jun 13, 2024

sstanculeanu Jun 14, 2024

ssd04 Jun 18, 2024

ssd04 Jun 18, 2024

sstanculeanu Jun 18, 2024

ssd04 Jun 18, 2024

sstanculeanu Jun 18, 2024

ssd04 Jun 18, 2024

sstanculeanu Jun 18, 2024

ssd04 Jun 18, 2024

sstanculeanu Jun 18, 2024

ssd04 Jun 18, 2024

sstanculeanu Jun 18, 2024

codecov bot commented Jun 18, 2024

Append log events for all inner transactions failure #6176

Append log events for all inner transactions failure #6176

Conversation

sstanculeanu commented May 16, 2024

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

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

codecov bot commented Jun 18, 2024

Codecov Report