feat(common): add sendTransaction, add mempool queue to nonce manager #1717

holic · 2023-10-09T10:08:24Z

pulled out of #1702

Adds a sendTransaction helper to mirror viem's sendTransaction but with our nonce handling
Adds a mempool queue for better nonce handling
Defaults to pending for transaction simulation, transaction count (initializing the nonce manager), etc.

…ueue

changeset-bot · 2023-10-09T10:08:28Z

🦋 Changeset detected

Latest commit: 8e7d8ef

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages

Name	Type
@latticexyz/common	Major
@latticexyz/block-logs-stream	Major
@latticexyz/cli	Major
@latticexyz/config	Major
@latticexyz/dev-tools	Major
@latticexyz/protocol-parser	Major
@latticexyz/store-indexer	Major
@latticexyz/store-sync	Major
@latticexyz/store	Major
@latticexyz/world-modules	Major
@latticexyz/world	Major
@latticexyz/react	Major
@latticexyz/abi-ts	Major
create-mud	Major
@latticexyz/ecs-browser	Major
@latticexyz/faucet	Major
@latticexyz/gas-report	Major
@latticexyz/network	Major
@latticexyz/noise	Major
@latticexyz/phaserx	Major
@latticexyz/recs	Major
@latticexyz/schema-type	Major
@latticexyz/services	Major
@latticexyz/solecs	Major
solhint-config-mud	Major
solhint-plugin-mud	Major
@latticexyz/std-client	Major
@latticexyz/std-contracts	Major
@latticexyz/store-cache	Major
@latticexyz/utils	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

holic · 2023-10-09T10:16:50Z

packages/common/src/createNonceManager.ts

@@ -68,10 +70,13 @@ export function createNonceManager({
    );
  }

+  const mempoolQueue = new PQueue({ concurrency: 1 });


My initial prototype of the nonce manager had this but later removed it because it didn't seem to help much, but once I started working on #1702 and aligned the block tag for 1) getting initial nonce value and 2) simulating txs, I found it much more stable to have a queue in front of the mempool.

Once you start sending lots of parallel txs (like we do in deploys), it's important that they land in the mempool in nonce order, and you can't really guarantee this unless you queue them like this. Submitting to mempool is fairly quick in most cases, but is slower than what we had before. #1702 does a better job at parallelizing overall, so it should be noticeably faster to deploy, even though we're technically waiting longer for submitting to the mempool.

and yes, we could make the concurrency configurable, but I think it might be worth waiting until we have tests and can really stress test the different options to decide if it's worth allowing this to be parallelized

holic · 2023-10-09T10:17:19Z

packages/common/src/createNonceManager.ts

 };

 export function createNonceManager({
  client,
-  address,
-  blockTag = "latest",
+  address, // TODO: rename to account?


didn't want to introduce a breaking change here so leaving this as a TODO

holic · 2023-10-09T10:22:12Z

packages/common/src/getNonceManagerId.ts

@@ -10,6 +10,7 @@ export async function getNonceManagerId({
  address: Hex;
  blockTag: BlockTag;
 }): Promise<string> {
+  // TODO: improve this so we don't have to call getChainId every time


right now, this is going to make an eth_chainId call per use of writeContract and sendTransaction

we can improve this by threading through a chain or chain ID from the upstream callers, but it's unclear what the "best practice" is in terms of viem client handling of chains (should they be mutable or not) and what to do for wallets that do have "mutable" chains (e.g. a Metamask wallet can switch network, but unclear if that should create a new client object or if it should mutate the chain on the existing client object)

once I get clarity, I'll follow up and optimize this to remove the extra call

alvrs · 2023-10-09T10:48:49Z

packages/common/src/sendTransaction.ts

+  client: Client<Transport, TChain, TAccount>,
+  request: SendTransactionParameters<TChain, TAccount, TChainOverride>
+): Promise<WriteContractReturnType> {
+  const account_ = request.account ?? client.account;


my initial question was "why account_ instead of account" but i see it's because we call parseAccount on account_ to get account below. Should we call this variable unparsedAccount or rawAccount then?

agree on clarity! I initially had this but then switched to account_ to match what viem uses internally, but happy to go back to the clearer name

alvrs · 2023-10-09T10:52:31Z

packages/common/src/sendTransaction.ts

+    debug("simulating tx to", request.to);
+    await call(client, {
+      ...request,
+      blockTag: "pending",
+      account,
+    } as CallParameters<TChain>);
+
+    // TODO: estimate gas


is there more to do here than to use estimateGas instead of call?

ahh so I thought viem internally did both a call/simulate + estimate gas, but looking closer at sendTransaction and prepareTransactionRequest, it looks like it's just a gas estimate (likely because we don't need the simulated return value in this context)

will see about updating, but may need to also have to add gas retry handling if there's too much time between calling this function and the tx getting sent to the mem pool (due to queue and/or nonce retries)

for context: I was initially just doing the simulate, not the gas estimate, because I wanted to know if it was safe to increment the nonce but still allow for the gas to be computed just before sending the tx

but since gas estimate is ~the same RPC cost as simulate and we can attempt to send the tx with potentially outdated gas and save a call, this seems worth it (assuming we have proper retry handling for bad gas)

~~looks like viem does use the return data of the simulate call for better error messages in case of failures/reverts~~ seems like both estimate gas and call revert with the same data, so I am now thinking there's something wrong with how errors are being parsed

I attempted to do this, along with consolidating behavior, but ran into issues with being able to parse errors from viem. I've already burned a few hours on this, so I'd like to pause and come back to this another time in a follow-up: 97fd6d7

sounds good. Do you think it's a viem bug or something on our side? If the former, is there a repro we could share with them?

I wasn't able to confirm if it was a viem issue or not. Using their StackBlitz starter kit didn't seem to surface it. But it definitely happens within the MUD repo, even though everything is pinned to exactly the same version (tried clearing all node modules and reinstalling and also tried bumping to a different exactly version, with no change in behavior I was seeing).

alvrs · 2023-10-09T10:58:15Z

packages/common/src/sendTransaction.ts

+  return await nonceManager.mempoolQueue.add(
+    () =>
+      pRetry(
+        async () => {
+          if (!nonceManager.hasNonce()) {
+            await nonceManager.resetNonce();
+          }
+
+          const nonce = nonceManager.nextNonce();
+          debug("sending tx with nonce", nonce, "to", preparedRequest.to);
+          return await viem_sendTransaction(client, { nonce, ...preparedRequest });
+        },
+        {
+          retries: 3,
+          onFailedAttempt: async (error) => {
+            // On nonce errors, reset the nonce and retry
+            if (nonceManager.shouldResetNonce(error)) {
+              debug("got nonce error, retrying", error.message);
+              await nonceManager.resetNonce();
+              return;
+            }
+            // TODO: prepare again if there are gas errors?
+            throw error;
+          },
+        }
+      ),
+    { throwOnTimeout: true }
+  );


for context / my understanding: with mempoolQueue concurrency set to 1 we wait for the transaction to be accepted by the rpc node before attempting to send the next transaction, but we're not waiting for the transaction to be included in a block, so it's still possible to include multiple transactions in one block, correct? While before we were "preparing" to see if the node would accept the transaction, but we weren't actually waiting for it to accept it before sending another one, so in some edge case it could have happened that we're sending two transactions with subsequent nonces, the first one gets dropped for some reason, and the second one is stuck?

Is this correct or am I missing something in my understanding?

with mempoolQueue concurrency set to 1 we wait for the transaction to be accepted by the rpc node before attempting to send the next transaction, but we're not waiting for the transaction to be included in a block, so it's still possible to include multiple transactions in one block, correct?

correct

While before we were "preparing" to see if the node would accept the transaction, but we weren't actually waiting for it to accept it before sending another one, so in some edge case it could have happened that we're sending two transactions with subsequent nonces, the first one gets dropped for some reason, and the second one is stuck?

correct! or, what I think I was seeing is that when parallelizing lots of txs, the node would receive the set of txs out of nonce order, so some would just fail/not be accepted by the node

This reverts commit 2796077c0a474758e7a11321d5d66d0f9a8e9944.

holic added 2 commits October 9, 2023 10:50

feat(common): add sendTransaction, add mempool queue to nonce manager

b9fb594

Merge remote-tracking branch 'origin/main' into holic/nonce-mempool-q…

d1513a7

…ueue

vercel bot deployed to Preview October 9, 2023 10:09 View deployment

holic commented Oct 9, 2023

View reviewed changes

missing import

661b7d0

vercel bot deployed to Preview October 9, 2023 10:25 View deployment

Create few-brooms-accept.md

0ede2ef

vercel bot deployed to Preview October 9, 2023 10:27 View deployment

holic marked this pull request as ready for review October 9, 2023 10:37

holic requested a review from alvrs as a code owner October 9, 2023 10:37

alvrs previously approved these changes Oct 9, 2023

View reviewed changes

holic added 3 commits October 9, 2023 14:57

explicit block tag

13fdb44

attempt to consolidate logic

97fd6d7

Revert "attempt to consolidate logic"

2035c3a

This reverts commit 2796077c0a474758e7a11321d5d66d0f9a8e9944.

holic dismissed alvrs’s stale review via 2035c3a October 9, 2023 13:57

vercel bot deployed to Preview October 9, 2023 13:59 View deployment

account_ -> rawAccount

8e7d8ef

vercel bot deployed to Preview October 9, 2023 14:02 View deployment

alvrs approved these changes Oct 9, 2023

View reviewed changes

holic merged commit 0660561 into main Oct 9, 2023

holic deleted the holic/nonce-mempool-queue branch October 9, 2023 14:57

This was referenced Oct 10, 2023

try to simulate with pending block tag #1700

Closed

nonce manager should use pending blockTag for getTransactionCount #1698

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(common): add sendTransaction, add mempool queue to nonce manager #1717

feat(common): add sendTransaction, add mempool queue to nonce manager #1717

holic commented Oct 9, 2023 •

edited

Loading

changeset-bot bot commented Oct 9, 2023 •

edited

Loading

holic Oct 9, 2023

holic Oct 9, 2023

holic Oct 9, 2023

holic Oct 9, 2023

alvrs Oct 9, 2023

holic Oct 9, 2023

alvrs Oct 9, 2023

holic Oct 9, 2023

holic Oct 9, 2023 •

edited

Loading

holic Oct 9, 2023 •

edited

Loading

holic Oct 9, 2023

alvrs Oct 9, 2023

holic Oct 9, 2023

alvrs Oct 9, 2023

holic Oct 9, 2023

feat(common): add sendTransaction, add mempool queue to nonce manager #1717

feat(common): add sendTransaction, add mempool queue to nonce manager #1717

Conversation

holic commented Oct 9, 2023 • edited Loading

changeset-bot bot commented Oct 9, 2023 • edited Loading

🦋 Changeset detected

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

holic Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

holic Oct 9, 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

holic commented Oct 9, 2023 •

edited

Loading

changeset-bot bot commented Oct 9, 2023 •

edited

Loading

holic Oct 9, 2023 •

edited

Loading

holic Oct 9, 2023 •

edited

Loading