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

Parsed tx.nonce returns empty value for nonce=0 #714

Closed
max-block opened this issue Jul 3, 2018 · 15 comments
Closed

Parsed tx.nonce returns empty value for nonce=0 #714

max-block opened this issue Jul 3, 2018 · 15 comments

Comments

@max-block
Copy link

There are two raw txs:

  • tx1 with nonce=0
  • tx2 with nonce=3

Let's parse them:

const Tx = require("ethereumjs-tx")
const tx1 = new Tx(
  "0xf86c808502cb417800827148948bc1779b4cb3fd0b33a3b2c129a1178f5b439ae688120a871cc00200008026a07b77ce4bc7d8b16a8b0fc2407262ffe4e365fefc7d49bb9777938628cdc2c1d2a00d01ec810908f69be06bfbe805dcf193804c2f5e4ca664b714436e046797f4e4"
)
const tx2 = new Tx(
  "0xf86c038502cb4178008271489448797ee0870a386c66dd8b0c5a233b01566be16288120a871cc00200008026a02428df35c47b2c5f671e31a65dc8f2ee287a805236c2a870e430f7811f15accea0293044614b92837228219ea7418b427c34aa2f6bfa2f1619f4d634a80d449338"
)
console.log(`tx1.nonce(${tx1.nonce.toString("hex")}), length=${tx1.nonce.length}`)
console.log(`tx2.nonce(${tx2.nonce.toString("hex")}), length=${tx2.nonce.length}`)

It prints:

tx1.nonce(), length=0
tx2.nonce(03), length=1

So for nonce=0 it returns empty value, but it should returns 0.

@max-block max-block changed the title Parsed tx.nonce returns "0x" (for nonce=0) Parsed tx.nonce returns empty value for nonce=0 Jul 3, 2018
@holgerd77
Copy link
Member

Hi @max7z, hmm, when I decode your first input string with the RLP library it already returns an empty buffer:

const ethUtil = require('ethereumjs-util')
ethUtil.rlp.decode("0xf86c808502cb417800827148948bc1779b4cb3fd0b33a3b2c129a1178f5b439ae688120a871cc00200008026a07b77ce4bc7d8b16a8b0fc2407262ffe4e365fefc7d49bb9777938628cdc2c1d2a00d01ec810908f69be06bfbe805dcf193804c2f5e4ca664b714436e046797f4e4")
[ <Buffer >,
  <Buffer 02 cb 41 78 00>,
  <Buffer 71 48>,
  <Buffer 8b c1 77 9b 4c b3 fd 0b 33 a3 b2 c1 29 a1 17 8f 5b 43 9a e6>,
  <Buffer 12 0a 87 1c c0 02 00 00>,
  <Buffer >,
  <Buffer 26>,
  <Buffer 7b 77 ce 4b c7 d8 b1 6a 8b 0f c2 40 72 62 ff e4 e3 65 fe fc 7d 49 bb 97 77 93 86 28 cd c2 c1 d2>,
  <Buffer 0d 01 ec 81 09 08 f6 9b e0 6b fb e8 05 dc f1 93 80 4c 2f 5e 4c a6 64 b7 14 43 6e 04 67 97 f4 e4> ]

Where did you get these tx strings in the first hand? Are you sure they are correctly encoded? Otherwise this might be (somewhat unlikely) a bug in the RLP library.

@max-block
Copy link
Author

max-block commented Jul 5, 2018

@holgerd77 I've already lost that tx, but I've created a single file with a new tx where I show all steps how I get this empty value for nonce field.

const { BigNumber } = require("bignumber.js")
const Tx = require("ethereumjs-tx")

// First let's create tx from such input parameters
const from = "0xf721a58f9b5ee1e1fcd54fcdcc763c04c842b1f8"
const privateKey = "be028c274b85d9bb17085f5476da75164f6855ec3f7c15ad39a8cee4df3f0b23"
const to = "0x2c43899c3406ecaca88bebefd741cfba163d32c3"
const gasPrice = "0x" + new BigNumber("30000000000").toString(16)
const gasLimit = "0x" + new BigNumber(50000).toString(16)
const value = "0x" + new BigNumber("10000000000000000").toString(16)
const nonce = "0x" + new BigNumber(0).toString(16)

const txParams = {
  from,
  to,
  nonce,
  gasPrice,
  gasLimit,
  chainId: 3
}
console.log({ txParams })
/* from console:
{ txParams:
   { from: '0xf721a58f9b5ee1e1fcd54fcdcc763c04c842b1f8',
     to: '0x2c43899c3406ecaca88bebefd741cfba163d32c3',
     nonce: '0x0',
     gasPrice: '0x6fc23ac00',
     gasLimit: '0xc350',
     chainId: 3 } }
*/

const tx = new Tx(txParams)
tx.sign(Buffer.from(privateKey, "hex"))
const serializedTx = tx.serialize()
const rawTx = "0x" + serializedTx.toString("hex")

console.log({ rawTx })
// from console:
// 0xf864808506fc23ac0082c350942c43899c3406ecaca88bebefd741cfba163d32c3808029a0c4f1203f34c57e0ab8cdc964297fa2a00297ecdc4593b65650630dccadc530b5a066d43f46db647115bb43c91542e2e01d7b596432812fa309980c6f25d90cb06e

// I've broadcasted this tx to ropsten: https://ropsten.etherscan.io/tx/0xf7a3ad995a3e80f1349a57edec1cabfc542f4a53a6d18e38db6b387bab83b322

// Now let's parse this this raw tx back and check nonce field:
const parsedTx = new Tx(
  "0xf864808506fc23ac0082c350942c43899c3406ecaca88bebefd741cfba163d32c3808029a0c4f1203f34c57e0ab8cdc964297fa2a00297ecdc4593b65650630dccadc530b5a066d43f46db647115bb43c91542e2e01d7b596432812fa309980c6f25d90cb06e"
)
console.log(`nonce(${parsedTx.nonce.toString("hex")}), length=${parsedTx.nonce.length}`)
// nonce(), length=0

// returns nonce as empty value, but it should be 0

@holgerd77
Copy link
Member

Hi @max7z, just tested this. It is indeed the case that the transaction library initializes zero-values as an empty (and not as a zero-) buffer. So both 0x00 and 0x as an input become <Buffer > in the transaction.

I am getting a bit shaky about this if this is a bug or something intended or made by design. Did some googling on zero value representation in Ethereum, stumbled e.g. on this (see point 3)).

On a technical side this roots back to https://github.com/ethereumjs/ethereumjs-util/issues/141, just opened the according issue.

Some background question on the concrete issue you have: do you have concrete problems on this / is this causing unexpected behavior on some end or were you just wondering about the output?

@max-block
Copy link
Author

Yes, I have a problem in my private projects. There is an interface for analyzing broadcasted transactions via parsing rawTx to structs.

Here is code:

export const parseTx = (rawTx: string) => {
  const tx = new Tx(rawTx)
  return {
    value: new BigNumber(bufferToHex(tx.value)).toString(10),
    gasLimit: new BigNumber(bufferToHex(tx.gasLimit)).toString(10),
    gasPrice: new BigNumber(bufferToHex(tx.gasPrice)).toString(10),
    nonce: new BigNumber(bufferToHex(tx.nonce)).toString(10),
    from: bufferToHex(tx.from),
    to: bufferToHex(tx.to),
    data: bufferToHex(tx.data)
  }
}

And when nonce=0, new BigNumber(bufferToHex(tx.nonce)).toString(10) returns NaN instead of 0. Now I have workaround for it, but I think that 0 and empty value are very different values. And nonce=0 is a valid state for tx and I find it confusing when it returns empty Buffer for valid nonce=0.

@holgerd77
Copy link
Member

Hi @max7z, thanks for the extensive answer. My first impression is that you are right on these zero values. I am relatively new in maintaining this library and I want to investigate this further before doing any changes, since this is a sensitive library.

I think there is a general need for an overhaul, the tests of the tx library are also pretty outdated. To manage expectations a bit, this will take some time and not be fixed in the coming 2-3 weeks. Promise to stay on this though, but I would like to get the testing right before to not introduce new errors here.

@davidmurdoch
Copy link
Contributor

Running into this issue in ganache-core as well. nonce is a Quantity field (at least in the context of a Transaction?) and 0 is an acceptable value here.

@holgerd77, let me know if you need anything from me to get this issue fixed and the tests updated and more robust! I'll gladly help out where I can.

@holgerd77
Copy link
Member

Yes, we should really make this super-high-priority. We actually have a problem with 0 values, this is touching ethereumjs-util https://github.com/ethereumjs/ethereumjs-util/issues/141 and the RLP library ethereumjs/rlp#28 as well.

@danjm is already working on updating the tests here ethereumjs/ethereumjs-tx#126. Dan, can we work on this together with the goal of having this fixed before Christmas?

@davidmurdoch Sorry, really not possible to fix this quicker (at least that's my estimation). This needs some really thorough analysis and side-effect thought to get this right, since this is touching core functionality on various sensitive libraries.

@danjm
Copy link
Contributor

danjm commented Nov 29, 2018

@holgerd77 Sounds good to me. I'll finish the test update PR later today, and then investigate this specific issue more thoroughly.

@davidmurdoch
Copy link
Contributor

@holgerd77, I understand the potential side-effect issues and appreciate your thoroughness in the fix.

For now I'm just redefining the nonce field as follows:

class Transaction extends EthereumJsTransaction {
  constructor(data) {
    super(data);

    // ethereumjs-tx doesn't allow for a `0` value in a nonce, but we want it to.
    // once https://github.com/ethereumjs/ethereumjs-tx/issues/112 is fixed we can remove this
    const index = this._fields.indexOf("nonce");
    const nonceFieldLength = 32;
    Object.defineProperty(this, "nonce", {
      enumerable: true,
      configurable: true,
      get: () => this.raw[index],
      set: (v) => {
        v = utils.toBuffer(v); // TODO: trim all but the last `0` from v
        assert(nonceFieldLength >= v.length, "The field nonce must not have more " + nonceFieldLength + " bytes");
        this.raw[index] = v;
      }
    });
  }
}

Again, let me know if you need any assistance.

@davidmurdoch
Copy link
Contributor

@holgerd77, @danjm See my comments about stripZeros here: https://github.com/ethereumjs/ethereumjs-util/issues/141#issuecomment-443534037

TL;DR: in the context of a JSON-RPC hex encoded value, stripping all zeros from a QUANTITY field is incorrect, in the context of a transaction that is being signed it is correct. How does this affect property lookup (e.g., tx.nonce) vs the same value hex encoded via toJSON?

I think a big question here is: should ethereumjs-tx have the ability to differentiate between a null nonce (and all the other fields, actually) and a 0 nonce. And if it shouldn't differentiate between the two in a Transaction, should it in a FakeTransaction?

@Fang-
Copy link
Contributor

Fang- commented Mar 28, 2019

Ran into this today. Is this still being worked on?

@danjm
Copy link
Contributor

danjm commented Apr 1, 2019

@Fang- Yes, this is still being worked on. I've just posted some very preliminary work to the discussion here: https://github.com/ethereumjs/ethereumjs-util/issues/141

I am hoping to resolve this issue over the next 7 days.

@holgerd77
Copy link
Member

Hi @ryanio, can you have a look along your work on #812 if this is still an issue?

@holgerd77
Copy link
Member

Update: depending on the answer here please also have a short look on the state of #712, since it was suggested in the last comment that the issues might be connected. Thanks!

@ryanio
Copy link
Contributor

ryanio commented Sep 15, 2020

This should be resolved with #812 since nonce will be stored as a BN instead of a Buffer, and stripZeroes is no longer used.

@ryanio ryanio mentioned this issue Sep 15, 2020
@ryanio ryanio closed this as completed Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants