Handle idempotent retry after error during initializing phase #1406

peterbroadhurst · 2023-09-24T22:11:48Z

Currently if an error occurs from a blockchain connector (EVMConnect for example) on submission of a transaction, then the operation goes immediately into Failed status - even if an idempotencyKey was supplied.

This means that a subsequent attempt to re-submit the transaction fails with:
FF10431: Idempotency key '...' already used for transaction '...'

This is not correct per the architecture described in https://hyperledger.github.io/firefly/head/reference/idempotency.html

Because the error happened during the submission of the idempotent transaction identifier into its FireFly Transaction Manager (FFTM) database, it is safe for a future retry to attempt to insert it again.

At that later point, the connector can return a 409 if it's already received that transaction, or it can successfully accept it into it's DB and process it.

This PR makes the following changes to move us towards the architecture defined behavior in this scenario:

Where RunOperation used to return complete as a bool, a new OpPhase has been introduced:
- OpPhaseComplete - good or bad, the operation is complete
- OpPhasePending - the operation has been accepted, and it's completion will happen asynchronously
- OpPhaseInitializing - the operation has not transitioned out of initializing yet
Wherever an operation is executed, a new idempotentSubmit parameter is added
- Set to true for all transactions where idempotencyKey != ""
- When this is true a transaction will stay in Initializing (not move to Failed) if:
  - An error occurs
  - The phase returned from RunOperation is OpPhaseInitializing
- Must be supplied externally to operation manager, as it does not have access to the TX object

These changes thus ensure that the Initializing status is held on to longer than is currently true.

Before: Initializing was only the status until the first attempt to submit the transaction was initiated
After: Initializing remains the status until the connector receives the idempotent submission of the TX

…pPhase enum Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

codecov-commenter · 2023-09-24T22:17:20Z

Codecov Report

Merging #1406 (3f2a21f) into main (a2ebfa7) will increase coverage by 0.00%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 3f2a21f differs from pull request most recent head 9e32841. Consider uploading reports for the commit 9e32841 to get more accurate results

@@           Coverage Diff           @@
##             main    #1406   +/-   ##
=======================================
  Coverage   99.98%   99.99%           
=======================================
  Files         321      321           
  Lines       22960    23026   +66     
=======================================
+ Hits        22956    23024   +68     
+ Misses          2        1    -1     
+ Partials        2        1    -1

Files	Coverage Δ
internal/assets/manager.go	`100.00% <ø> (ø)`
internal/assets/operations.go	`100.00% <100.00%> (ø)`
internal/assets/token_approval.go	`100.00% <100.00%> (ø)`
internal/assets/token_pool.go	`100.00% <100.00%> (ø)`
internal/assets/token_transfer.go	`100.00% <100.00%> (ø)`
internal/broadcast/manager.go	`100.00% <100.00%> (ø)`
internal/broadcast/operations.go	`100.00% <100.00%> (ø)`
internal/contracts/manager.go	`100.00% <100.00%> (ø)`
internal/contracts/operations.go	`100.00% <100.00%> (ø)`
internal/events/batch_pin_complete.go	`100.00% <100.00%> (ø)`
... and 12 more

... and 1 file with indirect coverage changes

peterbroadhurst · 2023-09-26T02:06:20Z

Validated with raw blockchain transactions:

Started core
Stopped EVMConnect
Submitted transaction with empty idempotencyKey
- Resulting operation status was Failed
Submitted transaction with idempotencyKey set to a value
- Resulting operation status was Initialized
Restarted EVMConnect and waited for FF to reconnect its WS
Resubmitted transaction with imdempotencyKey set to failed failu
- Resulting operation status was Succeeded
Confirmed FF10431 received if I tried to re-use idempotency key

…p-phase

awrichar · 2023-09-26T18:51:57Z

internal/assets/token_pool.go

-	_, err = am.operations.RunOperation(ctx, opActivatePool(op, pool))
+	_, err = am.operations.RunOperation(ctx, opActivatePool(op, pool),
+		false, // TODO: this operation should be made idempotent, but cannot inherit this from the TX per our normal semantics
+		//              as the transaction is only on the submitting side and this is triggered on all parties.


Is this saying we insert the operation but not the transaction on the non-submitter members? That seems odd/incorrect.

FWIW activation is an idempotent operation at the connector level (so shouldn't be problematic if we activate multiple times) - not sure if that changes how we want to handle this path.

That was my reading of the code - would appreciate if you can confirm for me.

It was based on reading this:

firefly/internal/definitions/handler_tokenpool.go

Lines 99 to 107 in 7067eb5

// Message will remain unconfirmed, but plugin will be notified to activate the pool

// This will ultimately trigger a pool creation event and a rewind

state.AddPreFinalize(func(ctx context.Context) error {

if err := dh.assets.ActivateTokenPool(ctx, pool); err != nil {

log.L(ctx).Errorf("Failed to activate token pool '%s': %s", pool.ID, err)

return err

}

return nil

})

Looks like we do eventually persist the token pool TX, after the activation completes:

firefly/internal/events/token_pool_created.go

Line 149 in 7067eb5

return em.confirmPool(ctx, existingPool, pool.Event)

So in the happy path, you do end up with a TX on all members, and the activation operations end up showing underneath it.

But it feels like we should probably persist the TX earlier, before we start creating operations.

Is that something you'd like to raise a separate issue for @awrichar ?

…r creating the TX Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

peterbroadhurst · 2023-09-27T05:45:15Z

I extended the fix after testing, as I found in the tokens scenarios if I attempt to recreate the error by having the resolve endpoint down for the signing address, we wrote zero operations to the database.

Then when we came back round ResubmitOperations said it resubmitted zero - correctly - but that's not the information we needed and we returned an idempotency error.

We actually needed to know _resubmitted zero, because no operations exist (vs. all operations reached Pending which was the previous assumption).

So I added a third total return parameter to ResubmitOperations, and in the case it is zero we now re-do the transactions as if we'd generated a new TX (but re-using the TX ID)

peterbroadhurst · 2023-09-27T21:00:35Z

Completed E2E testing (noting it's important to also have hyperledger/firefly-tokens-erc20-erc721#137 applied).

awrichar · 2023-09-28T14:52:44Z

internal/assets/operations.go

 		}
 		switch data.Transfer.Type {
 		case core.TokenTransferTypeMint:
-			return nil, false, plugin.MintTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods)
+			return nil, core.OpPhaseInitializing, plugin.MintTokens(ctx, op.NamespacedIDString(), data.Pool.Locator, data.Transfer, data.Pool.Methods)


Why are these transfer operations all considered "initializing"? If we get a successful return from the plugin, doesn't that mean the transaction has made it all the way through the token connector to the blockchain connector?

It's the phase we're in as a result of the error in this layer of the code, rather than the operation state.

e.g. this error occurred before we submitted the operation to the connector, so the calling code can decide if that means we don't store the operation at all, or we store it in state Initializing

I have not analyzed very scenario to know if the error path is such, that it could never succeed.
If I had done that (quite large) piece of work everywhere, I could have introduced a phase of OpPhaseRejected to mean - "this is never going to work, tell them it's broken... without even storing the operation at all".

OK - so the phase is more meaningful in an error return case in order to determine how to respond to the error (not so much in a success case)?

I noticed in some managers you used operations.ErrTernary, but not here.

Sorry - yes, this should be operations.ErrTernary almost certainly - I'd thought this was an error return.

ok - added a commit for that

internal/assets/token_pool.go

awrichar · 2023-09-28T15:44:59Z

internal/assets/token_approval.go

-		approval: approval,
+		mgr:              am,
+		approval:         approval,
+		idempotentSubmit: approval.IdempotencyKey != "",


Not sure we really need to store this separate bool here, as we can always compute it from the approval above at the point we need it (but I'm fine either way)

I would have to do more investigation to convince myself of that.. and if I did so, I'd probably want to rename approval to approvalInput (rather than the implying it's the actual approval object).

So... if it's ok, think I'd like to leave it as is for this PR

But yes, the thing we store is the input DTO, so possible it's named incorrectly.

awrichar · 2023-09-28T16:09:20Z

internal/assets/token_approval.go

 			if resubmitErr != nil {
 				// Error doing resubmit, return the new error
 				return false, resubmitErr
 			}
-			if len(resubmitted) > 0 {
+			if total == 0 {


This logic feels slightly confusing to me. I guess what we're trying to differentiate is these 3 cases:

no operations have ever been started for this transaction

some operations were previously initialized for this transaction, but we think they weren't fully submitted, so we've now retried them

some operations were previously initialized for this transaction, but we think they're all currently in flight, so we can't do anything but continue waiting

We're expressing these 3 states with a combination of two integer returns... wonder if there's a cleaner way to express it?

Yes - agreed.

I think pushing all this into a combined operation+transaction manager as a function that has plug points for the differentiation, rather than the whole of this section being boilerplate copied in lots of places would be good.

Choice is whether this little bit of extension to the boilerplate is the point to take on the bigger refactor.

I could merge the SubmitNewTransaction and ResubmitOperations functions to a new function, contains this logic in an opinionated way and take in the idempotencyKey out of the input-request, and return:

It's all done now - please return without error to the user

Nothing was done, here's the existing transaction ID

We've never heard of this idempotency key, here's a shiny new TX

This is already submitted completely, 409 to your user

Something else went wrong

Right, point taken - it may not be worth continuing to iterate on this interface until we have the cycles to do a larger refactor. We should capture that next step as a task though.

Raised #1410

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

awrichar

Looks good. I think all remaining feedback has been captured in new issues.

Handle idempotent retry after error during init phase - introducing O…

c1afd24

…pPhase enum Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

peterbroadhurst added 2 commits September 26, 2023 09:20

Merge branch 'fix-migrations' of github.com:kaleido-io/firefly into o…

0b3cf58

…p-phase

Merge branch 'main' of github.com:hyperledger/firefly into op-phase

cbb093e

awrichar reviewed Sep 26, 2023

View reviewed changes

Handle the case where we fail before writing the operations, but afte…

3f2a21f

…r creating the TX Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

peterbroadhurst marked this pull request as ready for review September 27, 2023 21:00

peterbroadhurst requested a review from a team as a code owner September 27, 2023 21:00

peterbroadhurst requested a review from awrichar September 27, 2023 21:00

awrichar reviewed Sep 28, 2023

View reviewed changes

internal/assets/token_pool.go Show resolved Hide resolved

awrichar reviewed Sep 28, 2023

View reviewed changes

peterbroadhurst mentioned this pull request Sep 28, 2023

Refactor Idempotency logic to reduce boilerplate and increase code readability #1410

Open

3 tasks

Only initializing if there's an error

9e32841

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

awrichar approved these changes Sep 28, 2023

View reviewed changes

peterbroadhurst merged commit b3443ec into main Sep 28, 2023
14 checks passed

peterbroadhurst deleted the op-phase branch September 28, 2023 19:54

peterbroadhurst mentioned this pull request Sep 28, 2023

Enterprise fork update from HL - Sep 28 2023 kaleido-io/firefly#89

Merged

peterbroadhurst mentioned this pull request Dec 20, 2023

FF23021: EVM reverted errors during invoke with idempotencyKey leaves ops in Initializing status #1435

Closed

awrichar mentioned this pull request Nov 1, 2024

break out of infinite retry for submitted ops #1595

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle idempotent retry after error during initializing phase #1406

Handle idempotent retry after error during initializing phase #1406

peterbroadhurst commented Sep 24, 2023 •

edited

Loading

codecov-commenter commented Sep 24, 2023 •

edited

Loading

peterbroadhurst commented Sep 26, 2023

awrichar Sep 26, 2023

peterbroadhurst Sep 27, 2023

awrichar Sep 27, 2023

peterbroadhurst Sep 28, 2023

awrichar Sep 28, 2023

peterbroadhurst commented Sep 27, 2023

peterbroadhurst commented Sep 27, 2023

awrichar Sep 28, 2023

peterbroadhurst Sep 28, 2023

peterbroadhurst Sep 28, 2023 •

edited

Loading

awrichar Sep 28, 2023

awrichar Sep 28, 2023

peterbroadhurst Sep 28, 2023

peterbroadhurst Sep 28, 2023

awrichar Sep 28, 2023

peterbroadhurst Sep 28, 2023

peterbroadhurst Sep 28, 2023

awrichar Sep 28, 2023 •

edited

Loading

awrichar Sep 28, 2023

peterbroadhurst Sep 28, 2023 •

edited

Loading

awrichar Sep 28, 2023

peterbroadhurst Sep 28, 2023

awrichar left a comment

	// Message will remain unconfirmed, but plugin will be notified to activate the pool
	// This will ultimately trigger a pool creation event and a rewind
	state.AddPreFinalize(func(ctx context.Context) error {
	if err := dh.assets.ActivateTokenPool(ctx, pool); err != nil {
	log.L(ctx).Errorf("Failed to activate token pool '%s': %s", pool.ID, err)
	return err
	}
	return nil
	})

Handle idempotent retry after error during initializing phase #1406

Handle idempotent retry after error during initializing phase #1406

Conversation

peterbroadhurst commented Sep 24, 2023 • edited Loading

codecov-commenter commented Sep 24, 2023 • edited Loading

Codecov Report

peterbroadhurst commented Sep 26, 2023

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

peterbroadhurst commented Sep 27, 2023

peterbroadhurst commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbroadhurst Sep 28, 2023 • edited Loading

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

awrichar Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbroadhurst Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awrichar left a comment

Choose a reason for hiding this comment

peterbroadhurst commented Sep 24, 2023 •

edited

Loading

codecov-commenter commented Sep 24, 2023 •

edited

Loading

peterbroadhurst Sep 28, 2023 •

edited

Loading

awrichar Sep 28, 2023 •

edited

Loading

peterbroadhurst Sep 28, 2023 •

edited

Loading