Re-introduce the extrinsic inclusion&success check properly.

[clangenb]

1455 has removed the pending xt inclusion check because the implementation did not work. The complete removal of the that feature was motivation because it should never have been in the light-client in the first place. We have to re-introduce this check elsewhere because there needs to be an additional check that the corresponding extrinsic was successful.

[brenzi]

Attempting a design for this. please review broad concept @clangenb:

Questions:

• shall we maintain a call queue or an extrinsic queue for everything the enclave wants to send?
  • a call queue would allow resubmission if nonces clash or mortality trouble. this means ValidatorAccessor would need access to ExtrinsicFactory
  • we may want to track extrinsic hashes and never use immortal, this way every attempt to send a call would have a unique extrinsic hash (right?). that may be difficult for call queue
• does the queue need to be persisted and shared?
  • only one validateer will attempt to send the extrinsics triggered by its block authoring.
  • what if the validateer restarts?
  • what if the validateer goes offline forever?
  • we can't give execution guarantees anyway, so we can probably leave it to the STF to handle timeouts and let the queue be volatile
• how should we allow generic callbacks for each call?
  • by default, extrinsic failure which prohibited block inclusion should trigger a retry (maybe the account had not enough funds but the error was observed and the enclave balance increased, so retry is meaningful
  • extrinsic failure after block inclusion (execution failure) can't have a default treatment for generic calls. However, a few common calls could be handled (like RA, block confirmations aso.)
  • the best would be a callback with (encoded call, block inclusion hash & number, success || error code, fees paid) (see #1250 too)

Attempting a solution sketch:

1. implement a call queue in ValidatorAccessor
   • why here? the enclave calls validator_accessor.send_extrinsic() for the destination parentchain accessor, so it's intuitively the point before calling the ocall to maintain a queue
   • calling send_extrinsic(call)
   • maybe we can just rename ImportQueue to Queue and use it for CallQueue with minimal modifications?
2. clear elements in the queue based on extrinsic success/failed events and execute callbacks
   • refactor indirect_calls_executor listen not only to invoke events, but also to all events with the enclave account involved
   • check if any calls in the pending queue have been confirmed
   • execute callback and supply extrinsic result
   • pop from queue
3. Callbacks would be hard-coded by call type, no dynamic closures possible
   • match call indices tuple and treat extrinsic results
     • track fees based on call index and arguments: #1250
   • the callback knows the original call arguments and is capable of resubmitting a freshly signed extrinsic with fresh nonce if desired
   • based on the call arguments, actions could be triggered which had to wait for extrinsic succes, like burning or unlocking unshielded funds for #1257
   • we may need this to be stateful, to allow "retry 3 times", so we'd need to count retries

Examples:

• the most security-critical extrinsic here may be unshield (#1257). If we burn funds although the vault transfer on L1 has failed, that's a disaster
• default-retry-forever of any extrinsic if InabilityToPayFees

[clangenb]

Before I read the solutions, I have thought about your Questions:

CallQueue?

• Yes, for resubmissions, whatever resubmits it, needs to know the extrinsic factory.
• Unique hash only guaranteed if mortality changes, which only changes if a new parentchain block has been produced meanwhile, which is not always the case for us. (Random mortality? probably bad idea...)

Shard & Persisted:

• This is a bit difficult, in general, we can most likely conclude: If we have to share it, we also have to persist it, so hopefully not necessary.
• Maybe best to think about it with a concrete, security relevant example: Unshielding Funds. Let's assume that the queue is not shared and not persisted. What happens it the validateer that sent the parentchain extrinsic went offline and the extrinsic was successful? It must absolutely not go unnoticed, massive attack vector. The enclave would unshield without burning L2 funds. Hmmm, I think we need a shared and persisted queue.

Callbacks?

• I think the current code is friendly to implementing callbacks for this, and I should just be hardcoded for every possible parentchain extrinsic, which is ok IMO because TrustedCalls leading to that parentchain extrinsic are also hardcoded. Might abstract this over a trait though. I don't really know yet.

I will now look at the proposed solution.

[clangenb]

It is a bit hard to judge the proposal without looking at the code. I think having the call queue somewhere in the light validation code makes sense, but I think the callback queue should be in the STF as this is call specific handling, and it makes sense to do this here too. The other things that I did not comment on, I think are fine as they are on first sight. While thinking about this, two other points popped up in my head though:

• I think we already agree that the timeout case definitely needs to be handled in case an extrinsic gets lost somewhere on the way. Hence, we have to move away from immortality so that we can be absolutely sure that an extrinsic will never make it into a block before we revert state in layer 2.

• Lastly, I am a bit unsure if listening for events should be avoided and we instead only look for successful calls of certain extrinsics, and maybe don't really use events in any other way, I fear that we run into the issue that we accidentally act twice on the same extrinsic if we go the mixed way. I am not sure how relevant this thought is, will think about it some more.