fix(txm): nonce gap

[GabrielMartinezRodriguez]

Description

The randomness service got stuck due to a nonce gap. This happened because, in the case where the server crashes for some reason, we might leave a transaction unsaved, while at the same time saving another transaction with a higher nonce. When the nonce manager starts and calculates the nonce gaps, we were incorrectly assuming that the nonce obtained from the blockchain's transaction count represents the last executed transaction for that account. However, that value actually represents the next available nonce.

For example, suppose we are processing two new transactions with nonces 9 and 10. If the server crashes and only the transaction with nonce 10 is saved, when the service restarts, it will calculate the nonce gaps assuming that nonce 9 was already used—because the transaction count returned from the blockchain indicates the next available nonce, not the last one executed. This leads to the gap and causes the service to get stuck

• Include all relevant context (but no need to repeat the issue's content).
• Draw attention to new, noteworthy & unintuitive elements.

Toggle Checklist

Checklist

Basics

• B1. I have applied the proper label & proper branch name (e.g. norswap/build-system-caching).
• B2. This PR is not so big that it should be split & addresses only one concern.
• B3. The PR targets the lowest branch it can (ideally master).

Reminder: PR review guidelines

Correctness

• C1. Builds and passes tests.
• C2. The code is properly parameterized & compatible with different environments (e.g. local,
  testnet, mainnet, standalone wallet, ...).
• C3. I have manually tested my changes & connected features.

< INDICATE BROWSER, DEMO APP & OTHER ENV DETAILS USED FOR TESTING HERE >

< INDICATE TESTED SCENARIOS (USER INTERFACE INTERACTION, CODE FLOWS) HERE >

• C4. I have performed a thorough self-review of my code after submitting the PR,
  and have updated the code & comments accordingly.

Architecture & Documentation

• D1. I made it easy to reason locally about the code, by (1) using proper abstraction boundaries,
  (2) commenting these boundaries correctly, (3) adding inline comments for context when needed.
• D2. All public-facing APIs & meaningful (non-local) internal APIs are properly documented in code
  comments.
• D3. If appropriate, the general architecture of the code is documented in a code comment or
  in a Markdown document.
• D4. An appropriate Changeset has been generated (and committed) for changes that touch npm published packages (currently packages/core and packages/react), see here for more info.

[cloudflare-workers-and-pages]

Deploying happychain with    Cloudflare Pages

Latest commit:	de3af6c
Status:	✅  Deploy successful!
Preview URL:	https://d2fae4a2.happychain.pages.dev
Branch Preview URL:	https://gabriel-fix-randomnness.happychain.pages.dev

View logs

[GabrielMartinezRodriguez]

• Randomness monitor service #584
• Txm traces #521
• fix(txm): returned nonce queue order #578
• fix(txm): not process the same block multiple times #577
• Fix: Viem sends random undefined blocks #575
• fix(txm): heap out of memory #573
• Avoid initiating new attempts if the gas conditions persist unchanged #571
• Dynamic priority fee #570
• TXM: Rpc liveness #562
• fix(txm): nonce gap #561 👈 (View in Graphite)
• PoC: Add metrics to TXM #503
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[not-reed]

probably need to clamp this to not go below zero?

[aodhgan]

agree, edge case for when nonce is 0 is unlikely but technically possible for a new txm instance

[GabrielMartinezRodriguez]

It’s actually the expected behavior for it to be -1. If the user has an account that has never been used, the blockchain nonce will be 0, and therefore the max executed nonce will become -1. We use the max executed nonce to check if there is any pending transaction with a nonce lower than the maximum executed nonce, and if so, we move it to Interrupted.

if (nonce <= this.transactionManager.nonceManager.maxExecutedNonce) {
    transaction.changeStatus(TransactionStatus.Interrupted)
    return
}

So I think it’s fine for it to remain at -1 because it serves our purpose.

If we change it to undefined, I think the code is going to get messier, because we would have to handle the undefined case in several places, whereas -1 is fully functional

[norswap]

This needs to revert — if it causes an issue for you, you'll need to foundryup to get the 1.0.0 release.

[GabrielMartinezRodriguez]

done