crypto: add a couple of tests for ed25519 signatures before/after verification #3783
Conversation
cce
force-pushed
the
weird-ed25519-signatures
branch
from
56f0852 to
e50e8a7
Compare
id-ms
left a comment
id-ms
left a comment
There was a problem hiding this comment.
From my understanding, the purpose of those tests is to check ed25519verify TEAL opcode under specially crafted public keys, right?
if so, what is the difference between those two tests? (I do understand that they have different logic, but this is not the focus of the test)
Is there a way to break one test without the other?
jannotti
left a comment
jannotti
left a comment
There was a problem hiding this comment.
Seems good, willing to merge as is, let me know if you want to make any of the changes I mentioned. (the sb trace is probably the most worthwhile)
| var txn transactions.SignedTxn | ||
| txn.Lsig.Logic = ops.Program | ||
| txn.Lsig.Args = [][]byte{data[:], vec[2], vec[1]} | ||
| sb := strings.Builder{} |
There was a problem hiding this comment.
I don't see where you're putting sb in to trace. But you shouldn't have to anyway. defaultEvalParams inserts a string.Builder for you, so just change to evalParams.Trace.
| evalParams.Proto.EnableBatchVerification = tc.enableBatchVerification | ||
| pass, err := EvalSignature(0, evalParams) | ||
| require.Equal(t, tc.valid, pass, "trace %s", sb.String()) | ||
| t.Log(sb.String()) |
There was a problem hiding this comment.
It would be nice to Log the trace only if the test is about to fail, so the output is less cluttered when something else fails.
| (== (txn Amount) 2000000)) | ||
| ) | ||
| */ | ||
| ops, err := AssembleStringWithVersion(`arg 0 |
There was a problem hiding this comment.
I like to use testProg for assembling in tests. It checks the error for you and does not return it, it disassembles and reassembles to check equality giving us more tests for free, and does some logging if things don't work out.
There is no difference between the tests from the POV of the ed25519verify opcode operation, in the second one I was working on a transaction that would result in an account balance change, but it is not necessary if we want to just assert the new ed25519 behavior. |
|
Unnecessary after #3781 merged |
3 participants
Summary
Now that the consensus upgrade for #3031 is complete this pushes a couple of unit tests related to signatures that were not valid before the batch verification upgrade, but are valid now.
Related: #3781
Test Plan
Adds two new tests.