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

multiple sendTransactions in short time: same nonce -> replacement transaction underpriced #223

Closed
nikolockenvitz opened this issue Aug 21, 2020 · 3 comments
Assignees
Labels
bug Something isn't working dependencies Refers to an update of a dependency file wontfix This will not be worked on

Comments

@nikolockenvitz
Copy link

Hi,

when I send a few transactions in a short time, only the first one goes through. In the debug log I see the following error message:

daf:ethr-did:identity-controller [ethjs-query] while formatting outputs from RPC '{"value":{"code":-32000,"message":"replacement transaction underpriced"}}'

To give you a reproducable example I created an minimal example repository where I add two services (one right after the other). The first one works but the second one will fail:

for (let i = 0; i < 2; i++) {
  const result = await identity.identityController.addService({
    type: "TestService", serviceEndpoint: `https://${i}.testservice.example.com`,
  });
}

Based on the debug error message I came to the conclusion that both transactions use the same nonce and thus the second one is rejected.
The cause seems to be in ethjs-provider-signer where the transaction count is retrieved and used as nonce. Somehow the previous transaction didn't increment the count yet (maybe because it's not yet in a block?) so the same nonce will be used again.

Possible fixes (?):

  • resolve Promise returned by addService() after transaction is validated
  • make sure transactionCount also counts unvalidated transactions (→ not an issue in this repo)
  • inject a nonce based on the knowledge that there is some pending transaction (?)

If I understand everything correctly I think this might be a bug of how transactionCount works and should be fixed there? Can anyone confirm this? Then it would not be a bug in this repo but somewhere else. In parallel I will investigate this further to find the root cause.

Maybe something similar: When I run getDidDocument() right after adding a service, the service is not yet there (probably transaction is sent but not yet validated in a block).

@nikolockenvitz
Copy link
Author

nikolockenvitz commented Aug 21, 2020

Think I got a fix, but currently a bit hacky.

The transactionCount is got from Infura and there are 3 possible values for BLOCK PARAMETER, i.e. latest, earliest and pending (https://infura.io/docs/ethereum/json-rpc/eth-getTransactionCount).

ethjs-provider-signer asks for latest (see here) but if I change that to pending I get the correct incremented transaction count. A solution would be to update it to pending there but I assume there is a reason why they choose latest. Maybe one can specify this via options and use latest as default. I will open an issue/PR in their repo shortly.

I will leave this issue open because the addService() (and correspondingly addPublicKey()) method is not working as expected and I expect some change is this repo to fix this (e.g. provide some config/options to ethjs-provider-signer).

UPDATE: of course it's not specifically for Infura but part of the general API: https://eth.wiki/json-rpc/API#eth_gettransactioncount

@mirceanis mirceanis added bug Something isn't working dependencies Refers to an update of a dependency file labels Aug 26, 2020
@mirceanis
Copy link
Member

Thank you for reporting this.
Kudos on the very detailed bug report!

Indeed, the generic solution is to use eth_getTransactionCount with the pending block param ( the use of latest is because of legacy reasons, it is not meant to be a general solution).
Depending on the security and consistency constraints, some apps may not want to do this by default, though. So there should be some flexibility here.

We plan to switch to ethers.js as the ethereum layer since it has some nice features that would help here, like using a Promise to resolve the nonce.
This will be done first in ethr-did since that is where the logic for this boils to.
This needs to be paired with some more flexibility to override some transaction parameters when calling these methods from DAF.

relates to #224 #205

@stale
Copy link

stale bot commented Dec 19, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Dec 19, 2020
@stale stale bot closed this as completed Dec 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Refers to an update of a dependency file wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants