[FVM] signature verification refactoring #2171
Conversation
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit 99d9259 The command Bench tests were run a total of 7 times on each branch. Results
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov Report
@@ Coverage Diff @@
## master #2171 +/- ##
==========================================
- Coverage 57.47% 56.14% -1.33%
==========================================
Files 645 653 +8
Lines 38428 39911 +1483
==========================================
+ Hits 22085 22409 +324
- Misses 13530 14630 +1100
- Partials 2813 2872 +59
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| observedSigs := make(map[string]bool) | ||
| type uniqueKey struct { | ||
| address flow.Address | ||
| index uint64 | ||
| } | ||
| observedSigs := make(map[uniqueKey]bool) | ||
| for _, sig := range append(tx.PayloadSignatures, tx.EnvelopeSignatures...) { | ||
| keyStr := sig.UniqueKeyString() | ||
| if observedSigs[keyStr] { | ||
| if observedSigs[uniqueKey{sig.Address, sig.KeyIndex}] { | ||
| return DuplicatedSignatureError{Address: sig.Address, KeyIndex: sig.KeyIndex} | ||
| } | ||
| observedSigs[keyStr] = true | ||
| observedSigs[uniqueKey{sig.Address, sig.KeyIndex}] = true |
There was a problem hiding this comment.
I assume this change is mostly for performance reasons, could you please check we have tests for this?
There was a problem hiding this comment.
Yes, it is for performance. Here are the tests https://github.com/onflow/flow-go/blob/master/fvm/transactionVerifier_test.go#L43-L68
| const UserTagString = "FLOW-V0.0-user" | ||
|
|
There was a problem hiding this comment.
this might be in use by go-sdk or others ?
There was a problem hiding this comment.
It is in use, this is a breaking change for the flow-go-sdk/examples. We should keep that in mind.
There was a problem hiding this comment.
I'll update the sdk too after merging this PR: onflow/flow-go-sdk#273
| return false, errors.NewUnknownFailure(fmt.Errorf( | ||
| hashAlgo.String(), "is not supported in transactions")) | ||
| } |
There was a problem hiding this comment.
not sure about this, you mean if there is some problematic old keys we would stop the network? I think we should just error and add a watch on the error type.
There was a problem hiding this comment.
I tagged you about this here. I suggested to panic, Janez suggested the Failure error. As mentioned in the comment, that case can only happen if there is a bug in the code. These keys are all account keys, they are supposed to be checked and added previously, and they are not supposed to be problematic.
What do you suggest to use here?
| if pk.Algorithm() != crypto.ECDSAP256 && pk.Algorithm() != crypto.ECDSASecp256k1 { | ||
| // TODO: check if we should panic | ||
| // This case only happens in production if there is a bug |
There was a problem hiding this comment.
we should probably run some analysis on the mainnet data to verify all data are okey
There was a problem hiding this comment.
These are all keys already checked and added using AddAccountKey, so this check is only a sanity check, and if it fails then we have am internal bug somewhere.
This is the same as above. I suggested to panic and Janez suggest Failure error. What do you suggest?
| const UserTagString = "FLOW-V0.0-user" | ||
|
|
There was a problem hiding this comment.
It is in use, this is a breaking change for the flow-go-sdk/examples. We should keep that in mind.
|
bors merge |
SignatureVerifierinterface to verify signatures.SignWithTagunable to sign other that the transaction tag for safety.fmt.Sprintffor key deduplication (performance)