ethereumjs / ethereumjs-tx

Project is in active development and has been moved to the EthereumJS VM monorepo.
https://github.com/ethereumjs/ethereumjs-vm/tree/master/packages/tx
Mozilla Public License 2.0
779 stars 235 forks source link

Self signed TXs doesn't work #113

Closed kipliklotrika closed 6 years ago

kipliklotrika commented 6 years ago

Self signed transactions doesn't work anymore.

It seems that this commit https://github.com/ethereumjs/ethereumjs-tx/commit/2dfdae9d7397e394488c206ac68990eccae4bafd broke that functionality. Particullary in this line:

this.from = data.from

This line will invoke setter of from property which will call Buffer.from(val), where val is undefined. In that case _from property will hold Buffer{0}. Later, this will have effect on:

  getSenderAddress () {
    if (this._from) {
      return this._from
    }
    const pubkey = this.getSenderPublicKey()
    this._from = ethUtil.publicToAddress(pubkey)
    return this._from
  }

from index.js. this._from will always be defined and no public key will be used.

holgerd77 commented 6 years ago

Hi @kipliklotrika, thanks for reporting, on a pre-note: just realized that our test coverage reporting for FakeTransaction (100%) is highly misleading.

Have opened an issue on this: https://github.com/ethereumjs/ethereumjs-tx/issues/114

holgerd77 commented 6 years ago

I am not completely getting the context of this, this line this.from = (data && data.from) ? data.from : '0x0000000000000000000000000000000000000000' was only out for a few couple of days with the v1.3.5 release directly followed by the v1.3.6 release switching back to the old version.

So this shouldn't have been worked before unless I overlook something. Any information on this?

holgerd77 commented 6 years ago

@kipliklotrika Which method did you originally call causing the error? verifySignature()?

kipliklotrika commented 6 years ago

I came upon the error by running self-signed transaction on the Ganache. Top of the stack trace:

Error: sender doesn't have enough funds to send tx. The upfront cost is: 4000000000000000 and the sender's account only has: 0
    at runCall (node_modules/ethereumjs-vm/dist/runTx.js:84:10)

In runTx.js function tries to check balance and invoke .from getter in:

var fromAccount = self.stateManager.cache.get(tx.from);

In mentioned case above, from will always be Buffer{0}.

I hope this description helps?

benjamincburns commented 6 years ago

@holgerd77 - just a heads up that this one caused me a bit of pain - had to do a late night re-release of ganache-core and ganache-cli once I realized caller-signed transactions were universally broken.

On the bright side it shined a light on a nasty flaw in our testing & dependency management. This issue wasn't visible to ganache-core's tests because its package-lock.json had us pinned to 1.3.4. Apparently npm doesn't respect transitive dependencies' package-lock.json files, as I bumped ganache-cli's package-lock.json prior to our last release and it picked up 1.3.6. I use zeppelin-solidity tests as a smoke test against the built version of ganache-cli and that all passed fine... little did I know...

Long story short, I'll be doing some work soon to make sure that ganache-cli uses the same exact packages as those which ganache-core uses.

holgerd77 commented 6 years ago

@benjamincburns Sorry for that. I'm just started looking deeper into this library and realized that it could generally be in a better shape. Will put some emphasis on this in the upcoming weeks but it will need some time until we are on a better ground here.

Do you have got a proposed fix for the issue described above? Still not 100% sure how best to handle this.

ajhodges commented 6 years ago

Pretty confident that I've addressed this issue in #118. Thanks to @kipliklotrika for finding this!

tonysparks5000 commented 5 years ago

His problem has made me feel like a crazy person!! Im using Ganache 2.0 gui and it seems like the problem is till here. should i switch to the cli? i just want to make sure my code works. properly test results .should i just use the ropsten test network?

alcuadrado commented 5 years ago

Hey @tonysparks5000. I'm sorry things aren't working as expected for you. Can you please provide more information about what's going wrong? Or better yet, can you open another issue? Thanks!