Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AVM: opcode for ed25519 verify of arbitrary messages. #3218

Merged
merged 2 commits into from
Mar 15, 2022

Conversation

jannotti
Copy link
Contributor

Brings ed25519 verify to feature parity with newly added ecdsa opcodes. Allows verification of any message, rather than requiring "program" and hash(code) prefix.

Same sort of testing as ed25519verify

@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2021

Codecov Report

Merging #3218 (d25367d) into master (eea0a75) will decrease coverage by 0.01%.
The diff coverage is 82.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3218      +/-   ##
==========================================
- Coverage   47.43%   47.42%   -0.02%     
==========================================
  Files         369      369              
  Lines       59494    59507      +13     
==========================================
- Hits        28221    28219       -2     
- Misses      27984    28000      +16     
+ Partials     3289     3288       -1     
Impacted Files Coverage Δ
data/transactions/logic/doc.go 77.41% <ø> (ø)
data/transactions/logic/opcodes.go 100.00% <ø> (ø)
data/transactions/logic/eval.go 85.67% <82.35%> (-0.05%) ⬇️
ledger/blockqueue.go 81.03% <0.00%> (-2.88%) ⬇️
network/wsPeer.go 65.83% <0.00%> (-2.23%) ⬇️
network/requestTracker.go 70.25% <0.00%> (-1.30%) ⬇️
data/abi/abi_type.go 92.03% <0.00%> (-1.00%) ⬇️
network/wsNetwork.go 62.84% <0.00%> (-0.50%) ⬇️
catchup/service.go 69.82% <0.00%> (+0.49%) ⬆️
ledger/acctupdates.go 65.96% <0.00%> (+0.95%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eea0a75...d25367d. Read the comment docs.

@tsachiherman
Copy link
Contributor

@jannotti could you hold off with this PR until after the batch verification get merged in ? There are some verification semantics that would be better off be uniform.

@jannotti
Copy link
Contributor Author

@jannotti could you hold off with this PR until after the batch verification get merged in ? There are some verification semantics that would be better off be uniform.

Sure, it's not time critical (belongs in AVM 1.1 and is gated by TEAL 6 in vFuture). But how can batch verification impact this? This is only for signature checking inside of AVM code. As long as there exists a crypto.SignatureVerifier.VerifyBytes type function (for verifying a single signature), this code will be unchanged.

pzbitskiy
pzbitskiy previously approved these changes Dec 3, 2021
Copy link
Contributor

@pzbitskiy pzbitskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good. Not sure about _bare naming, and the use case (remembering ed25519verify has such specific behavior because of avoiding some kind of misuses).

@jannotti
Copy link
Contributor Author

jannotti commented Feb 6, 2022

Now that we have the EnableBatchVerification consensus parameter, would it be reasonable to put this into the upcoming release? The call to func (v SignatureVerifier) VerifyBytes would be supplied the BatchVerify flag. Since it would release along with BatchVerification, there would be no backward compatibility issues.

@ryanRfox
Copy link
Contributor

ryanRfox commented Mar 7, 2022

Please consider an alternate to "bare" which may be more descriptive, such as "bytes" or "data".

@ryanRfox
Copy link
Contributor

ryanRfox commented Mar 7, 2022

@algorandskiy regarding misuse of this opcode, how and where do we inform developers of these risks and proper mitigation? OpCode description and/or Dev Portal docs?

@jannotti jannotti changed the title New opcode for ed25519 verifies of arbitrary messages. AVM: opcode for ed25519 verify of arbitrary messages. Mar 14, 2022
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good.

My only suggestion is that it would be nice to add a note in opDocExtras in either/both of the ed25519 opcode descriptions to describe how they differ, but that is not essential.

@jannotti jannotti merged commit 5978c3d into algorand:master Mar 15, 2022
@algojack algojack mentioned this pull request Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants