feat: store seed command in stress test CLI #657
Conversation
TomasArrachea
force-pushed
the
santiagopittella-stress-testing-copy
branch
from
c749355 to
ba71858
Compare
| note::create_p2id_note, | ||
| utils::Serializable, | ||
| }; | ||
| use miden_node_block_producer::store::StoreClient; |
There was a problem hiding this comment.
Is there a way to get the store client w/o going through the block producer? I wonder if #723 changes this. cc @Mirko-von-Leipzig.
There was a problem hiding this comment.
The block producer's StoreClient is a somewhat thin wrapper around the generated store client from miden-node-proto. Notably it does some additional domain type translations.
At present #723 doesn't change this - its primarily just about changing how we distribute the raw proto files at the moment.
In the short term exposing this StoreClient is the simplest route unfortunately, or alternatively just using the generated store client - which would essentially duplicate the code I imagine.
Perhaps in the medium term we should focus on moving all client/store wrappers into the crate with the domain types. So one crate for accessing the raw proto definitions, and another for internal use with nicer domain types.
The issue with the latter is how do we allow for extensions, e.g. enabling open-telemetry, TLS etc. Its possible I'm sure, but I find the tonic docs and naming to be confusing so its not immediately obvious to me how this works.
Another issue with moving the wrappers is that all domain types would need to live in this proto crate i.e. we cannot use types from miden-node-store, miden-node-block-producer etc, so those all have to move across. Which may or may not be fine. A lot of them are already there so perhaps its not such an issue.
There was a problem hiding this comment.
Yes, as for now this is the simplest way to interact with the Store. I agree it would make sense to, at some point, have the StoreClient declared as a domain type, but it's probably better to do that it in a separate PR.
There was a problem hiding this comment.
Another issue with moving the wrappers is that all domain types would need to live in this proto crate i.e. we cannot use types from miden-node-store, miden-node-block-producer etc, so those all have to move across. Which may or may not be fine. A lot of them are already there so perhaps its not such an issue.
At least for the store, this would require moving the StoreErrors. Besides that, I think most of these types are shared from miden-objects and it wouldn't be a problem.
There was a problem hiding this comment.
I'd love to make to so that stress-test does not depend on the block producer, but we can do this in the future. Let's create an issue for this.
|
This is how the latest output looks: |
bin/stress-test/src/main.rs
Outdated
| /// Creates and stores blocks into the store. Creates a given number of accounts, where each account | ||
| /// consumes a note created from a faucet. The cli accepts the following parameters: | ||
| /// - `data_directory`: Directory in which to store the database and raw block data. | ||
| /// - `num_accounts`: Number of accounts to create. |
There was a problem hiding this comment.
This doc looks unnecessary, it is from the command defined above.
bin/stress-test/src/main.rs
Outdated
| let start = Instant::now(); | ||
|
|
||
| let genesis_filepath = data_directory.join(GENESIS_STATE_FILENAME); | ||
| let database_filepath = data_directory.join("miden-store.sqlite3"); |
There was a problem hiding this comment.
This might be extracted into a constant.
| note::create_p2id_note, | ||
| utils::Serializable, | ||
| }; | ||
| use miden_node_block_producer::store::StoreClient; |
There was a problem hiding this comment.
I'd love to make to so that stress-test does not depend on the block producer, but we can do this in the future. Let's create an issue for this.
|
I tested this while I did a bunch of things (I even went into the DB a bunch of times to check progress, so probably not very representative results), and I got ~2.5 hours: cargo run --release -- seed-store --data-directory ./data --num-accounts 1000000 |
There was a problem hiding this comment.
All looks good! Thank you! I'm going to merge as is, but let's do the following thing:
- Create an issue for investigating how to make the stress test not depend on
block-producerfor the store API. - Run the account generation again first for 100K accounts (to compare with prior results) and then for 1M accounts to see if the results @igamigo got were representative.
Regarding the results, if these were in fact representative, we should create an issue to investigate what exactly is driving the performance. The database is not that huge (i.e., 2.5GB) and so, it shouldn't be taking ~800ms to retrieve block inputs or 1 - 2 seconds to insert a block.
@bobbinth i'm applying your suggestion of a new structure (your comment here). The only thing I changed is that I create all the accounts before hand.
I added some queries to check the size of each table, and the avg size of each entry in every table. This is the current output of the binary:
For example, this is the current output for 100k accounts:
This PR was built on top of #621 .