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

Missing non-bpf alternative for create_program_address #11458

Closed
hendrikhofstadt opened this issue Aug 7, 2020 · 18 comments
Closed

Missing non-bpf alternative for create_program_address #11458

hendrikhofstadt opened this issue Aug 7, 2020 · 18 comments

Comments

@hendrikhofstadt
Copy link
Contributor

Problem

In 1.3.0 create_program_address was migrated to a syscall. However there is no non-syscall fallback for tests.

@jackcmay
Copy link
Contributor

jackcmay commented Aug 7, 2020

Good point, you are talking about here?
https://github.com/solana-labs/solana/blob/master/sdk/src/program_stubs.rs

@hendrikhofstadt
Copy link
Contributor Author

I actually also need to access it from the CLI code so I would appreciate if it was available outside of the program feature flag.

@jackcmay
Copy link
Contributor

jackcmay commented Aug 7, 2020

Yeah, I'm looking at that exact thing, easier said than done

@jackcmay
Copy link
Contributor

jackcmay commented Aug 7, 2020

Why does you CLI define program?

@hendrikhofstadt
Copy link
Contributor Author

The CLI does not but I still need access to the derivation

@jackcmay
Copy link
Contributor

jackcmay commented Aug 7, 2020

Sorry, confused, if CLi doesn't define the program feature why can't you call it directly?

@hendrikhofstadt
Copy link
Contributor Author

The CLI uses predefined derivation methods from the program which uses the syscall function.

@mvines
Copy link
Member

mvines commented Aug 7, 2020

@jackcmay, I found it weird that there's Pubkey::create_program_address() for non-programs and then program::create_program_address for (currently only) bpf programs.

solana/sdk/src/pubkey.rs

Lines 94 to 116 in 7e25130

pub fn create_program_address(
seeds: &[&[u8]],
program_id: &Pubkey,
) -> Result<Pubkey, PubkeyError> {
let mut hasher = Hasher::default();
for seed in seeds.iter() {
if seed.len() > MAX_SEED_LEN {
return Err(PubkeyError::MaxSeedLengthExceeded);
}
hasher.hash(seed);
}
hasher.hashv(&[program_id.as_ref(), "ProgramDerivedAddress".as_ref()]);
let hash = hasher.result();
if curve25519_dalek::edwards::CompressedEdwardsY::from_slice(hash.as_ref())
.decompress()
.is_some()
{
return Err(PubkeyError::InvalidSeeds);
}
Ok(Pubkey::new(hash.as_ref()))
}

@jackcmay
Copy link
Contributor

jackcmay commented Aug 7, 2020

@hendrikhofstadt Do those pre-defined derivation calls need to be behind the program feature in your program crate?

@mvines program::crate_program_address is only intended to be called from within the BPF sandbox by a program and whose purpose is to create an unresolved symbol in the shared object that gets resolved at load time. `Pubkey::create_program_address() is should not be called directly from a program due to it's computational complexity and dependencies. Did you have something else in mind?

@mvines
Copy link
Member

mvines commented Aug 7, 2020

@mvines program::crate_program_address is only intended to be called from within the BPF sandbox by a program and whose purpose is to create an unresolved symbol in the shared object that gets resolved at load time. `Pubkey::create_program_address() is should not be called directly from a program due to it's computational complexity and dependencies. Did you have something else in mind?

I was just confused to find basically same function in two locations.

@hendrikhofstadt
Copy link
Contributor Author

@jackcmay They don't because I want to share them between the CLI and the program.

Ideally they would resolve to the syscall in bpf and the Pubkey:: function in the CLI.
I could implement that myself with flags but would prefer if that was done right in the SDK.

@jackcmay
Copy link
Contributor

jackcmay commented Aug 7, 2020

Same signature, different function :-). One is just a wrapper

@jackcmay
Copy link
Contributor

jackcmay commented Aug 7, 2020

@hendrikhofstadt What do you see as the right way in the SDK?

In your program you can do:

    #[cfg(not(feature = "program"))]
    use solana_sdk::pubkey::create_program_address
    #[cfg(feature = "program")]
    use solana_sdk::program::create_program_address

Are you suggesting putting the stub also in pubkey.rs?

@hendrikhofstadt
Copy link
Contributor Author

hendrikhofstadt commented Aug 7, 2020

From a developer experience standpoint I think it would be more intuitive to have the function in just one place using a target config flag to pick the implementation as you proposed.

Generally I would expect that to be the case for any function that has a faster syscall based implementation.

That way a new developer will write more efficient code by default.

@jackcmay
Copy link
Contributor

jackcmay commented Aug 7, 2020

@mvines @hendrikhofstadt

wdyt: #11460

@hendrikhofstadt
Copy link
Contributor Author

hendrikhofstadt commented Aug 8, 2020

@jackcmay Actually I think that syscall should rather do find_program_address or there should be a separate syscall.

The fact of having to calculate a nonce in addition to the seeds makes derived keys unusable for k->v mappings because there are now n (nonce) mappings for k->v.

I do understand that this negatively affects performance. At the same time why must derived keys not lie on the curve? Shouldn't the security of the curve be enough to guarantee that there will not be any collisions?

@jackcmay
Copy link
Contributor

Program addresses are never meant to be signable by an outside entity so we split the domain so that wasn't possible.

find_program_address is a nicety to search for a valid program address, this could be done directly by your program using the create_program_address. How about we continue this topic on discord.

What do you think about closing this issue as part of #11460 given the problem description?

@hendrikhofstadt
Copy link
Contributor Author

@jackcmay agree. Let's continue on discord

Fixed by #11460

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

No branches or pull requests

3 participants