-
Notifications
You must be signed in to change notification settings - Fork 95
feat: Validator database #1614
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: Validator database #1614
Conversation
|
@bobbinth I have validated these behaviours on this branch:
|
| #[instrument(target = COMPONENT, skip_all)] | ||
| pub async fn load(database_filepath: PathBuf) -> Result<miden_node_store::Db, DatabaseSetupError> { | ||
| let manager = ConnectionManager::new(database_filepath.to_str().unwrap()); | ||
| let pool = deadpool_diesel::Pool::builder(manager).max_size(16).build()?; |
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.
Does max_size mean the capacity, or is the actual capacity chosen differently and this is just the upper bound to that algorithm?
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.
An upper bound but I don't know what goes on under the hood. @drahnr do you know why we wouldn't just use default?
/// Maximum size of the [`Pool`].
///
/// Default: `cpu_count * 4`
///
/// [`Pool`]: super::Pool
pub max_size: usize,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.
No, I don't remember when or who introduced it
| #[instrument(target = COMPONENT, skip_all)] | ||
| pub async fn load(database_filepath: PathBuf) -> Result<miden_node_store::Db, DatabaseSetupError> { | ||
| let manager = ConnectionManager::new(database_filepath.to_str().unwrap()); | ||
| let pool = deadpool_diesel::Pool::builder(manager).max_size(16).build()?; |
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'd prefer to have our own builder pattern, with any options we use in practice.
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.
Unsure which builder we would be adding here. Is it something that would be used across both store and validator? Could roll it up into this issue #1653
| // Store the validated transaction. | ||
| self.db | ||
| .transact("insert_transaction", move |conn| insert_transaction(conn, &tx_info)) | ||
| .await?; |
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 should ensure that this is instrumented (it should be, but async + closures is tricky)
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.
2026-02-12T22:43:49.934198Z INFO rpc:validate_block:find_unvalidated_transactions: miden-validator: crates/validator/src/db/mod.rs:68: close, time.busy: 11.1ms, time.idle: 449µs rpc.service: "validator.Api", rpc.method: "SignBlock", otel.name: "validator.Api/SignBlock" tx_ids: [0x18f8ad1c9312257fe192507336a4b6590d2f0a92127506eb47738c77b18c258b, 0x7c9fed92d7bd767796a7d51f8e9a368bbf2b17a601ff471aa07dfc88bee7dcd1]
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.
2026-02-12T22:55:33.672764Z INFO rpc:submit_proven_transaction:insert_transaction: miden-validator: crates/validator/src/db/mod.rs:39: close, time.busy: 6.56ms, time.idle: 35.0µs rpc.service: "validator.Api", rpc.method: "SubmitProvenTransaction", otel.name: "validator.Api/SubmitProvenTransaction" tx_id: 0x99eeffc42a2b6150e6e7fed88f86350e926fd52992911b02bd4162abec0e8670
crates/validator/src/db/models.rs
Outdated
| pub fn new(info: &ValidatedTransactionInfo) -> Self { | ||
| Self { | ||
| id: info.tx_id().to_bytes(), | ||
| block_num: info.block_num().to_raw_sql(), | ||
| account_id: info.account_id().to_bytes(), | ||
| info: info.to_bytes(), | ||
| } |
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 handle all the serialization here, instead of serializing in VAlidatedTransactionInfo as I allude to in this comment
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.
LMK if my latest changes reflect your thinking here
| // Convert TransactionIds to bytes for query. | ||
| let tx_id_bytes: Vec<Vec<u8>> = tx_ids.iter().map(TransactionId::to_bytes).collect(); | ||
|
|
||
| // Query the database for matching transactions ids. | ||
| let raw_transaction_ids = schema::validated_transactions::table | ||
| .select(schema::validated_transactions::id) | ||
| .filter(schema::validated_transactions::id.eq_any(tx_id_bytes)) | ||
| .order(schema::validated_transactions::id.asc()) | ||
| .load::<Vec<u8>>(conn) | ||
| .map_err(DatabaseError::from)?; |
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 is probably not important at the moment, but since this will likely always remain a sqlite local database, I think we should just do an exists check in a loop? This would simplify the query, return values and output construction.
Though whether diesel supports this is unclear to me.
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.
Unsure how many transactions we expect to have in the average block, but feels counterintuitive to scale # of db queries with # of transactions.
| } | ||
|
|
||
| // Start the Validator if we have bound a socket. | ||
| if let Some(address) = validator_socket_address { |
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.
The configuration decision based on the validator_socket_address is a bit obfuscating the fact that the user did not provide a validator URL but a key
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.
Not sure that part of the stack should be considering the key. Key presence is considered elsewhere. This part of the stack just cares about whether to run the local instance or not
| /// Open a connection to the DB and apply any pending migrations. | ||
| #[instrument(target = COMPONENT, skip_all)] | ||
| pub async fn load(database_filepath: PathBuf) -> Result<miden_node_store::Db, DatabaseSetupError> { | ||
| let manager = ConnectionManager::new(database_filepath.to_str().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.
@Mirko-von-Leipzig @drahnr despite this (and the PRAGMA queries it makes for WAL etc) I still get db locked errors when running the integration tests.
ERROR rpc:submit_proven_transaction: miden-validator: crates/validator/src/server/mod.rs:126:
error: status: 'Internal error', self: "database is locked" rpc.service:
"validator.Api", rpc.method: "SubmitProvenTransaction", otel.name: "validator.Api/SubmitProvenTransaction"
Validator could receive parallel requests for submit_proven_transaction endpoint so I do think its something we need to solve (not just integration tests' fault).
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.
Adding this does avoid it. ATM I'm reusing the store's connection manager code for validator.
pub(crate) fn configure_connection_on_creation(
conn: &mut SqliteConnection,
) -> Result<(), ConnectionManagerError> {
// Wait up to 5 seconds for writer locks before erroring.
diesel::sql_query("PRAGMA busy_timeout=5000")
.execute(conn)
.map_err(ConnectionManagerError::ConnectionParamSetup)?;
Context
The Validator currently only stores validated transactions in-memory. We want to persist validated transactions for resilience and potential future use cases around debugging / audit-ability w.r.t public transactions.
The Validator will be run as a separate instance to the node so it needs its own database, schema, etc.
Relates to #1316.
Changes
ValidatorConfigstruct.