126 atomic balances #165
126 atomic balances #165
Conversation
|
Can you run |
|
sure |
src/accountant.rs
Outdated
| let bal = option.unwrap(); | ||
| let current = bal.load(Ordering::Relaxed) as i64; | ||
|
|
||
| if current < tr.data.tokens { |
There was a problem hiding this comment.
I was thinking you could start a loop here that continues until compare_exchange returns Ok or the balance check returns InsufficentFunds. You shouldn't need to add that new error type BalanceUpdatedBeforeTransactionCompleted.
rlkelly
force-pushed
the
126__atomic_balances
branch
from
b91d816 to
cece0b7
Compare
src/accountant.rs
Outdated
| self.process_verified_transaction_debits(tr)?; | ||
| self.process_verified_transaction_credits(tr); | ||
| Ok(()) | ||
| return match self.process_verified_transaction_debits(tr) { |
There was a problem hiding this comment.
Was this changed needed? I'm having trouble understanding what is different than the original code.
src/accountant.rs
Outdated
| } | ||
|
|
||
| /// Process a batch of verified transactions. | ||
| pub fn process_verified_transactions(&self, trs: Vec<Transaction>) -> Vec<Result<Transaction>> { | ||
| // Run all debits first to filter out any transactions that can't be processed | ||
| // in parallel deterministically. | ||
| let results: Vec<_> = trs.into_par_iter() | ||
| .map(|tr| self.process_verified_transaction_debits(&tr).map(|_| tr)) | ||
| .filter_map(|tr| match self.process_verified_transaction_debits(&tr) { |
There was a problem hiding this comment.
This is probably a slower version of the original code that loses information in those Err values.
There was a problem hiding this comment.
This is unnecessary now since we're looping on failed transactions, but it will raise errors if you have transactions that fail in process_packets_bench since they're not being filtered out
src/accountant.rs
Outdated
| @@ -131,23 +133,34 @@ impl Accountant { | |||
| // Hold a write lock before the condition check, so that a debit can't occur | |||
There was a problem hiding this comment.
With your changes, you can delete this comment.
src/accountant.rs
Outdated
|
|
||
| Ok(()) | ||
| return match result { | ||
| Ok(_) => Ok(()), |
There was a problem hiding this comment.
If you don't mind, please run Clippy before submitting PRs. It would have a suggestion to make this code more idiomatic.
|
This looks flawless to me. I can work out the CI issues. Thanks for taking this on and for adding |
rlkelly
force-pushed
the
126__atomic_balances
branch
from
284288c to
de358d5
Compare
src/accountant_skel.rs
Outdated
| @@ -774,11 +774,11 @@ mod bench { | |||
| // Seed the 'from' account. | |||
| let rando0 = KeyPair::new(); | |||
| let tr = Transaction::new(&mint.keypair(), rando0.pubkey(), 1_000, last_id); | |||
| acc.process_verified_transaction(&tr).unwrap(); | |||
| let _ = acc.process_verified_transaction(&tr); | |||
There was a problem hiding this comment.
All the way back please. That unwrap() was an important sanity check.
src/accountant_skel.rs
Outdated
| @@ -803,7 +803,10 @@ mod bench { | |||
| drop(skel.historian.sender); | |||
| let entries: Vec<Entry> = skel.historian.receiver.iter().collect(); | |||
| assert_eq!(entries.len(), 1); | |||
| assert_eq!(entries[0].events.len(), txs as usize); | |||
| assert_eq!( | |||
|
Some inconsistency between stable rustfmt and nightly rustfmt. I'll take care of that in a followup patch. Merging! |
126 atomic balances
… of solana-labs#165) (solana-labs#166) ci: fix Windows gh release pipeline (solana-labs#165) (cherry picked from commit 3863bb1) Co-authored-by: Yihau Chen <[email protected]>
… of anza-xyz#165) (anza-xyz#167) ci: fix Windows gh release pipeline (anza-xyz#165) (cherry picked from commit 3863bb1) Co-authored-by: Yihau Chen <[email protected]>
pull request for issue 126, updated some error handling and testing