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

Should web3.eth.accounts.signTransaction convert nonce to hex? #3281

Closed
michaelsbradleyjr opened this issue Dec 18, 2019 · 6 comments
Closed
Labels
1.x 1.0 related issues Discussion

Comments

@michaelsbradleyjr
Copy link
Contributor

michaelsbradleyjr commented Dec 18, 2019

Expected behavior

web3.eth.accounts.signTransaction should convert tx nonce property to hex.

(I'm actually not 100% sure about this, that's why the title for this issue is a question.)

Actual behavior

The nonce property is not modified, e.g. if nonce was 123 it does not become '0x7b'.

This seems to be a changed behavior after v1.2.1. See also: https://github.com/ethereum/web3.js/pull/3125/files#diff-7bb2a20126193b9ecfe4723f83429c49L172.

The reason I noticed is that geth complains about a nonce that can't be properly unmarshaled because it's a number instead of a valid hex string.

Versions

web3 v1.2.4

@cgewecke
Copy link
Collaborator

cgewecke commented Dec 19, 2019

@michaelsbradleyjr

Do you happen to have a reproduction case we could look at for the marshalling error?

Have tried to trigger it in #3282 without luck. There are passing E2E tests vs a geth instance running in --dev mode for nonce formatting cases here. (Geth is pulled from docker as ethereum:client-go:stable.)

Walking the code it looks like the tx object is passed to formatters._txInputFormatters, which seems to handle the nonce and is tested here.

@michaelsbradleyjr
Copy link
Contributor Author

michaelsbradleyjr commented Dec 19, 2019

Right now I don't have a reproduction case that's easy to run. I can provide a bit more context and on my end will try to narrow down what's happening.

Context: https://github.com/embark-framework/embark/blob/master/packages/plugins/rpc-manager/src/lib/eth_sendTransaction.ts#L25

I'm working on a branch that upgrades web3 from 1.2.1 to 1.2.4. What I found is that unless I changed that line to what's below then geth would send an unmarshalling error:

payload.nonce = `0x${newNonce.toString(16)}`;

The biggest difference between the E2E test you setup and what the embark code is doing is that your code, after signing, sends with web3.eth.sendSignedTransaction. In embark the signing takes place in a proxy that sits logically in front of the real node (geth or parity); after the tx is signed the in-flight http/ws request gets updated and propagated to the real node.

Does formatters._txInputFormatters get involved in web3.eth.accounts.signTransaction or web3.eth.sendSignedTransaction?

@cgewecke
Copy link
Collaborator

@michaelsbradleyjr Yes, in accounts.signTransaction, the formatters are run here:
https://github.com/ethereum/web3.js/blob/4d57461fe8e9d26662d5a5afc9c03f38fb7a28be/packages/web3-eth-accounts/src/index.js#L175

Logging the transaction object immediately after that for the case where nonce is not defined (and transactionCount === 0) yields:

{
 "to": "0x7d829ae78ac722d5788afe7bb8cb20e085806bb8",
 "value": "0x16345785d8a0000",
 "gasLimit": "0x5208",
 "gasPrice": "0x2540be400",
 "chainId": 1337,
 "nonce": "0x0",
 "networkId": 1,
 "gas": "0x5208"
}

Where nonce is a number and transactionCount is 1;

{
 "nonce": "0x1",
 "to": "0x7d829ae78ac722d5788afe7bb8cb20e085806bb8",
 "value": "0x16345785d8a0000",
 "gasLimit": "0x5208",
 "gasPrice": "0x2540be400",
 "chainId": 1337,
 "networkId": 1,
 "gas": "0x5208"
}

The final output of signTransaction looks something like:

{
 "messageHash": "0x0014eb06b15ae6b9e8ff7bd7eb1267dce551a2acc33bf12d6ceb65ccfeb03065",
 "v": "0x0a96",
 "r": "0x7f4b15c2ad7475f99ee72de3bdf76522e58253cb7c0f34c1fcbf2852c5ea245f",
 "s": "0x48d53638b670f6db6c03af39c76f3e68aa24ce3af8a517e3f47058111e07f86a",
 "rawTransaction": "0xf86e808502540be40082520894f06ed35dd28a47898bd22143dd3c4d3619c030cb88016345785d8a000080820a96a07f4b15c2ad7475f99ee72de3bdf76522e58253cb7c0f34c1fcbf2852c5ea245fa048d53638b670f6db6c03af39c76f3e68aa24ce3af8a517e3f47058111e07f86a",
 "transactionHash": "0x650a1dc1df920aaa5bf5288a8c7df86a77f8a1553d3bcaf93c150fd34abd9177"
}

Scanned the Embark code and don't understand where it might be going wrong. LGTM.

Another change that (outside-chance) could be relevant is #3119 - mostly because it directly impacts the formatting line here.

@nivida nivida added 1.x 1.0 related issues Discussion labels Dec 19, 2019
@michaelsbradleyjr
Copy link
Contributor Author

@cgewecke thanks for much for your quick attention on this. Given the E2E test you wrote and your follow-up re: the formatter, I'm fairly confident the problem is on Embark's side of things. If you share that confidence, feel free to close this issue. I'll continue to hunt the bug, and if it turns out to genuinely be a problem with web3 I'll follow-up here or in a new issue.

@cgewecke
Copy link
Collaborator

Ok, sounds good. I wonder if Embark was implicitly relying on Web3 reformatting the nonce in a way that stopped with #3119 - e.g now that the tx object is _.cloned, the modification doesn't 'stick' on Embark's side.

If you find anything else out, please ping - thanks @michaelsbradleyjr.

@michaelsbradleyjr
Copy link
Contributor Author

I found some problems in web3's signTransaction, will open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Discussion
Projects
None yet
Development

No branches or pull requests

3 participants