Link inner tx to results #6527
Link inner tx to results #6527
Conversation
now get transaction should return results for inner tx hashes
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
process/transaction/shardProcess.go
Outdated
| @@ -789,6 +783,26 @@ func (txProc *txProcessor) processInnerTx( | |||
|
|
|||
| txFee := txProc.computeInnerTxFee(innerTx) | |||
|
|
|||
| innerTxHash := originalTxHash | |||
There was a problem hiding this comment.
can you make a separate function for this ?
If would be easier to understand - otherwise you look at this and you do not seem that it is ok.
Also, can you rename originalTxHash - to relayerTxHash ?
Or it is relayerTxHash always ? If it is not, than it can stay originalTxHash.
There was a problem hiding this comment.
added new method + kept originalTxHash.. in case of relayedV3, it can be either relayerTxHash either innerTxHash
process/transaction/shardProcess.go
Outdated
| log.Debug("txLogsProcessor.SaveLog failed", "error", ignorableErr.Error()) | ||
| } | ||
| func (txProc *txProcessor) createCompletedRelayedTxV3EventLog(tx data.TransactionHandler, originalTxHash []byte) { | ||
| if len(txProc.relayedTxV3InnerHashes) == 0 { |
There was a problem hiding this comment.
can you change this and accumulate the inner hashes into a structure which is not a local variable for the txProc ?
I think it can be done.
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
dblookupext/historyRepository.go
Outdated
| continue | ||
| } | ||
|
|
||
| for _, innerTx := range innerTxs { |
There was a problem hiding this comment.
can you extract this into a method?
hr.recordInnerTx
| } | ||
|
|
||
| for _, innerTx := range txInfoFromMap.tx.GetUserTransactions() { | ||
| innerTxHash, errCalculateHash := core.CalculateHash(bpp.marshalizer, bpp.hasher, innerTx) |
There was a problem hiding this comment.
can you extract this into a method saveInnerTxToStorage
process/transaction/shardProcess.go
Outdated
| innerTxHash := originalTxHash | ||
|
|
||
| var err error | ||
| if txProc.enableEpochsHandler.IsFlagEnabled(common.HashForInnerTransactionFlag) { |
There was a problem hiding this comment.
Is this method called only for v3?
otherwise this will change also the behavior for SCRs for v1 and v2 relay txs, those need to be retested
There was a problem hiding this comment.
yes, processInnerTx is specific to relayed v3
process/transaction/shardProcess.go
Outdated
| innerTxIdx) | ||
| } | ||
|
|
||
| txProc.relayedTxV3InnerHashes = append(txProc.relayedTxV3InnerHashes, innerTxHash) |
There was a problem hiding this comment.
in case of a v1 or v2 tx, then this is not a relayedV3 inner hash
There was a problem hiding this comment.
this method should be called for v3 only
| } | ||
|
|
||
| prefix := miniblockHash[0] | ||
| actualHash := miniblockHash[1:] |
There was a problem hiding this comment.
The actualHash is the relayTxHash, and it appears you are not returning this
It might be needed to create the link to the relayTx that included this inner tx
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
| @@ -538,6 +538,11 @@ func (atp *apiTransactionProcessor) lookupHistoricalTransaction(hash []byte, wit | |||
| return nil, fmt.Errorf("%s: %w", ErrCannotRetrieveTransaction.Error(), err) | |||
| } | |||
|
|
|||
| if len(parentTxHash) > 0 { | |||
| tx.OriginalTransactionHash = hex.EncodeToString(parentTxHash) | |||
There was a problem hiding this comment.
you are doing twice the same encoding
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
sstanculeanu
dismissed stale reviews from sasurobert and AdoAdoAdo
via
a2de898
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
|
|
||
| tu.shardCoordinator.ComputeId(innerTx.RcvAddr) | ||
|
|
||
| frontEndTx := &transaction.ApiTransactionResult{ |
| innerTxHash = make([]byte, 0) | ||
| } | ||
|
|
||
| tu.shardCoordinator.ComputeId(innerTx.RcvAddr) |
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
|
|
||
| # FixRelayedBaseCostEnableEpoch represents the epoch when the fix for relayed base cost will be enabled | ||
| FixRelayedBaseCostEnableEpoch = 7 | ||
| RelayedTransactionsV3EnableEpoch = 4 |
There was a problem hiding this comment.
Seems good, previous three merged into one, then added a new one 👍
|
|
||
| relayedHashPrefix := []byte{byte(RelayedTxHash)} | ||
| relayedTxHashPrefixed := append(relayedHashPrefix, []byte(txHash)...) | ||
| errPut := hr.miniblockHashByTxHashIndex.Put(innerTxHash, relayedTxHashPrefixed) |
There was a problem hiding this comment.
Thus, now we keep heterogeneous data in this storer. In a future PR, we should refactor to adjust its naming.
| @@ -538,6 +544,13 @@ func (atp *apiTransactionProcessor) lookupHistoricalTransaction(hash []byte, wit | |||
| return nil, fmt.Errorf("%s: %w", ErrCannotRetrieveTransaction.Error(), err) | |||
| } | |||
|
|
|||
| if len(parentTxHash) > 0 { | |||
| encodedParentTxHash := hex.EncodeToString(parentTxHash) | |||
| tx.OriginalTransactionHash = encodedParentTxHash | |||
| encodedParentTxHash := hex.EncodeToString(parentTxHash) | ||
| tx.OriginalTransactionHash = encodedParentTxHash | ||
| tx.PreviousTransactionHash = encodedParentTxHash | ||
| tx.Type = string(transaction.TxTypeInner) |
There was a problem hiding this comment.
We should announce this, or should be extra careful on downstream applications. Can be considered a somehow-breaking change.
There was a problem hiding this comment.
We need to double check from downstream applications.
| } | ||
|
|
||
| hashSize := hr.hasher.Size() | ||
| if len(miniblockHash) == hashSize { |
There was a problem hiding this comment.
this adds a previously not needed check between the hasher.Size() and the hashSize.
if we would want to change the hasher, previously it would work without any changes, with the new code if the hash size is different it will not. Just to be aware in the future about this "connection"
| @@ -569,6 +582,7 @@ func (atp *apiTransactionProcessor) lookupHistoricalTransaction(hash []byte, wit | |||
| func putMiniblockFieldsInTransaction(tx *transaction.ApiTransactionResult, miniblockMetadata *dblookupext.MiniblockMetadata) *transaction.ApiTransactionResult { | |||
| tx.Epoch = miniblockMetadata.Epoch | |||
| tx.Round = miniblockMetadata.Round | |||
| putRoundAndEpochOnInnerTxs(tx) | |||
|
|
|||
| tx.MiniBlockType = block.Type(miniblockMetadata.Type).String() | |||
There was a problem hiding this comment.
no need for the next fields to be set in the inner txs?
maybe at least the miniblockHash and miniBlockType.
| isForRelayer := bytes.Equal(dbScResult.RcvAddr, txWithRelayerAddr.GetRelayerAddr()) | ||
| shouldConsiderScrForRelayer := isFlagEnabled && isForRelayer | ||
|
|
||
| return isForRelayed && (isForSender || shouldConsiderScrForRelayer) && differentHash |
There was a problem hiding this comment.
for after the fix, should the differentHash be true or false? If you have 1 SCR from an innerTx are OriginalTxHash and PrevTxHash equal or not?
could this be refactored somehow so that the functionality before and after is more visible?
| } | ||
|
|
||
| if !bytes.Equal(relayedTxHash, originalTxHash) { | ||
| txProc.innerTxsHashesHolder.Append(originalTxHash) |
There was a problem hiding this comment.
the innerTxsHashedHolder appends only after the activation of the fix flag when the hashes are different.
the name getOriginalTxHashForInnerTxOfRelayedV3 might not be the most appropiate, because the originalTxHash and previousTxHash are actually set to the relayedTxHash... just an observation.
| @@ -1293,16 +1308,32 @@ func (txProc *txProcessor) createCompleteEventLog(scr data.TransactionHandler, o | |||
| } | |||
| } | |||
|
|
|||
| func (txProc *txProcessor) saveFailedLogsIfNeeded(originalTxHash []byte) { | |||
There was a problem hiding this comment.
this is removed without any flag, and the createCompletedRelayedTxV3EventLog was added;
these are backwards compatbile, right?
if someone processed the failedLogs, after the upgrade, with not activation flag it won't generate them any more.
| isScrRefundForRelayer := scr.IsRefund && (scr.RcvAddr == tx.RelayerAddress) | ||
| shouldConsiderRefundSentToRelayer := isScrRefundForRelayer && isHashForInnerTxActive | ||
| isScrForTxSender := scr.RcvAddr == tx.Sender | ||
| if !scr.IsRefund || (!isScrForTxSender && !shouldConsiderRefundSentToRelayer) { |
There was a problem hiding this comment.
this is also very hard to understand what happens before and after the activation. Could it be refactored so that the before activation remains the same like before, and after the activation it has another logic?
|
Run Tests: |
|
|
|
Run Tests: |
|
📊 MultiversX Automated Test Report: View Report 🔄 Build Details:
🚀 Environment Variables:
|
|
proper fix in #6529 |
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?