Fixed transaction status for relayed v1 & v2 intra-shard transactions #426
Conversation
iulianpascalau
commented
Feb 9, 2024
•
iulianpascalau
commented
- fixed transaction status for relayed v1 & v2 intra-shard transactions
| "version": 1, | ||
| "options": 0 | ||
| } | ||
| } |
| "version": 1, | ||
| "options": 0 | ||
| } | ||
| } |
process/transactionProcessor.go
Outdated
| // sanity checks | ||
| senderAddress, err := tp.pubKeyConverter.Decode(tx.Sender) | ||
| if err != nil { | ||
| return transaction.TxStatusFail |
There was a problem hiding this comment.
If sender is metachain (e.g. reward transactions), works as expected?
On failure to decode, perhaps TxStatusUnknown?
There was a problem hiding this comment.
It seems that for reward transactions, process-status currently works fine:
There was a problem hiding this comment.
whoa, thanks for the input. Will add these examples and will come up with a fix
process/transactionProcessor.go
Outdated
| @@ -484,6 +500,26 @@ func checkIfMoveBalanceNotarized(tx *transaction.ApiTransactionResult) bool { | |||
| return true | |||
| } | |||
|
|
|||
| func (tp *TransactionProcessor) checkIfIntraShardRelayedTransaction( | |||
There was a problem hiding this comment.
This checks relayer shard & user shard, but not shard of innerTx.receiver. If inner tx is cross-shard, but relayed one (wrapper) is intra-shard, a "false positive" success might happen (even if tx is still pending). If so, then the check maybe should be "is tx relayed & completely intra-shard, all actors in a shard".
I am not sure about this, though - it's just a question, not a comment.
There was a problem hiding this comment.
yup, agree, will come up with a fix for this :)
|
|
||
| dataField := string(tx.Data) | ||
| if strings.Index(dataField, relayedTxV1DataMarker) != 0 { | ||
| return false, fmt.Errorf("wrong relayed v1 data marker position") |
There was a problem hiding this comment.
Correct, ProcessingTypeOn* identifies Relayed V1, and the check is for Relayed V1 ☑️
process/transactionProcessor.go
Outdated
| } | ||
|
|
||
| isSameShard := tp.proc.GetShardCoordinator().SameShard(senderAddress, receiverAddress) | ||
| isSameShardOnRelayed := tp.proc.GetShardCoordinator().SameShard(senderAddress, receiverAddress) |
There was a problem hiding this comment.
Can also be written as 2 checks / 2 calls of SameShard(): (senderWhichIsRelayer, innerTx.SndAddr) and (innerTx.SndAddr, innerTx.RcvAddr) - but all right to stay as it is, as well, it's more explicit.
There was a problem hiding this comment.
About relayed V1 that wrap NFT transfers: not-yet-completed transactions of such an exotic kind might reach the "is relayed completely intrashard" check, and can be mistaken as completely intrashard, and thus reach an earlier "done" status - even though they are pending.
Even so, this is an extremely exotic case (no occurrence on mainnet). I think we can skip it for now, what do you think?
There was a problem hiding this comment.
refactored the solution
iulianpascalau
dismissed stale reviews from cristure, andreibancioiu, and sstanculeanu
via
a977822
| @@ -500,7 +499,50 @@ func checkIfMoveBalanceNotarized(tx *transaction.ApiTransactionResult) bool { | |||
| return true | |||
| } | |||
|
|
|||
| func (tp *TransactionProcessor) checkIfCompletelyIntraShardRelayedTransaction(tx *transaction.ApiTransactionResult) (bool, error) { | |||
| func (tp *TransactionProcessor) addMissingLogsOnProcessingExceptions( | |||
iulianpascalau
dismissed stale reviews from cristure and andreibancioiu
via
2dc3755
iulianpascalau
changed the title
