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

Add keccak-secp256k1 instruction #11839

Merged
merged 2 commits into from
Sep 16, 2020
Merged

Conversation

sakridge
Copy link
Member

Problem

Performing keccak+secp256k1 signature check on solana would require too many bpf instructions.

Summary of Changes

Add builtin instruction that performs a hash+sigverify on the instruction data.

Fixes #

@sakridge sakridge force-pushed the sepc256k1 branch 4 times, most recently from c698d47 to eb3ea51 Compare August 25, 2020 22:12
@codecov
Copy link

codecov bot commented Aug 25, 2020

Codecov Report

Merging #11839 into master will increase coverage by 0.0%.
The diff coverage is 89.3%.

@@           Coverage Diff            @@
##           master   #11839    +/-   ##
========================================
  Coverage    82.0%    82.0%            
========================================
  Files         338      341     +3     
  Lines       79640    79890   +250     
========================================
+ Hits        65342    65579   +237     
- Misses      14298    14311    +13     

@sakridge sakridge force-pushed the sepc256k1 branch 12 times, most recently from 9a65d24 to a9f586f Compare August 26, 2020 23:03
@sakridge
Copy link
Member Author

I'm not finalized on the construction of the account data, but is this how you were thinking @jackcmay

@jackcmay
Copy link
Contributor

Looking good so far

@sakridge sakridge force-pushed the sepc256k1 branch 5 times, most recently from 2942daf to bfa9359 Compare August 31, 2020 16:37
@sakridge sakridge marked this pull request as ready for review September 12, 2020 16:57
return Err(TransactionError::AccountNotFound);
}
let program_id = &self.message().account_keys[instruction.program_id_index as usize];
if crate::secp256k1_program::check_id(program_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We will probably have more of these, could generalize the verify structure to accommodate additions in the future

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't find a way to do like a match Pubkey, so I think I'll punt on this for now, although it does seem worthwhile to have some kind of framework here. I might be the one adding the next one anyway...

@jackcmay
Copy link
Contributor

Is the BPF program usage of these results coming in a followup PR?

@sakridge
Copy link
Member Author

Is the BPF program usage of these results coming in a followup PR?

Well, the BPF program will use the instruction introspection part that I'm working on to then inspect the transaction and see that the correct instructions are there to do the secp verify. I split that into this PR: #11943

Seemed easier to do them separately so not as much complexity at once.

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

Mostly nits here. It'd be great to find something cleaner for the FeeCalculator changes, it can wait if we're crunched though

sdk/src/fee_calculator.rs Outdated Show resolved Hide resolved
sdk/src/secp256k1.rs Outdated Show resolved Hide resolved
sdk/src/transaction.rs Outdated Show resolved Hide resolved
sdk/src/transaction.rs Show resolved Hide resolved
sdk/src/secp256k1.rs Outdated Show resolved Hide resolved
@sakridge sakridge force-pushed the sepc256k1 branch 8 times, most recently from 26ea2ef to 0278454 Compare September 15, 2020 22:25
Verifies eth addreses with ecrecover function
@sakridge sakridge merged commit 3930cb8 into solana-labs:master Sep 16, 2020
@sakridge sakridge deleted the sepc256k1 branch September 16, 2020 01:23
@sakridge sakridge added the v1.3 label Sep 21, 2020
mergify bot pushed a commit that referenced this pull request Sep 21, 2020
* Implement keccak-secp256k1 instruction

Verifies eth addreses with ecrecover function

* Move secp256k1 test

(cherry picked from commit 3930cb8)

# Conflicts:
#	Cargo.lock
#	runtime/Cargo.toml
sakridge added a commit that referenced this pull request Sep 21, 2020
* Implement keccak-secp256k1 instruction

Verifies eth addreses with ecrecover function

* Move secp256k1 test
mergify bot added a commit that referenced this pull request Sep 21, 2020
* Implement keccak-secp256k1 instruction

Verifies eth addreses with ecrecover function

* Move secp256k1 test

Co-authored-by: sakridge <sakridge@gmail.com>

// Parse out signature
let signature_index = offsets.signature_instruction_index as usize;
if signature_index > instruction_datas.len() {
Copy link
Member

Choose a reason for hiding this comment

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

😭 #13014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants