fix: Sort recycled nonces in ascending order for reuse or cancellation (#634)

* fix: Sort recycled nonces in ascending order for reuse or cancellation

* fix: Update cancelRecycledNoncesWorker to work with sorted set

Author: d4mr

src/db/wallets/walletNonce.ts

@@ -13,8 +13,8 @@ export const lastUsedNonceKey = (chainId: number, walletAddress: Address) =>
   `nonce:${chainId}:${normalizeAddress(walletAddress)}`;
 
 /**
- * The "recycled nonces" set stores unsorted nonces to be reused or cancelled.
- * Example: [ "25", "23", "24" ]
+ * The "recycled nonces" sorted set stores nonces to be reused or cancelled, sorted by nonce value.
+ * Example: [ "23", "24", "25" ]
  */
 export const recycledNoncesKey = (chainId: number, walletAddress: Address) =>
   `nonce-recycled:${chainId}:${normalizeAddress(walletAddress)}`;
@@ -86,15 +86,15 @@ export const acquireNonce = async (
   chainId: number,
   walletAddress: Address,
 ): Promise<{ nonce: number; isRecycledNonce: boolean }> => {
-  // Acquire an recylced nonce, if any.
-  let nonce = await _acquireRecycledNonce(chainId, walletAddress);
-  if (nonce !== null) {
-    return { nonce, isRecycledNonce: true };
+  // Try to acquire the lowest recycled nonce first
+  const recycledNonce = await _acquireRecycledNonce(chainId, walletAddress);
+  if (recycledNonce !== null) {
+    return { nonce: recycledNonce, isRecycledNonce: true };
   }
 
   // Else increment the last used nonce.
   const key = lastUsedNonceKey(chainId, walletAddress);
-  nonce = await redis.incr(key);
+  let nonce = await redis.incr(key);
   if (nonce === 1) {
     // If INCR returned 1, the nonce was not set.
     // This may be a newly imported wallet.
@@ -125,22 +125,25 @@ export const recycleNonce = async (
   }
 
   const key = recycledNoncesKey(chainId, walletAddress);
-  await redis.sadd(key, nonce.toString());
+  await redis.zadd(key, nonce, nonce.toString());
 };
 
 /**
- * Acquires a recycled nonce that is unused.
+ * Acquires the lowest recycled nonce that is unused.
  * @param chainId
  * @param walletAddress
  * @returns
  */
 const _acquireRecycledNonce = async (
   chainId: number,
   walletAddress: Address,
-) => {
+): Promise<number | null> => {
   const key = recycledNoncesKey(chainId, walletAddress);
-  const res = await redis.spop(key);
-  return res ? parseInt(res) : null;
+  const result = await redis.zpopmin(key);
+  if (result.length === 0) {
+    return null;
+  }
+  return parseInt(result[0]);
 };
 
 const _syncNonce = async (
@@ -166,7 +169,7 @@ const _syncNonce = async (
 /**
  * Returns the last used nonce.
  * This function should be used to inspect nonce values only.
- * Use `incrWalletNonce` if using this nonce to send a transaction.
+ * Use `acquireNonce` to fetch a nonce for sending a transaction.
  * @param chainId
  * @param walletAddress
  * @returns number
@@ -178,7 +181,7 @@ export const inspectNonce = async (chainId: number, walletAddress: Address) => {
 };
 
 /**
- * Delete all wallet nonces. Useful when the get out of sync.
+ * Delete all wallet nonces. Useful when they get out of sync.
  */
 export const deleteAllNonces = async () => {
   const keys = [

src/worker/tasks/cancelRecycledNoncesWorker.ts

@@ -29,16 +29,16 @@ const handler: Processor<any, void, string> = async (job: Job<string>) => {
   const keys = await redis.keys("nonce-recycled:*");
 
   for (const key of keys) {
-    const { chainId, walletAddress } = fromUnusedNoncesKey(key);
+    const { chainId, walletAddress } = fromRecycledNoncesKey(key);
 
-    const unusedNonces = await getAndDeleteUnusedNonces(key);
-    job.log(`Found unused nonces: key=${key} nonces=${unusedNonces}`);
+    const recycledNonces = await getAndDeleteRecycledNonces(key);
+    job.log(`Found recycled nonces: key=${key} nonces=${recycledNonces}`);
 
-    if (unusedNonces.length > 0) {
+    if (recycledNonces.length > 0) {
       const success: number[] = [];
       const fail: number[] = [];
       const ignore: number[] = [];
-      for (const nonce of unusedNonces) {
+      for (const nonce of recycledNonces) {
         try {
           await sendCancellationTransaction({
             chainId,
@@ -65,28 +65,29 @@ const handler: Processor<any, void, string> = async (job: Job<string>) => {
   }
 };
 
-const fromUnusedNoncesKey = (key: string) => {
+const fromRecycledNoncesKey = (key: string) => {
   const [_, chainId, walletAddress] = key.split(":");
   return {
     chainId: parseInt(chainId),
     walletAddress: walletAddress as Address,
   };
 };
 
-const getAndDeleteUnusedNonces = async (key: string) => {
-  // Returns all unused nonces for this key and deletes the key.
+const getAndDeleteRecycledNonces = async (key: string) => {
+  // Returns all recycled nonces for this key and deletes the key.
   // Example response:
   // [
   //   [ null, [ '1', '2', '3', '4' ] ],
   //   [ null, 1 ]
   // ]
-  const multiResult = await redis.multi().smembers(key).del(key).exec();
+  const multiResult = await redis.multi().zrange(key, 0, -1).del(key).exec();
   if (!multiResult) {
     throw new Error(`Error getting members of ${key}.`);
   }
   const [error, nonces] = multiResult[0];
   if (error) {
     throw new Error(`Error getting members of ${key}: ${error}`);
   }
-  return (nonces as string[]).map((v) => parseInt(v)).sort();
+  // No need to sort here as ZRANGE returns elements in ascending order
+  return (nonces as string[]).map((v) => parseInt(v));
 };