[Feat] Facilitate record queries#3047
Conversation
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
ledger/src/get.rs
Outdated
| } | ||
|
|
||
| /// Returns a tuple containing the number of all the input records and all the output records. | ||
| pub fn get_record_count(&self) -> (usize, usize) { |
There was a problem hiding this comment.
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.
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.
Well we define the functions here... So we need it here.
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider unit testing or explicitly E2E testing the different cases
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 synthesizer/tests/test_vm_execute_and_finalize.rs and saw the only one with a chance of record consumption was synthesizer/tests/tests/vm/execute_and_finalize/mint_and_split.aleo. I modified the inputs to correctly consume one and my tested function did behave correctly; however, a new commit shortly after broke that so that my function was no longer being tested under record consumption. We suspect even if we fix the mint_and_split.aleo test case again, it will soon break.) In summary, I couldn't find any high-level tests which reliably involved record consumption.
As part of the dynamic dispatch work, we are adding many tests involving record consumption. Most test cases in the subfolders of https://github.com/ProvableHQ/snarkVM/blob/3a67e6a1f2091500d2a0a4778bfa0c51495ae1fb/synthesizer/src/vm/tests/test_v12 do (this is all happening in the feat/dynamic-dispatch-extension-case1wip branch). Cf. e. g.
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 :)
| let Some(transition_ids_w_fee) = execution_store.id_map().get_confirmed(&tx_id)? else { | ||
| continue; | ||
| }; | ||
| let (transition_ids, _fee) = &*transition_ids_w_fee; |
There was a problem hiding this comment.
Should we count I/O records in the _fee?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmm I'm not familiar with the database schema, but Fee transactions are different from Fee objects used in Execute or Deploy transactions.
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
| /// Returns a tuple containing the number of all the input records and all the output records. | ||
| pub fn get_record_count(&self) -> RecordCount { | ||
| let transition_store = self.vm.block_store().transition_store(); | ||
| let num_input_records = transition_store.input_store().record_map().len_confirmed(); |
There was a problem hiding this comment.
These seem quite expensive, as it's a O(N) scan for each len_confirmed call.
There was a problem hiding this comment.
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.
|
Why is this feature necessary? |
@niklaslong would you like to provide the rationale? |
4 participants
This PR introduces 2 new methods,
Ledger::{get_record_count, get_num_block_records}, and a few helper "getter" methods that they require to work.A snarkOS-side counterpart PR will follow.