Fix transaction potentially rejected for the wrong reasons (#912)

[norswap]

cf. #912

Questions: what should I put in the filledwith, lllcversion and sourceHash fields of the filler?

I haven't validated this against a running node at the moment, but if it's convenient, I might set one up and do it.

Not sure if immensely useful, but here is the script I used to generate the new values.

[winsvega]

To generate this tests we need to use old testeth that can still do it.

However I remember the rule of s signature validation was implemented there. There for the whole issue must be investigated because the tests must be correct

[winsvega]

Need to check with testeth
Is there really a bug?

[norswap]

It's really not that hard to see:
EIP-2:

All transaction signatures whose s-value is greater than secp256k1n/2 are now considered invalid. The ECDSA recover precompiled contract remains unchanged and will keep accepting high s-values; this is useful e.g. if a contract recovers old Bitcoin signatures.

https://en.bitcoin.it/wiki/Secp256k1:

n = FFFFFFFF FFFFFFFF FFFFFFFF FFFFFFFE BAAEDCE6 AF48A03B BFD25E8C D0364141

in src/TransactionTestsFiller/ttData/DataTestNotEnoughGASFiller.json:

"s" : "0xefffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804"

Together:

FFFFFFFF FFFFFFFF FFFFFFFF FFFFFFFE BAAEDCE6 AF48A03B BFD25E8C D0364141
efffd310 ac743f37 1de3b9f7 f9cb56c0 b28ad436 01b4ab94 9f53faa0 7bd2c804

Obviously, anything whose high-order hex digit is E is going to be more than half of a number whose high-order hex digit (in a string of the same length) is F.

Hence s > n/2, hence the signature is invalid starting from Homestead.

Hence the transaction will get rejected on Homestead+, indepently of the thing that is supposed to be tested (e.g. "not enough gas").

In a certain sense, the tests are not bugged, because they will always succeed on a correct implementation. But they're not testing what they are supposed to be testing.

[winsvega]

just checked this file DataTestNotEnoughGASFiller.json and filled DataTestNotEnoughGAS
the test is correct with s/2 requirements, what is the issue?

[winsvega]

but you right that it could be rejected to a wrong reason than assumed by test.
I talked to Martin, we think we can revive thet transaction suite test to cover trasaction structure

[winsvega]

fixed with reworked transaction tests