Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Remove NonceManager #1326

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

fix: Remove NonceManager #1326

wants to merge 1 commit into from

Conversation

MantisClone
Copy link
Member

@MantisClone MantisClone commented Jan 16, 2024

Description of the changes

  • Remove the NonceManager from the Request Node

Motivation

(Copied from https://github.com/RequestNetwork/marble/pull/105 written by @alexandre-abrioux)

The NonceManager does not handle catching errors from the RPC.

When the RPC refuses a transaction, because for instance:

  • the transaction gas fees are too low for the RPC to accept it
  • or the RPC times out

; then an error is thrown and not handled by the NonceManager. In that case, the NonceManager still increases its internal counter.

Since the initial transaction was never persisted on-chain, the following transactions are never picked up by the network because their nonce is too high.

More sources:
In ethers.js documentation https://docs.ethers.org/v5/api/experimental/#experimental-noncemanager

Currently the NonceManager does not handle re-broadcast. If you attempt to send a lot of transactions to the network on a node that does not control that account, the transaction pool may drop your transactions.

In the future, it'd be nice if the NonceManager remembered transactions and watched for them on the network, rebroadcasting transactions that appear to have been dropped.

In ethers.js code (latest version, v6, not the version we use): https://github.com/ethers-io/ethers.js/blob/v6.9.0/src.ts/providers/signer-noncemanager.ts#L82
there is this comment:

// @todo: Maybe handle interesting/recoverable errors?
// Like don't increment if the tx was certainly not sent

This is the code for the currently used version of ethers.js (v5): https://github.com/ethers-io/ethers.js/blob/v5.5.1/packages/experimental/src.ts/nonce-manager.ts

Additionally, I suspect that the "OldNonce" error was also caused by the NonceManager.
Fixes #1292

@coveralls
Copy link

Coverage Status

coverage: 87.126% (-0.002%) from 87.128%
when pulling 823d26b on remove-nonce-manager
into 5c83706 on master.

benjlevesque
benjlevesque previously approved these changes Jan 16, 2024
Copy link
Contributor

@benjlevesque benjlevesque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try this and monitor; I'm not 100% sure it will increase stability. Indeed Marble's implementation (note: Marble is an asynchronous resilient persistance layer implementation for RequestNetwork, developped at Request Finance) does not allow multiple transactions to happen simultenaously.

@MantisClone
Copy link
Member Author

Agreed! I wasn't sure on this one either. I'll be ready to roll back if needed.

KolevDarko
KolevDarko previously approved these changes Jan 17, 2024
@MantisClone
Copy link
Member Author

Actually I think I'll hold off merging this and do more local testing first.

leoslr
leoslr previously approved these changes Jan 21, 2024
@MantisClone MantisClone marked this pull request as draft January 26, 2024 02:00
@MantisClone MantisClone dismissed stale reviews from leoslr, KolevDarko, alexandre-abrioux, and benjlevesque January 26, 2024 02:01

PR needs more work.

@kevindavee kevindavee removed their request for review March 26, 2024 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix "OldNonce" errors in Request Node. Separate receiving and persisting using a queue.
6 participants