Fix vote program bugs

Also:

* Add an assertion to the transaction builder if not enough
keypairs were provided for all keys that require signatures.
* Expose bugs in the runtime.

programs/native/vote/src/lib.rs

@@ -21,12 +21,6 @@ fn entrypoint(
     trace!("process_instruction: {:?}", data);
     trace!("keyed_accounts: {:?}", keyed_accounts);
 
-    // all vote instructions require that accounts_keys[0] be a signer
-    if keyed_accounts[0].signer_key().is_none() {
-        error!("account[0] is unsigned");
-        Err(ProgramError::InvalidArgument)?;
-    }
-
     match deserialize(data).map_err(|_| ProgramError::InvalidUserdata)? {
         VoteInstruction::InitializeAccount => vote_program::initialize_account(keyed_accounts),
         VoteInstruction::DelegateStake(delegate_id) => {

programs/native/vote/tests/vote.rs

@@ -0,0 +1,100 @@
+//use solana_runtime::bank::BankError;
+use solana_runtime::bank::{Bank, Result};
+use solana_sdk::genesis_block::GenesisBlock;
+use solana_sdk::hash::hash;
+//use solana_sdk::native_program::ProgramError;
+use solana_sdk::pubkey::Pubkey;
+use solana_sdk::signature::{Keypair, KeypairUtil};
+use solana_sdk::system_instruction::SystemInstruction;
+use solana_sdk::transaction_builder::{BuilderInstruction, TransactionBuilder};
+use solana_sdk::vote_program::{self, Vote, VoteInstruction, VoteState};
+use solana_sdk::vote_transaction::VoteTransaction;
+
+struct VoteBank<'a> {
+    bank: &'a Bank,
+}
+
+impl<'a> VoteBank<'a> {
+    fn new(bank: &'a Bank) -> Self {
+        bank.add_native_program("solana_vote_program", &vote_program::id());
+        Self { bank }
+    }
+
+    fn create_vote_account(
+        &self,
+        from_keypair: &Keypair,
+        vote_id: Pubkey,
+        lamports: u64,
+    ) -> Result<()> {
+        let last_id = self.bank.last_id();
+        let tx = VoteTransaction::fund_staking_account(from_keypair, vote_id, last_id, lamports, 0);
+        self.bank.process_transaction(&tx)
+    }
+
+    fn submit_vote(&self, vote_keypair: &Keypair, tick_height: u64) -> Result<VoteState> {
+        let last_id = self.bank.last_id();
+        let tx = VoteTransaction::new_vote(vote_keypair, tick_height, last_id, 0);
+        self.bank.process_transaction(&tx)?;
+        self.bank.register_tick(&hash(last_id.as_ref()));
+
+        let vote_account = self.bank.get_account(&vote_keypair.pubkey()).unwrap();
+        Ok(VoteState::deserialize(&vote_account.userdata).unwrap())
+    }
+}
+
+#[test]
+fn test_vote_via_bank() {
+    let (genesis_block, from_keypair) = GenesisBlock::new(10_000);
+    let bank = Bank::new(&genesis_block);
+    let vote_bank = VoteBank::new(&bank);
+
+    let vote_keypair = Keypair::new();
+    let vote_id = vote_keypair.pubkey();
+    vote_bank
+        .create_vote_account(&from_keypair, vote_id, 100)
+        .unwrap();
+
+    let vote_state = vote_bank.submit_vote(&vote_keypair, 0).unwrap();
+    assert_eq!(vote_state.votes.len(), 1);
+}
+
+#[test]
+fn test_vote_via_bank_with_no_signature() {
+    let (genesis_block, mallory_keypair) = GenesisBlock::new(10_000);
+    let bank = Bank::new(&genesis_block);
+    let vote_bank = VoteBank::new(&bank);
+
+    let vote_keypair = Keypair::new();
+    let vote_id = vote_keypair.pubkey();
+    vote_bank
+        .create_vote_account(&mallory_keypair, vote_id, 100)
+        .unwrap();
+
+    let mallory_id = mallory_keypair.pubkey();
+    let last_id = bank.last_id();
+    let vote_ix = BuilderInstruction::new(
+        vote_program::id(),
+        &VoteInstruction::Vote(Vote::new(0)),
+        vec![(vote_id, false)], // <--- attack!! No signature.
+    );
+
+    // Sneak in an instruction so that the transaction is signed but
+    // the 0th account in the second instruction is not! The program
+    // needs to check that it's signed.
+    let tx = TransactionBuilder::default()
+        .push(SystemInstruction::new_move(mallory_id, vote_id, 1))
+        .push(vote_ix)
+        .sign(&[&mallory_keypair], last_id);
+
+    let _result = bank.process_transaction(&tx);
+
+    // And ensure there's no vote.
+    let vote_account = bank.get_account(&vote_id).unwrap();
+    let vote_state = VoteState::deserialize(&vote_account.userdata).unwrap();
+    assert_eq!(vote_state.votes.len(), 0);
+
+    //assert_eq!(
+    //    result,
+    //    Err(BankError::ProgramError(1, ProgramError::InvalidArgument))
+    //);
+}

sdk/src/transaction_builder.rs

@@ -53,14 +53,24 @@ impl TransactionBuilder {
 
     /// Return pubkeys referenced by all instructions, with the ones needing signatures first.
     /// No duplicates and order is preserved.
-    fn keys(&self) -> Vec<Pubkey> {
+    fn keys(&self) -> (Vec<Pubkey>, Vec<Pubkey>) {
-        let mut key_and_signed: Vec<_> = self
+        let mut keys_and_signed: Vec<_> = self
             .instructions
             .iter()
             .flat_map(|ix| ix.accounts.iter())
             .collect();
-        key_and_signed.sort_by(|x, y| y.1.cmp(&x.1));
+        keys_and_signed.sort_by(|x, y| y.1.cmp(&x.1));
-        key_and_signed.into_iter().map(|x| x.0).unique().collect()
+
+        let mut signed_keys = vec![];
+        let mut unsigned_keys = vec![];
+        for (key, signed) in keys_and_signed.into_iter().unique_by(|x| x.0) {
+            if *signed {
+                signed_keys.push(*key);
+            } else {
+                unsigned_keys.push(*key);
+            }
+        }
+        (signed_keys, unsigned_keys)
     }
 
     /// Return program ids referenced by all instructions.  No duplicates and order is preserved.
@@ -82,16 +92,18 @@ impl TransactionBuilder {
 
     /// Return a signed transaction.
     pub fn sign<T: KeypairUtil>(&self, keypairs: &[&T], last_id: Hash) -> Transaction {
-        let keys = self.keys();
         let program_ids = self.program_ids();
-        let instructions = self.instructions(&keys, &program_ids);
+        let (mut signed_keys, unsigned_keys) = self.keys();
         for (i, keypair) in keypairs.iter().enumerate() {
-            assert_eq!(keypair.pubkey(), keys[i], "keypair-pubkey mismatch");
+            assert_eq!(keypair.pubkey(), signed_keys[i], "keypair-pubkey mismatch");
         }
-        let unsigned_keys = &keys[keypairs.len()..];
+        assert_eq!(keypairs.len(), signed_keys.len(), "not enough keypairs");
+
+        signed_keys.extend(&unsigned_keys);
+        let instructions = self.instructions(&signed_keys, &program_ids);
         Transaction::new_with_instructions(
             keypairs,
-            unsigned_keys,
+            &unsigned_keys,
             last_id,
             self.fee,
             program_ids,
@@ -147,7 +159,7 @@ mod tests {
             .push(Instruction::new(program_id, &0, vec![(id0, true)]))
             .push(Instruction::new(program_id, &0, vec![(id0, true)]))
             .keys();
-        assert_eq!(keys, vec![id0]);
+        assert_eq!(keys, (vec![id0], vec![]));
     }
 
     #[test]
@@ -158,7 +170,7 @@ mod tests {
             .push(Instruction::new(program_id, &0, vec![(id0, false)]))
             .push(Instruction::new(program_id, &0, vec![(id0, true)]))
             .keys();
-        assert_eq!(keys, vec![id0]);
+        assert_eq!(keys, (vec![id0], vec![]));
     }
 
     #[test]
@@ -170,7 +182,7 @@ mod tests {
             .push(Instruction::new(program_id, &0, vec![(id0, false)]))
             .push(Instruction::new(program_id, &0, vec![(id1, false)]))
             .keys();
-        assert_eq!(keys, vec![id0, id1]);
+        assert_eq!(keys, (vec![], vec![id0, id1]));
     }
 
     #[test]
@@ -183,7 +195,7 @@ mod tests {
             .push(Instruction::new(program_id, &0, vec![(id1, false)]))
             .push(Instruction::new(program_id, &0, vec![(id0, true)]))
             .keys();
-        assert_eq!(keys, vec![id0, id1]);
+        assert_eq!(keys, (vec![id0], vec![id1]));
     }
 
     #[test]
@@ -195,7 +207,7 @@ mod tests {
             .push(Instruction::new(program_id, &0, vec![(id0, false)]))
             .push(Instruction::new(program_id, &0, vec![(id1, true)]))
             .keys();
-        assert_eq!(keys, vec![id1, id0]);
+        assert_eq!(keys, (vec![id1], vec![id0]));
     }
 
     #[test]
@@ -205,6 +217,17 @@ mod tests {
         TransactionBuilder::default().sign(&[&keypair], Hash::default());
     }
 
+    #[test]
+    #[should_panic]
+    fn test_transaction_builder_missing_keypair() {
+        let program_id = Pubkey::default();
+        let keypair0 = Keypair::new();
+        let id0 = keypair0.pubkey();
+        TransactionBuilder::default()
+            .push(Instruction::new(program_id, &0, vec![(id0, true)]))
+            .sign(&Vec::<&Keypair>::new(), Hash::default());
+    }
+
     #[test]
     #[should_panic]
     fn test_transaction_builder_wrong_key() {

sdk/src/vote_program.rs

@@ -272,6 +272,11 @@ pub fn process_vote(keyed_accounts: &mut [KeyedAccount], vote: Vote) -> Result<(
         Err(ProgramError::InvalidArgument)?;
     }
 
+    if keyed_accounts[0].signer_key().is_none() {
+        error!("account[0] should sign the transaction");
+        Err(ProgramError::InvalidArgument)?;
+    }
+
     let mut vote_state = VoteState::deserialize(&keyed_accounts[0].account.userdata)?;
     vote_state.process_vote(vote);
     vote_state.serialize(&mut keyed_accounts[0].account.userdata)?;
@@ -403,6 +408,21 @@ mod tests {
         assert_eq!(vote_state.credits(), 0);
     }
 
+    #[test]
+    fn test_vote_signature() {
+        let from_id = Keypair::new().pubkey();
+        let mut from_account = Account::new(100, 0, Pubkey::default());
+        let vote_id = Keypair::new().pubkey();
+        let mut vote_account = create_vote_account(100);
+        initialize_and_deserialize(&from_id, &mut from_account, &vote_id, &mut vote_account)
+            .unwrap();
+
+        let vote = Vote::new(1);
+        let mut keyed_accounts = [KeyedAccount::new(&vote_id, false, &mut vote_account)];
+        let res = process_vote(&mut keyed_accounts, vote);
+        assert_eq!(res, Err(ProgramError::InvalidArgument));
+    }
+
     #[test]
     fn test_vote_without_initialization() {
         let vote_id = Keypair::new().pubkey();