-
Notifications
You must be signed in to change notification settings - Fork 87
feat(node): configure ntx script cache size via clap #1454
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: next
Are you sure you want to change the base?
feat(node): configure ntx script cache size via clap #1454
Conversation
| bp_checkpoint: Arc<Barrier>, | ||
| script_cache_size: NonZeroUsize, | ||
| ) -> Self { | ||
| let script_cache = LruCache::new(script_cache_size); |
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.
I think we should just update new() and not have with_script_cache_size().
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.
I think we should just update
new()and not havewith_script_cache_size().
updated
| const ENV_MAX_TXS_PER_BATCH: &str = "MIDEN_MAX_TXS_PER_BATCH"; | ||
| const ENV_MAX_BATCHES_PER_BLOCK: &str = "MIDEN_MAX_BATCHES_PER_BLOCK"; | ||
| const ENV_MEMPOOL_TX_CAPACITY: &str = "MIDEN_NODE_MEMPOOL_TX_CAPACITY"; | ||
| const ENV_NTX_SCRIPT_CACHE_SIZE: &str = "MIDEN_NTX_DATA_STORE_SCRIPT_CACHE_SIZE"; |
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.
Please add this to bin/node/.env like the other env vars.
| const COMPONENT: &str = "miden-ntx-builder"; | ||
|
|
||
| pub const DEFAULT_SCRIPT_CACHE_SIZE: NonZeroUsize = | ||
| NetworkTransactionBuilder::DEFAULT_SCRIPT_CACHE_SIZE; |
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.
We don't need this do we?
| /// Each cached script contains the deserialized `NoteScript` object, so the actual memory usage | ||
| /// depends on the complexity of the scripts being cached. | ||
| const DEFAULT_SCRIPT_CACHE_SIZE: NonZeroUsize = NonZeroUsize::new(1000).unwrap(); | ||
| pub const DEFAULT_SCRIPT_CACHE_SIZE: NonZeroUsize = NonZeroUsize::new(1000).unwrap(); |
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.
This doesn't need to be pub does it?
reopen #1340
Fixes #1335