-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[Feat] Facilitate record queries #3047
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
base: staging
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -15,6 +15,8 @@ | |||
|
|
||||
| use super::*; | ||||
|
|
||||
| use crate::store::TransactionType; | ||||
|
|
||||
| impl<N: Network, C: ConsensusStorage<N>> Ledger<N, C> { | ||||
| /// Returns the committee for the given `block height`. | ||||
| pub fn get_committee(&self, block_height: u32) -> Result<Option<Committee<N>>> { | ||||
|
|
@@ -378,4 +380,89 @@ impl<N: Network, C: ConsensusStorage<N>> Ledger<N, C> { | |||
| _ => bail!("Invalid bond_state in finalize storage."), | ||||
| } | ||||
| } | ||||
|
|
||||
| /// Returns a tuple containing the number of all the input records and all the output records. | ||||
| pub fn get_record_count(&self) -> (usize, usize) { | ||||
| let transition_store = self.vm.block_store().transition_store(); | ||||
| let num_input_records = transition_store.input_store().record_map().len_confirmed(); | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These seem quite expensive, as it's a O(N) scan for each
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd call it somewhat expensive - in all likelihood, it's the other call that's the more expensive one due to the number of queries involved; obtaining the number of records for a given map is O(N), but it's an optimized operation. |
||||
| let num_output_records = transition_store.output_store().record_map().len_confirmed(); | ||||
| (num_input_records, num_output_records) | ||||
| } | ||||
|
|
||||
| /// Returns the list of input and output records applicable to the given block height. | ||||
| pub fn get_num_block_records(&self, height: u32) -> Result<(usize, usize)> { | ||||
| let block_store = self.vm.block_store(); | ||||
| let block_hash = match block_store.get_block_hash(height)? { | ||||
| Some(block_hash) => block_hash, | ||||
| None => bail!("Block {height} does not exist in storage"), | ||||
| }; | ||||
|
|
||||
| let Some(block_transaction_ids) = block_store.transactions_map().get_confirmed(&block_hash)? else { | ||||
| return Ok(Default::default()); | ||||
| }; | ||||
|
|
||||
| let transaction_store = block_store.transaction_store(); | ||||
| let mut transaction_ids_with_type = Vec::with_capacity(block_transaction_ids.len()); | ||||
| for tx_id in block_transaction_ids.iter() { | ||||
| let Some(tx_ty) = transaction_store.id_map().get_confirmed(tx_id)? else { | ||||
| bail!("Missing type for transaction {tx_id}"); | ||||
| }; | ||||
| transaction_ids_with_type.push((*tx_id, tx_ty)); | ||||
| } | ||||
|
|
||||
| let transition_store = transaction_store.transition_store(); | ||||
| let execution_store = transaction_store.execution_store(); | ||||
| let fee_store = transaction_store.fee_store(); | ||||
| let input_store = transition_store.input_store(); | ||||
| let output_store = transition_store.output_store(); | ||||
|
|
||||
| let mut num_input_records = 0usize; | ||||
| let mut num_output_records = 0usize; | ||||
|
|
||||
| let mut process_transition_ids = |transition_ids: &[N::TransitionID]| -> Result<()> { | ||||
| for transition_id in transition_ids { | ||||
| let input_ids = transition_store.get_input_ids(transition_id)?; | ||||
| let output_ids = transition_store.get_output_ids(transition_id)?; | ||||
|
|
||||
| for id in input_ids { | ||||
| if input_store.record_map().contains_key_confirmed(&id)? { | ||||
| num_input_records += 1; | ||||
| } | ||||
| } | ||||
| for id in output_ids { | ||||
| if output_store.record_map().contains_key_confirmed(&id)? { | ||||
| num_output_records += 1; | ||||
| } | ||||
| } | ||||
|
Comment on lines
+427
to
+436
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider unit testing or explicitly E2E testing the different cases
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be happy to do so; is there any existing test that guarantees the existence of records, which I could repurpose for record counting checks?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually ongoing work by @Antonio95 :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @ljedrz ! For some more context, what are your testing needs? I recently added some functionality that I wanted to specifically test (among other cases) on transactions involving transitions which consumed records. (Extra background: I went through the test cases in As part of the dynamic dispatch work, we are adding many tests involving record consumption. Most test cases in the subfolders of
That code will likely not be merged for a few weeks, so if you want to add tests before that you can take inspiration from what I sent, or I'd also be happy to write a test case if you tell me what you need in a bit more detail :)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CC @niklaslong |
||||
| } | ||||
|
|
||||
| Ok(()) | ||||
| }; | ||||
|
|
||||
| for (tx_id, tx_ty) in transaction_ids_with_type { | ||||
| match *tx_ty { | ||||
| TransactionType::Deploy => { | ||||
| continue; | ||||
| } | ||||
| TransactionType::Execute => { | ||||
| let Some(transition_ids_w_fee) = execution_store.id_map().get_confirmed(&tx_id)? else { | ||||
| continue; | ||||
| }; | ||||
| let (transition_ids, _fee) = &*transition_ids_w_fee; | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we count I/O records in the _fee?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fee records should be counted by the loop in L455 - the fee in this particular map is only an indicator if there was a fee associated with the related execution and its transitions.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm I'm not familiar with the database schema, but Fee transactions are different from Fee objects used in Execute or Deploy transactions. |
||||
|
|
||||
| process_transition_ids(transition_ids)?; | ||||
| } | ||||
| TransactionType::Fee => { | ||||
| let Some(transition_id_w_root_and_proof) = fee_store.fee_map().get_confirmed(&tx_id)? else { | ||||
| continue; | ||||
| }; | ||||
| let (transition_id, _root, _proof) = &*transition_id_w_root_and_proof; | ||||
|
|
||||
| process_transition_ids(&[*transition_id])?; | ||||
| } | ||||
| } | ||||
| } | ||||
|
|
||||
| Ok((num_input_records, num_output_records)) | ||||
| } | ||||
| } | ||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: a struct with named fields will save developers of dependents a lot of reasoning work - I still regret our
*_cost*functions returning tuples.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, though the place for these structs would be snarkOS, no? Or is it ok if we include "target" objects in this module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we define the functions here... So we need it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
657a046