Merge pull request #1321 from JoinColony/fix/mtx-broadcaster-rebroadcasts

area · web-flow · commit 6554f0723cc3 · 2025-02-07T10:14:57.000Z
Adjust robustness of MTX broadcaster
diff --git a/hardhat.config.js b/hardhat.config.js
@@ -44,16 +44,20 @@ task("test", "Run tests").setAction(async () => {
   app.use(bodyParser.json());
   app.post("/", async function (req, res) {
     try {
+      // console.log(">>>>>>", req.body);
       const response = await hre.network.provider.request(req.body);
+      // console.log("<<<<<<", response);
       res.send({ jsonrpc: "2.0", result: response, id: req.body.id });
     } catch (error) {
-      const abiCoder = new ethers.utils.AbiCoder();
-      const decoded = abiCoder.decode(["string"], `0x${error.data.slice(10)}`);
-      res.send({
-        jsonrpc: "2.0",
-        error: { message: `Error: VM Exception while processing transaction: reverted with reason string '${decoded[0]}'` },
-        id: req.body.id,
-      });
+      console.log(error);
+      try {
+        const abiCoder = new ethers.utils.AbiCoder();
+        const decoded = abiCoder.decode(["string"], `0x${error.data.slice(10)}`);
+        const message = `Error: VM Exception while processing transaction: reverted with reason string '${decoded[0]}'`;
+        res.send({ jsonrpc: "2.0", error: { message }, id: req.body.id });
+      } catch (e) {
+        res.send({ jsonrpc: "2.0", error: { message: error.message }, id: req.body.id });
+      }
     }
   });
 
diff --git a/helpers/test-helper.js b/helpers/test-helper.js
@@ -579,6 +579,25 @@ exports.revert = async function revert(provider, snapshotId) {
   });
 };
 
+exports.hardhatDropTransaction = async function hardhatDropTransaction(provider, txHash) {
+  return new Promise((resolve, reject) => {
+    provider.send(
+      {
+        jsonrpc: "2.0",
+        method: "hardhat_dropTransaction",
+        params: [txHash],
+        id: new Date().getTime(),
+      },
+      (err) => {
+        if (err) {
+          return reject(err);
+        }
+        return resolve();
+      },
+    );
+  });
+};
+
 exports.hardhatSnapshot = async function hardhatSnapshot(provider) {
   const res = await provider.request({
     method: "evm_snapshot",
diff --git a/packages/package-utils/ExtendedNonceManager.js b/packages/package-utils/ExtendedNonceManager.js
@@ -1,46 +1,21 @@
 const { NonceManager } = require("@ethersproject/experimental");
 
 class ExtendedNonceManager extends NonceManager {
-  constructor(signer) {
-    super(signer);
-    this.signedTransactions = {};
-    this.nonce = 0;
-    this.signer.provider.on("block", async () => {
-      Object.keys(this.signedTransactions).map(async (txHash) => {
-        const nodeTx = await this.signer.provider.getTransaction(txHash);
-        if (!nodeTx) {
-          this.signer.provider.sendTransaction(this.signedTransactions[txHash]);
-          return;
-        }
-        if (nodeTx.blockNumber) {
-          // It's been mined, so forget it.
-          delete this.signedTransactions[txHash];
-        }
-        // Otherwise it's known, but not mined yet. No action required.
-      });
-    });
-  }
-
   async sendTransaction(transactionRequest) {
-    // What nonce are we going to attach to this?
-    // Definitely not any we've sent and are pending
-    const pendingNonces = Object.keys(this.signedTransactions).map((txhash) => this.signedTransactions[txhash].nonce);
-
-    // At least whatever the endpoint says, or whatever we've already sent if higher
-    let nonce = await this.signer.getTransactionCount();
-
-    // Note the order we did the above two lines in - if a tx is mined between these two lines,
-    // and got removed by the `on block` handler above, by doing it in this order we won't be tripped up
-    // And we'll skip any nonces we've already used
-    while (pendingNonces.includes(nonce)) {
-      nonce += 1;
+    try {
+      const txCount = await this.signer.getTransactionCount("latest");
+      this.setTransactionCount(txCount);
+      const response = super.sendTransaction(transactionRequest);
+      const tx = await response;
+      await tx.wait();
+      return response;
+    } catch (e) {
+      if (e.code === "NONCE_EXPIRED" || e.code === "TRANSACTION_REPLACED") {
+        // The nonce has expired, so try again
+        return this.sendTransaction(transactionRequest);
+      }
+      throw e;
     }
-    transactionRequest.nonce = nonce; // eslint-disable-line no-param-reassign
-    this.nonce = nonce + 1;
-    const response = super.sendTransaction(transactionRequest);
-    const tx = await response;
-    this.signedTransactions[tx.hash] = transactionRequest;
-    return response;
   }
 }
 
diff --git a/packages/package-utils/RetryProvider.js b/packages/package-utils/RetryProvider.js
@@ -8,7 +8,7 @@ class RetryProvider extends ethers.providers.StaticJsonRpcProvider {
   }
 
   static attemptCheck(err, attemptNumber) {
-    const allowedErrorCodes = ["CALL_EXCEPTION", "UNPREDICTABLE_GAS_LIMIT"];
+    const allowedErrorCodes = ["CALL_EXCEPTION", "UNPREDICTABLE_GAS_LIMIT", "NONCE_EXPIRED"];
     if (allowedErrorCodes.includes(err.code)) {
       console.log(`Got a ${err.code}, no retrying`);
       return false;
diff --git a/test/packages/metaTransactionBroadcaster.js b/test/packages/metaTransactionBroadcaster.js
@@ -9,7 +9,15 @@ const axios = require("axios");
 const { TruffleLoader, RetryProvider } = require("../../packages/package-utils");
 const { setupEtherRouter } = require("../../helpers/upgradable-contracts");
 const { UINT256_MAX, CURR_VERSION } = require("../../helpers/constants");
-const { web3GetTransaction, currentBlockTime } = require("../../helpers/test-helper");
+const {
+  web3GetTransaction,
+  currentBlockTime,
+  stopMining,
+  startMining,
+  sleep,
+  hardhatDropTransaction,
+  mineBlock,
+} = require("../../helpers/test-helper");
 
 const MetatransactionBroadcaster = require("../../packages/metatransaction-broadcaster/MetatransactionBroadcaster");
 const { getMetaTransactionParameters, getPermitParameters, setupColony } = require("../../helpers/test-data-generator");
@@ -379,17 +387,21 @@ contract("Metatransaction broadcaster", (accounts) => {
       expect(balanceAccount2).to.eq.BN(1200000);
       const balanceColony = await metaTxToken.balanceOf(colony.address);
       expect(balanceColony).to.eq.BN(600000);
+
+      // Check the transactions happened with the hashes returned
+      const tx1 = await web3GetTransaction(txHash);
+      expect(tx1.gas).to.be.lt.BN(500000);
+      const tx2 = await web3GetTransaction(txHash2);
+      expect(tx2.gas).to.be.lt.BN(500000);
     });
 
-    it("a valid transaction is broadcast and mined, even if the broadcaster's nonce manager fell behind", async function () {
+    it("a nonce collision is resolved correctly", async function () {
       await metaTxToken.unlock();
       await metaTxToken.mint(USER0, 1500000, { from: USER0 });
-      await metaTxToken.mint(USER1, 1500000, { from: USER0 });
 
       const txData = await metaTxToken.contract.methods.transfer(colony.address, 300000).encodeABI();
 
       const { r, s, v } = await getMetaTransactionParameters(txData, USER0, metaTxToken.address);
-      const { r: r2, s: s2, v: v2 } = await getMetaTransactionParameters(txData, USER1, metaTxToken.address);
 
       // Send to endpoint
 
@@ -402,21 +414,98 @@ contract("Metatransaction broadcaster", (accounts) => {
         v,
       };
 
-      const res = await axios.post("http://127.0.0.1:3000/broadcast", jsonData, {
+      await stopMining();
+      const p = new web3.eth.providers.HttpProvider("http://localhost:8545");
+
+      const postRequest = axios.post("http://127.0.0.1:3000/broadcast", jsonData, {
         headers: {
           "Content-Type": "application/json",
         },
       });
 
+      let firstTxHash = null;
+      while (!firstTxHash) {
+        const block = await provider.send("eth_getBlockByNumber", ["pending", true]);
+        if (block.transactions.length > 0) {
+          const [firstTx] = block.transactions;
+          firstTxHash = firstTx.hash;
+        }
+        await sleep(1000);
+      }
+
+      await hardhatDropTransaction(p, firstTxHash);
+
+      const currentNonce = await provider.getTransactionCount(USER0);
+
+      await startMining();
+
+      // This will have the nonce of the dropped transaction, which the broadcaster will try to
+      // rebroadcast when it sees a block, but it will fail because the nonce is too low
+      // it should then rebroadcast with the correct nonce
+      await metaTxToken.mint(USER1, 1, { from: USER0 });
+
+      let newNonce = 0;
+      while (newNonce < currentNonce + 2) {
+        newNonce = await provider.getTransactionCount(USER0);
+        await mineBlock();
+        await sleep(1000);
+      }
+      const res = await postRequest;
+
+      const { txHash } = res.data.data;
+      expect(txHash.length).to.be.equal(66);
+
+      expect(res.data).to.be.deep.equal({
+        status: "success",
+        data: {
+          txHash,
+        },
+      });
+
+      // Check the transaction happened
+      const balanceAccount1 = await metaTxToken.balanceOf(USER0);
+      expect(balanceAccount1).to.eq.BN(1200000);
+      const balanceColony = await metaTxToken.balanceOf(colony.address);
+      expect(balanceColony).to.eq.BN(300000);
+
+      // Check the transactions happened with the hashes returned
+      const tx1 = await web3GetTransaction(txHash);
+      if (!tx1) {
+        throw new Error("Transaction not found");
+      }
+      expect(tx1.gas).to.be.lt.BN(500000);
+
+      const originalTx = await web3GetTransaction(firstTxHash);
+      expect(originalTx).to.be.null;
+    });
+
+    it("a valid transaction is broadcast and mined, even if the broadcaster's nonce manager fell behind", async function () {
+      await metaTxToken.unlock();
+      await metaTxToken.mint(USER0, 1500000, { from: USER0 });
+      await metaTxToken.mint(USER1, 1500000, { from: USER0 });
+
+      const txData = await metaTxToken.contract.methods.transfer(colony.address, 300000).encodeABI();
+
+      const { r, s, v } = await getMetaTransactionParameters(txData, USER0, metaTxToken.address);
+
+      const txCount = await provider.getTransactionCount(accounts[0]);
+      await broadcaster.nonceManager.setTransactionCount(txCount);
+
       // Make an unexpected transaction
       await metaTxToken.mint(USER2, 1500000, { from: USER0 });
 
-      jsonData.r = r2;
-      jsonData.s = s2;
-      jsonData.v = v2;
-      jsonData.userAddress = USER1;
+      // Send to endpoint
+
+      const jsonData = {
+        target: metaTxToken.address,
+        payload: txData,
+        userAddress: USER0,
+        r,
+        s,
+        v,
+      };
 
-      await axios.post("http://127.0.0.1:3000/broadcast", jsonData, {
+      const res = await axios.post("http://127.0.0.1:3000/broadcast", jsonData, {
         headers: {
           "Content-Type": "application/json",
         },
@@ -437,7 +526,11 @@ contract("Metatransaction broadcaster", (accounts) => {
       const balanceAccount1 = await metaTxToken.balanceOf(USER0);
       expect(balanceAccount1).to.eq.BN(1200000);
       const balanceAccount2 = await metaTxToken.balanceOf(colony.address);
-      expect(balanceAccount2).to.eq.BN(600000);
+      expect(balanceAccount2).to.eq.BN(300000);
+
+      // Check the transactions happened with the hashes returned
+      const tx1 = await web3GetTransaction(txHash);
+      expect(tx1.gas).to.be.lt.BN(500000);
     });
 
     it("a valid transaction is broadcast and mined, even if the broadcaster's nonce manager got ahead", async function () {
@@ -448,7 +541,6 @@ contract("Metatransaction broadcaster", (accounts) => {
       const txData = await metaTxToken.contract.methods.transfer(colony.address, 300000).encodeABI();
 
       const { r, s, v } = await getMetaTransactionParameters(txData, USER0, metaTxToken.address);
-      const { r: r2, s: s2, v: v2 } = await getMetaTransactionParameters(txData, USER1, metaTxToken.address);
 
       // Send to endpoint
 
@@ -461,22 +553,11 @@ contract("Metatransaction broadcaster", (accounts) => {
         v,
       };
 
-      let res = await axios.post("http://127.0.0.1:3000/broadcast", jsonData, {
-        headers: {
-          "Content-Type": "application/json",
-        },
-      });
-
       // Set the nonce
       const txCount = await provider.getTransactionCount(accounts[0]);
       await broadcaster.nonceManager.setTransactionCount(txCount + 1);
 
-      jsonData.r = r2;
-      jsonData.s = s2;
-      jsonData.v = v2;
-      jsonData.userAddress = USER1;
-
-      res = await axios.post("http://127.0.0.1:3000/broadcast", jsonData, {
+      const res = await axios.post("http://127.0.0.1:3000/broadcast", jsonData, {
         headers: {
           "Content-Type": "application/json",
         },
@@ -497,7 +578,11 @@ contract("Metatransaction broadcaster", (accounts) => {
       const balanceAccount1 = await metaTxToken.balanceOf(USER0);
       expect(balanceAccount1).to.eq.BN(1200000);
       const balanceAccount2 = await metaTxToken.balanceOf(colony.address);
-      expect(balanceAccount2).to.eq.BN(600000);
+      expect(balanceAccount2).to.eq.BN(300000);
+
+      // Check the transaction happened with the hash returned
+      const tx = await web3GetTransaction(txHash);
+      expect(tx.gas).to.be.lt.BN(500000);
     });
 
     it("a valid EIP712 transaction is broadcast and mined", async function () {
@@ -539,6 +624,10 @@ contract("Metatransaction broadcaster", (accounts) => {
       // Check the transaction happened
       const allowed = await metaTxToken.allowance(USER0, colony.address);
       expect(allowed).to.eq.BN(1);
+
+      // With the expected hash
+      const tx = await web3GetTransaction(txHash);
+      expect(tx.gas).to.be.lt.BN(500000);
     });
 
     it("an EIP712 transaction with an invalid spender is not broadcast and mined", async function () {
@@ -600,26 +689,22 @@ contract("Metatransaction broadcaster", (accounts) => {
         s,
         v,
       };
-      try {
-        const res = await axios.post("http://127.0.0.1:3000/broadcast", jsonData, {
-          headers: {
-            "Content-Type": "application/json",
-          },
-        });
+      const res = await axios.post("http://127.0.0.1:3000/broadcast", jsonData, {
+        headers: {
+          "Content-Type": "application/json",
+        },
+      });
 
-        const { txHash } = res.data.data;
+      const { txHash } = res.data.data;
 
-        expect(txHash.length).to.be.equal(66);
+      expect(txHash.length).to.be.equal(66);
 
-        expect(res.data).to.be.deep.equal({
-          status: "success",
-          data: {
-            txHash,
-          },
-        });
-      } catch (err) {
-        console.log(err.response.data);
-      }
+      expect(res.data).to.be.deep.equal({
+        status: "success",
+        data: {
+          txHash,
+        },
+      });
 
       // Check the transaction happened
       const roles = await colony.getUserRoles(USER1, 1);
@@ -628,6 +713,10 @@ contract("Metatransaction broadcaster", (accounts) => {
 
       const expectedRoles = roleArchitecture | roleFunding; // eslint-disable-line no-bitwise
       expect(roles).to.equal(ethers.utils.hexZeroPad(ethers.BigNumber.from(expectedRoles).toHexString(), 32));
+
+      // With the expected hash
+      const tx = await web3GetTransaction(txHash);
+      expect(tx.gas).to.be.lt.BN(500000);
     });
 
     it("a multicall transaction that calls something invalid is rejected", async function () {

Original file line number	Diff line number	Diff line change
`@@ -8,7 +8,7 @@ class RetryProvider extends ethers.providers.StaticJsonRpcProvider {`
`8`	`8`	`}`
`9`	`9`
`10`	`10`	`static attemptCheck(err, attemptNumber) {`
`11`		`- const allowedErrorCodes = ["CALL_EXCEPTION", "UNPREDICTABLE_GAS_LIMIT"];`
	`11`	`+ const allowedErrorCodes = ["CALL_EXCEPTION", "UNPREDICTABLE_GAS_LIMIT", "NONCE_EXPIRED"];`
`12`	`12`	`if (allowedErrorCodes.includes(err.code)) {`
`13`	`13`	console.log(`Got a ${err.code}, no retrying`);
`14`	`14`	`return false;`