Transactions with atomic sequences of Programs #1381
Conversation
aeyakovenko
requested review from
garious and
mvines
and removed request for
garious
aeyakovenko
force-pushed
the
programs
branch
3 times, most recently
from
d5d6937 to
8b0c19b
Compare
src/transaction.rs
Outdated
| @@ -10,6 +10,17 @@ pub const SIGNED_DATA_OFFSET: usize = size_of::<Signature>(); | |||
| pub const SIG_OFFSET: usize = 0; | |||
| pub const PUB_KEY_OFFSET: usize = size_of::<Signature>() + size_of::<u64>(); | |||
|
|
|||
| #[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)] | |||
| pub struct Program { | |||
There was a problem hiding this comment.
Can you rename this to Instruction?
|
Can you rebase this PR to get clippy feedback from CI? |
aeyakovenko
force-pushed
the
programs
branch
3 times, most recently
from
f9e804a to
d55bce8
Compare
| pub fn process_transaction(tx: &Transaction, accounts: &mut [Account]) -> Result<()> { | ||
| pub fn process_transaction( | ||
| tx: &Transaction, | ||
| pix: usize, |
There was a problem hiding this comment.
nit: it's not at all obvious to me what a pix is. can we use more letters in this variable name?
There was a problem hiding this comment.
Mostly skimmed, the Transaction struct changes LGTM. We need to add multisig transactions and a way for programs to know what keys have signed the transaction, as it's no longer safe for a program to assume it's key[0] was what signed the transaction. But we can do that as a part of #1293 later
src/transaction.rs
Outdated
| /// The program code that executes this transaction is identified by the program_id. | ||
| /// this is an offset into the Transaction::program_keys field | ||
| pub program_id: u8, | ||
| /// Indecies into the keys array of which accounts to load |
| .iter_mut() | ||
| .map(|a| (a.program_id, a.tokens)) | ||
| .collect(); | ||
| fn execute_transaction(&self, tx: &Transaction, tx_accounts: &mut [Account]) -> Result<()> { |
There was a problem hiding this comment.
This can be two functions, the original function renamed to execute_instruction and this additional loop in a new execute_instructions. If the original function had any unit tests, this would have been the natural thing to do.
src/transaction.rs
Outdated
|
|
||
| let program_id = serialize(&(&self.program_id)).expect("serialize program_id"); | ||
| data.extend_from_slice(&program_id); | ||
| let mut data = serialize(&(&self.account_keys)).expect("serialize account_keys"); |
There was a problem hiding this comment.
Did you test this? The double-reference will work since serialize will accept any type, but is it useful? How does it affect the serialization format? If I remove it, what breaks?
Need more data. Maybe just post the relevant |
src/budget_transaction.rs
Outdated
| @@ -83,7 +83,7 @@ impl BudgetTransaction for Transaction { | |||
| let budget = Budget::Pay(payment); | |||
| let instruction = Instruction::NewContract(Contract { budget, tokens }); | |||
| let userdata = serialize(&instruction).unwrap(); | |||
| Self::new( | |||
| Self::new_one_program( | |||
There was a problem hiding this comment.
Looks like new was the right name. What happened here?
There was a problem hiding this comment.
@garious i am ok with either one. Just seemed that new was the most general interface. I can name this one new and the other one new_multi_instruction. How does that sound?
There was a problem hiding this comment.
Yes, sounds good. Use new for the most common, not the most general. The most general one will keep changing as we add parameters. That'd be a big pain to keep updating all the earlier, probably simpler, use cases.
src/transaction.rs
Outdated
| ) -> Self { | ||
| let program_keys = vec![program_id]; | ||
| let instructions = vec
Transactions contain a vector of instructions that are executed atomically. Bench shows a 2.3x speed up when using 5 instructions per tx.
aeyakovenko
force-pushed
the
programs
branch
from
d14ffbe to
2831715
Compare
|
@garious i think i addressed everyones comments. thanks for the review input everyone! |
Transactions contain a vector of instructions that are executed atomically.
Bench shows a 2.3x speed up when using 5 instructions per tx.
More ips for the same tps.
with pr
master
2.5/(5.6/5) = 2.23x more ips o a mac. We could pack more than 5 instructions into a larger transaction size. 5 fit into 512 bytes.