Retry on icon skip transaction error

[trmaphi]

No description provided.

[dhlee-icon]

As we talked in slack, this issue regarding score execution time doesn't happen only when fragmentation sends. it could occur when it is normal sending. why don't you change the source code as it sends new TX after confirming previous TX state is transitioned to EXECUTION. let me know if any side effect is expected

[trmaphi]

Why don't you change the source code as it sends new TX after confirming previous TX state is transitioned to EXECUTION

To add a mechanism to wait previous transaction state from pending into execution, we have to map a shared Interface in any blockchain connect by BTP in relay() loop. I'm not sure other chain we do will support getting transaction state by resultParam

	reSegment := true
	for j, segment := range rm.Segments {
		reSegment = false

		if segment.GetResultParam == nil {
			segment.TransactionResult = nil
			if resultParam, err := b.sender.Relay(segment); err != nil {
				b.log.Panicf("fail to Relay err:%+v", err)
			} else {
				segment.GetResultParam = resultParam
			}

                        // wait for pending transaction state to execution state here
			b.logRelaying("after relay", rm, segment, j)
			b.updateResult(rm, segment)

		}
	}

	if reSegment {
		rm.Segments = make([]*chain.Segment, 0)
	}

[dhlee-icon]

Can we add one more function like checkTxState() to "type Sender interface" .
the function checks chain-specific traffic threshold(ie, tx state should be EXEUCUTION in icon), if the condition doesn't meet, it blocks.
if there is no such threshold, it is not implemented.
and we can change relay() in btp.go as the following

	reSegment := true
	for j, segment := range rm.Segments {
		reSegment = false

		if segment.GetResultParam == nil {
			segment.TransactionResult = nil
			if resultParam, err := b.sender.Relay(segment); err != nil {
				b.log.Panicf("fail to Relay err:%+v", err)
			} else {
				segment.GetResultParam = resultParam
			}

                        // wait for pending transaction state to execution state here
                        b.sender. checkTxState(resultParam) // if there is no check logic, it just let it go.
		        b.logRelaying("after relay", rm, segment, j)
			b.updateResult(rm, segment)

		}
	}

	if reSegment {
		rm.Segments = make([]*chain.Segment, 0)
	}

Since it should not block forever, it can continue after certain time. and after that it can retry skipped one.

feel free to share your thought.

[canhlinh]

@dhlee-icon I think you are misunderstanding what he is doing here.

This code doesn't handle the SKIP_TRANSACTION at the time of sending a transaction. It catches this error when getting the result of the transaction at here

The mapErrorWithTransactionResult function will map error that returned from tx's result. With the corresponding error, we will decide what should we need to do next. In this case, We will retry to send the message again by passing it back to the buffered messages.

Here is where we handle the SKIP_TRANSACTION error. That's how the current code flow is doing. Just set segment.GetResultParam = nil then the message will be re-sent again. Of course, it doesn't mean the message will be re-sent immediately. It needs to wait for its turn.

What do you think?

[canhlinh]

@trmaphi looks good to me. Could you do a manual test on your side?

[canhlinh]

@trmaphi I've reviewed this PR again.

I discovered that this PR does not totally solve the problem. We only dealt with retrying to send a new transaction for Segment, not Fragment.

As a result, if the problem is caused by sending a Fragment, it does not appear to work.

Look at this line of code. We're missing the ability to manage the state of a large number of fragments. The code grp, err = s.signAndSendTransaction(tps[i]) will overwrite the previous transaction.

Let's come up with a new way to retry sending failed fragments.

[trmaphi]

I'd like to explain how we come with this solution to make sure we are on the same page. And we're welcome any suggestions with a clear justifications on pros and cons of a given solution

Sum up:

The error root cause is the execution of the transaction takes a too long time to complete the block. The skipped state of transactions can happen to all states of a transaction, except for the final state like NOTFOUND, SUCCEED, FAILED.

There's no state as EXECUTION, as MoonKyu Song - ICONLOOP pointed out:

• PENDING → EXECUTING → SUCCESS
• PENDING → NOTFOUND
• PENDING → EXECUTING → REVERTED

PENDING or EXECUTING can transit to SKIPPED, but in the last state (SUCCESS/NOTFOUND/REVERTED) the transaction state can't be transited SKIPPED

BMR design relates to this error:

• BMR can only know a transaction is SKIPPED after called Sender.GetResult()
• BMR uses Sender.Relay() to send the message, and in the SKIPPED case this function won't' return an error.
• BMR will use Btp.UpdateSegment() to trigger reSegmentation with Sender.Segment() to produce the same the Segmentation and/or Fragmentation with the same transactions params with different signature + timestamp of signature.

The solution is to retry the same TransactionParams with different signature + timestamp of signature. In BMR resend solution is activated by set segment.GetResultParam = nil.

The term resend meaning that the buffered RelayMessage will go through the Segmentation and/or Fragmentation again. And BMR send it with the same TransactionParams with different signature + timestamp of signature

[trmaphi]

Q & A:

As far as I understand, there is no problem in sending tx in itself, so there is no need to put the overhead of checking the state of previous tx

Yes, we don't need to put the state checking of the previous transaction

However, the current problem is that there is a problem in generating a new signature at the time of resending, so you need to solve it and resend it.

Sender.Relay() call Sender.signAndSendTransaction(), then Sender.signAndSendTransaction() call Sender.Client.SignTransaction(). If we call Sender.Relay() with a Segment after Resegmentation, we will have the new transactions with new signature timestamp.

[trmaphi]

@dhlee-icon Feel free to ask any question relates to this MR.

[trmaphi]

Because the max number of relay chain blocks in a message is 3599 blocks and the justifications with vote message have fixed size. During the test, the maximum number of Fragmentation is 5. So the worst scenario, we only need to resegment and/or refragment, then send 5 transactions

@canhlinh we don't need to save the state of fragments locally anymore if only 5 transactions need to be built and sent at the worst case.

[canhlinh]

@trmaphi Your idea is ok. It would be nice if you can provide some tests.