Skip to content
This repository has been archived by the owner on Apr 6, 2020. It is now read-only.

Self signed TXs doesn't work #113

Closed
kipliklotrika opened this issue Jul 4, 2018 · 9 comments
Closed

Self signed TXs doesn't work #113

kipliklotrika opened this issue Jul 4, 2018 · 9 comments

Comments

@kipliklotrika
Copy link

Self signed transactions doesn't work anymore.

It seems that this commit 2dfdae9 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
Copy link
Member

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: #114

@holgerd77
Copy link
Member

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
Copy link
Member

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

@kipliklotrika
Copy link
Author

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 added a commit to trufflesuite/ganache that referenced this issue Jul 9, 2018
@benjamincburns
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Contributor

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

mikeseese pushed a commit to trufflesuite/ganache that referenced this issue Sep 10, 2018
…162)

This pull request copies the build process from `ganache-cli` to `ganache-core` while also optimizing the resulting webpacked bundle to only include `ganache-core`'s internals plus pure-js implementations of our native transitive dependencies. All other dependencies are installed on `install`.

Additionally, ganache-core and ganache-cli will now default to using native dependencies, if successfully `install`ed, only falling back to the pure-js implementations if those dependencies can't be installed (i.e., because node-gyp on Windows is tricky).

---

This PR also purges some unnecessary `dependencies` from package.json, moves others to `devDependencies`, and updates and pins the remaining direct dependencies to their latest patch versions (at time of writing), with the exception of:

- `level-sublevel`; as version `6.6.5` needlessly regressed to a dependency on a vulnerable version of `levelup`. `6.6.5` actually looks like it was accidentally published from an older branch named `master2`. `¯\_(ツ)_/¯`
- `ethereumjs-tx`; as version [`1.3.5`](https://github.com/ethereumjs/ethereumjs-tx/releases/tag/v1.3.5) introduced bug ethereumjs/ethereumjs-tx#113 which was not fixed in [`1.3.7`](https://github.com/ethereumjs/ethereumjs-tx/releases/tag/v1.3.7) in my testing.

---

On `npm publish` our tests will automatically be run, the webpack build created, and the tests run again on the new webpacked build. We _may_ want to just trust that Travis will build and test everything for us, but having everything run on `publish` ensures that we don't ship an old or broken `./build` directory (since `./build` will *not* be created on `npm install`).

Our new `.npmignore` will ignore most of our unneeded files and directories on npm publish, but it will include `./lib` and `./build`.

Before `npm publish`ing to production we'll want to test the interaction of the new build steps, our `.npmignore`, and `./build` on a variety of OSes (mainly Windows in different configurations to ensure the non-native fallback method works appropriately), perhaps by temporarily publishing to npm under a different name.

---

With all that said and done, once axic/scrypt.js#3 is merged and published to npm we'll likely get rid of our node webpack build all together; currently, the only other consideration is `eth-block-tracker`, which is potentially node 6 incompatible, so we may still want to use its es5 build. _Note: `secp256k1` already automatically falls back to a pure-JS implementation if a native build isn't possible._
@tonysparks5000
Copy link

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
Copy link
Member

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!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants