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 SystemInstruction::CreateAccountWithSeed #7390

Merged
merged 5 commits into from Dec 12, 2019

Conversation

@rob-solana
Copy link
Contributor

rob-solana commented Dec 10, 2019

Problem

support for multiple accounts with the same authority exists, but no provision for
creating accounts with addresses derived from the authority exists. it'd be really nice to define a space of addresses reachable for create_account() that don't require generating an ephemeral keypair.

Summary of Changes

add a system instruction that allows signers to create and assign in a subset of the
pubkey address space as derived addresses

Fixes #

@rob-solana rob-solana requested review from garious, aeyakovenko and t-nelson Dec 10, 2019
Copy link
Member

aeyakovenko left a comment

Maybe something to consider in the future - Transactions could encode a seed vector as well, then we could verify signatures for seed addresses.

return Err(InstructionError::MissingRequiredSignature);
}

// re-derive the address, must match to

This comment has been minimized.

Copy link
@mvines

mvines Dec 10, 2019

Member
Suggested change
// re-derive the address, must match to
// re-derive the address, must match `to`
space: u64,
program_id: &Pubkey,
) -> Result<(), InstructionError> {
// from is the source of the derived address, the caller must have

This comment has been minimized.

Copy link
@mvines

mvines Dec 10, 2019

Member
Suggested change
// from is the source of the derived address, the caller must have
// `from` is the source of the derived address, the caller must have
/// * lamports - number of lamports to transfer to the new account
/// * space - memory to allocate if greater then zero
/// * program_id - the program id of the new account
CreateAccountWithSeed {

This comment has been minimized.

Copy link
@mvines

mvines Dec 10, 2019

Member

We'll need to adjust solana-web3.js once this lands, do you want a hand with that @rob-solana?

This comment has been minimized.

Copy link
@rob-solana

rob-solana Dec 10, 2019

Author Contributor

lemme take a crack first

This comment has been minimized.

Copy link
@rob-solana

rob-solana Dec 10, 2019

Author Contributor
Copy link
Contributor

t-nelson left a comment

Looks good! Just the one design Q

fn create_account_with_seed(
from: &mut KeyedAccount,
to: &mut KeyedAccount,
seed: &str,

This comment has been minimized.

Copy link
@t-nelson

t-nelson Dec 10, 2019

Contributor

Any reason for the restriction to 32B ASCII string? [u8; 32] could support some internationalization and hash digests

This comment has been minimized.

Copy link
@mvines

mvines Dec 10, 2019

Member

yeah good point, we don't care that the seed is valid utf-8

This comment has been minimized.

Copy link
@rob-solana

rob-solana Dec 10, 2019

Author Contributor

I wanted to keep the space smaller than 256 bytes of seed to deter grinding attacks. 32 ascii chars takes away 32 bits.

This comment has been minimized.

Copy link
@aeyakovenko

aeyakovenko Dec 10, 2019

Member

@rob-solana grinding on what though?

This comment has been minimized.

Copy link
@rob-solana

rob-solana Dec 10, 2019

Author Contributor

trying to DoS a guy you think wants his authorized staker+"shrill conveyance 23/50"

This comment has been minimized.

Copy link
@aeyakovenko

aeyakovenko Dec 10, 2019

Member

@rob-solana you can always grind with pubkey generation. I think we should either have a Hash/[u8;32], or u64.

This comment has been minimized.

Copy link
@t-nelson

t-nelson Dec 10, 2019

Contributor

Requiring from's signature effectively makes griefing the address space a second-preimage attack on SHA256. We should probably be OK with that. Though fewer bits do minimize opportunities for shenanigans. Would it make sense to restrict the seed to something as small as a u16?

This comment has been minimized.

Copy link
@rob-solana

rob-solana Dec 10, 2019

Author Contributor

were it as small as a u16, mnemonics get harder. ascii is nice for mnemonics (in English). opening to utf-8 would make it look less amateur-hour.

The grinding attack is picking another source from (obs having the keypair), and griefing some other dude's from. The goal is to make that griefing look hard or futile.

I think we can do this with:

  1. u64, maybe low 8 bytes of hash(mnemonic string)?
  2. hashing a limited-length string as proposed. Upgrading to utf-8 hurts the deterrent a little as you can cover 256 bits of space with only 13-ish utf-8 chars (at 20bits/char).

We can also say "who cares? grind away!"

This comment has been minimized.

Copy link
@rob-solana

rob-solana Dec 10, 2019

Author Contributor

I'm planning to remove the ascii stuff, appreciate input here @t-nelson @aeyakovenko @garious

This comment has been minimized.

Copy link
@t-nelson

t-nelson Dec 10, 2019

Contributor

were it as small as a u16, mnemonics get harder. ascii is nice for mnemonics (in English). opening to utf-8 would make it look less amateur-hour.

Right, the u16 suggestion would discard the mnemonic for an index (from gets I accounts under program_id). Which gives cheap introspection, but no pseudonymity.

The grinding attack is picking another source from (obs having the keypair), and griefing some other dude's from. The goal is to make that griefing look hard or futile.

Yep. A SHA256 second preimage search is pretty futile IMO. Restricting free bits per from is about the most effective deterrent we can enact. Grinding froms is always possible and slower than the hashing step. You've already covered the other speedup. That is, requiring fixed bits at the end of the input data, preventing working from a midstate.

I think we can do this with:

1. u64, maybe low 8 bytes of hash(mnemonic string)?

2. hashing a limited-length string as proposed.  Upgrading to utf-8 hurts the deterrent a little as you can cover 256 bits of space with only 13-ish utf-8 chars (at 20bits/char).

I think (valid) utf-8 would actually be more restricted. For non-ASCII code points, the top two bits of every byte are fixed and the first byte also consumes up to three more bits to encode the length. Not all code pages are in use or full either, further restricting valid bitstrings

We can also say "who cares? grind away!"

Probably fine. See, https://lbc.cryptoguru.org/about

@garious

This comment has been minimized.

Copy link
Member

garious commented Dec 10, 2019

The implications here might be more far-reaching than intended. Without derived addresses, we can assert all account addresses are ed25519 pubkeys. And with that invariant we can say every account creator at one time possessed its private key. Is that a valuable invariant? Hard to say. BIP32 would preserve it, but not exactly an option for ed25519.

@rob-solana

This comment has been minimized.

Copy link
Contributor Author

rob-solana commented Dec 10, 2019

The implications here might be more far-reaching than intended. Without derived addresses, we can assert all account addresses are ed25519 pubkeys. And with that invariant we can say every account creator at one time possessed its private key. Is that a valuable invariant? Hard to say. BIP32 would preserve it, but not exactly an option for ed25519.

does Pubkey::new_rand() (a perfectly acceptable to for a transfer) guarantee an ed25519 pubkey?

@garious

This comment has been minimized.

Copy link
Member

garious commented Dec 10, 2019

does Pubkey::new_rand() (a perfectly acceptable to for a transfer) guarantee an ed25519 pubkey?

I've always been quietly uncomfortable with new_rand since I wrote it, but didn't want the ed25519 dependency in pubkey.rs, which needs to compile to BPF. In any case, the unit-testing utility doesn't set a precedent. It can change. So can the genesis accounts. This instruction, however, once added to the System interface, is forever.

@aeyakovenko

This comment has been minimized.

Copy link
Member

aeyakovenko commented Dec 10, 2019

@garious there is no way to prevent a transfer to a random address. Destinations don’t require a proof.

@garious

This comment has been minimized.

Copy link
Member

garious commented Dec 10, 2019

there is no way to prevent a transfer to a random address. Destinations don’t require a proof.

There may be a way to enforce the address is a valid pubkey. We should consider locking it down and replacing new_rand() with Keypair::new().pubkey()

@garious

This comment has been minimized.

Copy link
Member

garious commented Dec 10, 2019

I'm not saying we should go one way or another on this, but it's generally not a great idea to define a data type as the bits of either one data type or another, depending on some usage (semantic coupling). Effectively, that makes Pubkey a C-style union representing either a public key or a random string of bits. You might argue, no, it's just a random string of bits, but in fact we interpret it as a public key quite often (i.e. System::Transfer). And if random bits, why are we using Pubkey? I'd suggest sleeping on it. There's probably a more elegant solution here - maybe defining a new type AccountAddress as an enum instead of a Pubkey.

@mergify mergify bot dismissed stale reviews from aeyakovenko and t-nelson Dec 10, 2019

Pull request has been modified.

@@ -50,11 +92,18 @@ fn create_system_account(
return Err(SystemError::ResultWithNegativeLamports.into());
}

assign_account_to_program(to, program_id)?;

This comment has been minimized.

Copy link
@ryoqun

ryoqun Dec 10, 2019

Member

Not calling assign_account_to_program anymore for normal accounts starts to skip this existing check for account.signer_key. Is that OK?

This comment has been minimized.

Copy link
@rob-solana

rob-solana Dec 10, 2019

Author Contributor

that check is already present in create_account() and is not enforced by create_account_with_seed()

this change was intentional

note that no tests needed to change, we're still requiring to signature in the non-seed case

This comment has been minimized.

Copy link
@ryoqun

ryoqun Dec 10, 2019

Member

Ok, thanks for clarifying!

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 10, 2019

Codecov Report

Merging #7390 into master will decrease coverage by 12.5%.
The diff coverage is 73.5%.

@@            Coverage Diff            @@
##           master   #7390      +/-   ##
=========================================
- Coverage    80.6%     68%   -12.6%     
=========================================
  Files         245     245              
  Lines       48408   57525    +9117     
=========================================
+ Hits        39025   39135     +110     
- Misses       9383   18390    +9007
@rob-solana

This comment has been minimized.

Copy link
Contributor Author

rob-solana commented Dec 10, 2019

I'm not saying we should go one way or another on this, but it's generally not a great idea to define a data type as the bits of either one data type or another, depending on some usage (semantic coupling). Effectively, that makes Pubkey a C-style union representing either a public key or a random string of bits. You might argue, no, it's just a random string of bits, but in fact we interpret it as a public key quite often (i.e. System::Transfer). And if random bits, why are we using Pubkey? I'd suggest sleeping on it. There's probably a more elegant solution here - maybe defining a new type AccountAddress as an enum instead of a Pubkey.

sleeping on it good.

I wonder if we can force the resulting generated addresses to be unreachable pubkeys in ed25519

@aeyakovenko

This comment has been minimized.

Copy link
Member

aeyakovenko commented Dec 10, 2019

@rob-solana @garious we can set a bit in the Account to indicate that its not a real pubkey and cannot be signed with. that makes more sense than changing the address type.

@garious

This comment has been minimized.

Copy link
Member

garious commented Dec 10, 2019

Perhaps the Account.owner could be the dynamic tag that implies how an AccountAddress should be interpreted. Or perhaps the "seed" needs to be an existing account address. Just some thoughts...

@rob-solana

This comment has been minimized.

Copy link
Contributor Author

rob-solana commented Dec 10, 2019

the target customer for this work found keypair::new() + create_account() + set_staker() to be untenable (i.e. a burner keypair for the "derived" account), objected to the existence of the ephemeral keypair to sign the create_account() TX

@rob-solana

This comment has been minimized.

Copy link
Contributor Author

rob-solana commented Dec 10, 2019

there is no way to prevent a transfer to a random address. Destinations don’t require a proof.

There may be a way to enforce the address is a valid pubkey. We should consider locking it down and replacing new_rand() with Keypair::new().pubkey()

I think just recasting Pubkey::new_rand() to do that would work?

@aeyakovenko

This comment has been minimized.

Copy link
Member

aeyakovenko commented Dec 10, 2019

@garious seed as an existing account address won’t prevent a easy to factor pubkey. I don’t see a problem of relaxing the account address = pubkey relationship. We have no way to force addresses to be strong public keys.

@rob-solana

This comment has been minimized.

Copy link
Contributor Author

rob-solana commented Dec 10, 2019

@garious seed as an existing account address won’t prevent a easy to factor pubkey. I don’t see a problem of relaxing the account address = pubkey relationship. We have no way to force addresses to be strong public keys.

In this discussion we should keep in mind that builtin program_ids and sysvars have this "not a Pubkey of a Keypair" property, too.

@garious

This comment has been minimized.

Copy link
Member

garious commented Dec 10, 2019

I think our best path forward is a new 32 byte AccountAddress type with a From instance for Pubkey.

@t-nelson

This comment has been minimized.

Copy link
Contributor

t-nelson commented Dec 10, 2019

maybe defining a new type AccountAddress as an enum instead of a Pubkey.

Bonus points for a potential curve migration mechanism

rob-solana added 3 commits Dec 10, 2019
@rob-solana rob-solana force-pushed the rob-solana:address-generator branch from 86915b6 to e00a910 Dec 10, 2019
@mergify mergify bot dismissed garious’s stale review Dec 10, 2019

Pull request has been modified.

@rob-solana

This comment has been minimized.

Copy link
Contributor Author

rob-solana commented Dec 10, 2019

updated to use a 32-char utf-8 string, which is equivalently or possibly more restrictive in grinding from seed as ascii

@mergify mergify bot dismissed t-nelson’s stale review Dec 10, 2019

Pull request has been modified.

@garious garious changed the title address with seed generator Add SystemInstruction::CreateAccountWithSeed Dec 11, 2019
@rob-solana rob-solana merged commit ea0ba19 into solana-labs:master Dec 12, 2019
12 checks passed
12 checks passed
Summary 1 rule matches and 3 potential rules
Details
buildkite/solana Build #16458 passed (35 minutes, 20 seconds)
Details
buildkite/solana/bench Passed (11 minutes, 29 seconds)
Details
buildkite/solana/checks Passed (1 minute, 52 seconds)
Details
buildkite/solana/coverage Passed (10 minutes, 12 seconds)
Details
buildkite/solana/local-cluster Passed (18 minutes, 18 seconds)
Details
buildkite/solana/move Passed (4 minutes, 32 seconds)
Details
buildkite/solana/pipeline-upload Passed (3 seconds)
Details
buildkite/solana/shellcheck Passed (29 seconds)
Details
buildkite/solana/stable Passed (33 minutes, 17 seconds)
Details
buildkite/solana/stable-perf Passed (17 minutes, 2 seconds)
Details
ci-gate Pull Request accepted for CI pipeline
@rob-solana rob-solana deleted the rob-solana:address-generator branch Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.