-
Notifications
You must be signed in to change notification settings - Fork 220
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
feat: improve the TMS validation process #4694
feat: improve the TMS validation process #4694
Conversation
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 agree with the added safeguard to ignore concurrent requests, but have some doubts about handling the inconsistent data errors (see below).
Any opinions, anyone?
) | ||
}) | ||
.for_protocol(op_id)?; | ||
.expect("Tx validation mined height is missing"); |
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 do not believe this is an improvement. In my mind the previous code that returned an error was better, but still not ideal.
I would prefer that we just log an error message and continue to the next last mined transaction should we have data inconsistencies. This way the protocol will not stop and do an endless repeat should we have a single row with problematic data.
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 removed this as the TMS defines a mined transaction as a tx with a mined_height
and mined_block
.
The above query we ask to get us a transaction with these properties explicitly asks the db for this.
See:
tari/base_layer/wallet/src/transaction_service/storage/sqlite_db.rs
Lines 1069 to 1075 in 92ceef9
let tx = completed_transactions::table | |
// Note: Check 'mined_in_block' as well as 'mined_height' is populated for faux transactions before it is confirmed | |
.filter(completed_transactions::mined_in_block.is_not_null()) | |
.filter(completed_transactions::mined_height.is_not_null()) | |
.filter(completed_transactions::mined_height.gt(0)) | |
.order_by(completed_transactions::mined_height.desc()) | |
.first::<CompletedTransactionSql>(&*conn) |
So the only reason this can be None is if somehow the memory got corrupted between asking the db and us now getting the value.
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 can't be an expect
let mut _lock = validation_in_progress.try_lock().map_err(|_| { | ||
debug!( | ||
target: LOG_TARGET, | ||
"Transaction Validation Protocol (Id: {}) spawned while a previous protocol was busy, ignored", id | ||
); | ||
TransactionServiceProtocolError::new(id, TransactionServiceError::TransactionValidationInProgress) | ||
})?; |
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 agree with this safeguard.
How does a user manually trigger a validation? |
Description
Adds a mutex on the TMS validation process to stop multiple validations running per time.
Motivation and Context
Validation on the transactions is run when the Base_node state changes, which is an automated event triggered when a base_node lags, a new block is received, or the wallet connects to a new base node. The other path for this is user triggered where the user manually triggers a validation.
Triggering more than one validation at a time is a waste of resources and network traffic as the wallet now asks the same basenode for the same data more than once.