Add SystemInstruction::CreateAccountWithSeed #7390

rob-solana · 2019-12-10T01:04:20Z

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 #

aeyakovenko

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

mvines · 2019-12-10T01:52:10Z

runtime/src/system_instruction_processor.rs

+        return Err(InstructionError::MissingRequiredSignature);
+    }
+
+    // re-derive the address, must match to


Suggested change

// re-derive the address, must match to

// re-derive the address, must match `to`

mvines · 2019-12-10T01:52:25Z

runtime/src/system_instruction_processor.rs

+    space: u64,
+    program_id: &Pubkey,
+) -> Result<(), InstructionError> {
+    // from is the source of the derived address, the caller must have


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

mvines · 2019-12-10T01:55:26Z

sdk/src/system_instruction.rs

+    /// * 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 {


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

lemme take a crack first

ok, I failed: solana-labs/solana-web3.js#571

t-nelson

Looks good! Just the one design Q

t-nelson · 2019-12-10T02:18:19Z

runtime/src/system_instruction_processor.rs

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


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

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

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

@rob-solana grinding on what though?

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

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

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?

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:

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

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!"

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

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 · 2019-12-10T02:30:55Z

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 · 2019-12-10T02:38:20Z

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 · 2019-12-10T02:54:50Z

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 · 2019-12-10T03:20:08Z

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

garious · 2019-12-10T03:25:03Z

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 · 2019-12-10T03:34:14Z

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.

Pull request has been modified.

ryoqun · 2019-12-10T04:03:05Z

runtime/src/system_instruction_processor.rs

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

-    assign_account_to_program(to, program_id)?;


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

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

Ok, thanks for clarifying!

codecov · 2019-12-10T04:17:15Z

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 · 2019-12-10T04:27:39Z

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 · 2019-12-10T04:34:42Z

@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 · 2019-12-10T04:41:27Z

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 · 2019-12-10T05:13:22Z

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 · 2019-12-10T05:16:25Z

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 · 2019-12-10T05:19:08Z

@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 · 2019-12-10T06:03:20Z

@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 · 2019-12-10T15:22:51Z

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

t-nelson · 2019-12-10T15:38:52Z

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

Bonus points for a potential curve migration mechanism

Pull request has been modified.

rob-solana · 2019-12-10T22:11:09Z

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

Pull request has been modified.

rob-solana requested review from garious, aeyakovenko and t-nelson December 10, 2019 01:11

aeyakovenko previously approved these changes Dec 10, 2019

View reviewed changes

mvines reviewed Dec 10, 2019

View reviewed changes

t-nelson previously approved these changes Dec 10, 2019

View reviewed changes

ryoqun reviewed Dec 10, 2019

View reviewed changes

rob-solana added 3 commits December 10, 2019 11:19

address generator

ec5bc5e

coverage

3ffa729

fixups

e00a910

rob-solana force-pushed the address-generator branch from 86915b6 to e00a910 Compare December 10, 2019 19:19

garious previously approved these changes Dec 10, 2019

View reviewed changes

remove ascii restriction

404b2fe

t-nelson previously approved these changes Dec 10, 2019

View reviewed changes

illustrate that utf-8 doesn't expand the space

0b1f4ca

garious changed the title ~~address with seed generator~~ Add SystemInstruction::CreateAccountWithSeed Dec 11, 2019

rob-solana merged commit ea0ba19 into solana-labs:master Dec 12, 2019

rob-solana deleted the address-generator branch December 12, 2019 19:12

	// re-derive the address, must match to
	// re-derive the address, must match `to`

	// from is the source of the derived address, the caller must have
	// `from` is the source of the derived address, the caller must have

Add SystemInstruction::CreateAccountWithSeed #7390

Add SystemInstruction::CreateAccountWithSeed #7390

Conversation

rob-solana commented Dec 10, 2019 • edited Loading

Problem

Summary of Changes

aeyakovenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-nelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rob-solana Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garious commented Dec 10, 2019

rob-solana commented Dec 10, 2019

garious commented Dec 10, 2019

aeyakovenko commented Dec 10, 2019

garious commented Dec 10, 2019 • edited Loading

garious commented Dec 10, 2019

Choose a reason for hiding this comment

rob-solana Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 10, 2019 • edited Loading

Codecov Report

rob-solana commented Dec 10, 2019

aeyakovenko commented Dec 10, 2019

garious commented Dec 10, 2019

rob-solana commented Dec 10, 2019

rob-solana commented Dec 10, 2019

aeyakovenko commented Dec 10, 2019 • edited Loading

rob-solana commented Dec 10, 2019

garious commented Dec 10, 2019

t-nelson commented Dec 10, 2019

rob-solana commented Dec 10, 2019

rob-solana commented Dec 10, 2019 •

edited

Loading

rob-solana Dec 10, 2019 •

edited

Loading

garious commented Dec 10, 2019 •

edited

Loading

rob-solana Dec 10, 2019 •

edited

Loading

codecov bot commented Dec 10, 2019 •

edited

Loading

aeyakovenko commented Dec 10, 2019 •

edited

Loading