brave-intl / bat-go

Pass "go", collect 200 BAT
Mozilla Public License 2.0
41 stars 31 forks source link

Never Update `PaymentState` `ExternalIdempotency` Data #2504

Closed Sneagan closed 4 weeks ago

Sneagan commented 2 months ago

We need to retry quickly. However, there is a potential race condition for on-chain payments that sign as part of the Authorization step (such as Solana) if we retry too fast:

  1. Authorize call 1 (signs)
  2. Authorize call 2 starts after 1 begins but before it completes (signs)
  3. Authorize call 1 finishes before 2
  4. Pay 1 is called after Authorize 1 finishes, but before Authorize 2 finishes
  5. Pay 2 is called after Authorize 2 finishes but before Pay 1 finishes
  6. Double pay

In the current implementation, this risk is mitigated by proceeding to an Authorized state before generating a signed transaction, ensuring that retries go straight to Pay(). However, this creates another race condition which, while not a risk, results in undesirable failures:

  1. Authorize call
  2. Pay call before Authorize persists a signed transaction
  3. Pay finds ExternalIdempotency data missing and fails immediately

While this doesn't present a double pay risk, it means that we fail unnecessarily when retrying fast. The following changes are needed to both avoid the race condition and the failure.

This means that in the case of the first race condition where two Authorize calls are racing, only the first one will have its signed transaction persisted into QLDB. In that case, a race between two Pay calls will be attempting to submit the same signed transaction and there is no risk of double payment.