Closed frgtn closed 6 years ago
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?
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
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.
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.
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 https://github.com/ethereumjs/ethereumjs-tx/pull/94, this should make the various cases work again.
Ok, this https://github.com/ethereumjs/ethereumjs-tx/pull/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.
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.
@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.
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?
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 (https://github.com/ethereumjs/ethereumjs-tx/pull/110), will do a bug fix release with this right after.
Ok, just released v1.3.6 https://github.com/ethereumjs/ethereumjs-tx/releases/tag/v1.3.6 with the fix.
Ganache CLI v6.7.0 (ganache-core: 2.8.0)
same issue
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.
Change to how includeSignature default value is handled in
FakeTransaction.hash
method introduced in b49d3ca0e86a9b7c1f0b03f46fd9b7804da454c1 (#94) brokeganache-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 functionsecp256k1.sign
gets passed a private key consisting of0
s which causes the error.Full stack trace:
Downgrading to
ethereumjs-tx
version1.3.3
resolved the issue.I will provide a minimal example later.