includeSignature change breaks ganache-core contract deployment

[frgtn]

Change to how includeSignature default value is handled in FakeTransaction.hash method introduced in b49d3ca (#94) broke ganache-core contract deployment.

Contract deployment now fails with: Error: nonce generation function failed or private key is invalid.

The problem is that previously FakeTransaction attempted to generate the signature only if this._from was set. Now it attempts to set the signature regardless of it being present. Later down the line function secp256k1.sign gets passed a private key consisting of 0s which causes the error.

Full stack trace:

Uncaught Error: nonce generation function failed or private key is invalid
      at Object.exports.ecsign (node_modules/ethereumjs-util/dist/index.js:371:23)
      at FakeTransaction.sign (node_modules/ethereumjs-tx/index.js:233:25)
      at FakeTransaction.hash (node_modules/ethereumjs-tx/fake.js:66:12)
      at /opt/app/node_modules/ganache-core/lib/statemanager.js:614:29
      at self.blockchain.getQueuedNonce (node_modules/ganache-core/lib/statemanager.js:843:7)
      at /opt/app/node_modules/ganache-core/lib/blockchain_double.js:377:5
      at /opt/app/node_modules/merkle-patricia-tree/baseTrie.js:77:5
      at /opt/app/node_modules/merkle-patricia-tree/baseTrie.js:461:14
      at /opt/app/node_modules/async/lib/async.js:52:16
      at done (node_modules/async/lib/async.js:246:17)
      at /opt/app/node_modules/async/lib/async.js:44:16
      at Object.return (node_modules/merkle-patricia-tree/baseTrie.js:485:9)
      at processNode (node_modules/merkle-patricia-tree/baseTrie.js:276:32)
      at processNode (node_modules/merkle-patricia-tree/baseTrie.js:521:5)
      at /opt/app/node_modules/merkle-patricia-tree/baseTrie.js:503:15
      at /opt/app/node_modules/merkle-patricia-tree/baseTrie.js:180:7
      at _combinedTickCallback (internal/process/next_tick.js:131:7)
      at process._tickCallback (internal/process/next_tick.js:180:9)

Downgrading to ethereumjs-tx version 1.3.3 resolved the issue.

I will provide a minimal example later.

[holgerd77]

Ah yes, tested this locally and could reproduce the error. Sorry for that, merged this too thoughless since this was coming from the MetaMask side so I somehow assumed that this should be safe.

What would you suggest? Would it be a way to just use a random different private key for fake tx signing when from is set to the zero address? Or should this in your opinion be completely reverted?

[frgtn]

I think that removal of && this._from check before attempting to generate a signature could've been accidental while cleaning up codebase, so my suggestion would be to reintroduce it. Rationale behind the thinking is that the signature can't be generated without this._from anyway.

What you're suggesting could also work because the key used to generate a signature was constructed from the this._from by concatenating part of it to itself, so it didn't make any sense but was consistent. Generating a random private key would break this consistency, but I highly doubt that any code relies on it when it doesn't provide a from address. But I'm fairly new to Ethereum ecosystem so I might be wrong, please double-check :)

cc @Artazor

[frgtn]

Added a failing test case for this in #109 as well as a trivial fix by setting default from address to 0x0000000000000000000000000000000000000001 instead of zero. This makes secp256k1 stop complaining about invalid key. Not sure what implications for this are though.

[holgerd77]

Hmm, I think on a second though this is relatively clear: if a combination of includeSignature = true and and a not-set from is used, the function should simply throw an exception, since this is non-defined/allowed behavior.

Will prepare a PR on this.

[holgerd77]

Hmm, hmm 😄, another take on this after a third and fourth thought: things are a bit more complex and also a bit tricky. Since these changes were introduced in minor releases I think it gets to harsh with throwing an exception on this and this will probably cause unnecessary trouble for other libraries. And maybe this is also not necessary considering the fake nature of this.

I think I will just revert this PR #94, this should make the various cases work again.

[holgerd77]

Ok, this #110 would be my suggestion for a fix, I think this should be the most non-intrusive way balancing a bit the needs of existing libraries and the already done API changes.

[LogvinovLeon]

We've bumped into that because ganache-core was generating the same transaction hash no matter what was the from address. Because it was calling tx.hash() without the parameter assuming the documented behavior instead of the actual one. If this new version fixes that - I'm OK with it. The only thing I'd like to mention is that those changing are breaking and it would probably we safer to introduce them in a major version bump so that people with the caret in their package.json don't get them by accident.

[holgerd77]

@LogvinovLeon Yes, thanks, I also thought longer about this this morning. I will then go the complete way and undo both PR changes from v1.3.5, deprecated this package, release as a new version v1.3.6 and release the default value change as a new v1.4.0 release. This might cause a bit in-between hazzle with the breaking release already out but on-mid term definitely the cleanest way.

[frgtn]

Thanks for taking care of this! I agree that it would be best to release the default value change in 1.4.0.

One thought about this - current default value implementation where it just sets the from address to 0x0 kind of hides the problem - it crops up only when faketx.hash is called and the error is quite cryptic and hard to figure out. Maybe it would be better to instead throw an error in fake.tx hash if from was not set indicating that from address is required? It would make much easier to debug failing tests for packages that try to ugprade to 1.4.0.

If from becomes mandatory forFakeTx, then it would be even better to throw in the constructor. What do you think?

[holgerd77]

a) One last last (hopefully :-)) thing on the default value: after thinking about this over the weekend and have another look I think we can leave this in the 1.3.x version range. This was inconsistent behavior before: When FakeTransaction.hash() was called in the old version, includeSignature was regarded as false but passed on to Transaction.hash() where this was taken as true (when being undefined). This is just inconsistent behavior and so this is more of a bug fix then a default value change. So I'll leave this.

I just merged the revertion of #94 (#110), will do a bug fix release with this right after.

[holgerd77]

Ok, just released v1.3.6 https://github.com/ethereumjs/ethereumjs-tx/releases/tag/v1.3.6 with the fix.

[fastchain]

Ganache CLI v6.7.0 (ganache-core: 2.8.0)

same issue

[alcuadrado]

I think you should report this in https://github.com/trufflesuite/ganache-core @fastchain, as it's using an old version of this library, and upgrading it probably fixes it.