-
Notifications
You must be signed in to change notification settings - Fork 39
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(platform)!: withdrawal automatic retries after core rejection #2185
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,64 @@ | ||||||||||||||||||||
use crate::error::execution::ExecutionError; | ||||||||||||||||||||
use crate::error::Error; | ||||||||||||||||||||
use crate::platform_types::platform::Platform; | ||||||||||||||||||||
|
||||||||||||||||||||
use crate::rpc::core::CoreRPCLike; | ||||||||||||||||||||
use dpp::block::block_info::BlockInfo; | ||||||||||||||||||||
|
||||||||||||||||||||
use crate::platform_types::platform_state::PlatformState; | ||||||||||||||||||||
use dpp::version::PlatformVersion; | ||||||||||||||||||||
use drive::grovedb::Transaction; | ||||||||||||||||||||
|
||||||||||||||||||||
mod v0; | ||||||||||||||||||||
mod v1; | ||||||||||||||||||||
|
||||||||||||||||||||
impl<C> Platform<C> | ||||||||||||||||||||
where | ||||||||||||||||||||
C: CoreRPCLike, | ||||||||||||||||||||
{ | ||||||||||||||||||||
/// Rebroadcasts expired withdrawal documents if any exist. | ||||||||||||||||||||
/// | ||||||||||||||||||||
/// This function attempts to rebroadcast expired withdrawal documents by checking if there are | ||||||||||||||||||||
/// any documents with the status `EXPIRED`. It updates the status of such documents to | ||||||||||||||||||||
/// `BROADCASTED`, increments their revision, and reschedules them for broadcasting. | ||||||||||||||||||||
/// | ||||||||||||||||||||
/// # Parameters | ||||||||||||||||||||
/// - `block_info`: Information about the current block (e.g., timestamp). | ||||||||||||||||||||
/// - `transaction`: The transaction within which the rebroadcast should be executed. | ||||||||||||||||||||
/// - `platform_version`: The version of the platform, used to determine the correct method implementation. | ||||||||||||||||||||
Comment on lines
+25
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add missing documentation for The function documentation is missing a description for the Apply this diff to add the missing parameter documentation: /// # Parameters
/// - `block_info`: Information about the current block (e.g., timestamp).
+/// - `last_committed_platform_state`: The last committed state of the platform.
/// - `transaction`: The transaction within which the rebroadcast should be executed.
/// - `platform_version`: The version of the platform, used to determine the correct method implementation. 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @QuantumExplorer missing param There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||
/// | ||||||||||||||||||||
/// # Returns | ||||||||||||||||||||
/// - `Ok(())` if the rebroadcast process succeeds without issues. | ||||||||||||||||||||
/// - `Err(ExecutionError::UnknownVersionMismatch)` if the platform version is unsupported. | ||||||||||||||||||||
pub fn rebroadcast_expired_withdrawal_documents( | ||||||||||||||||||||
&self, | ||||||||||||||||||||
block_info: &BlockInfo, | ||||||||||||||||||||
last_committed_platform_state: &PlatformState, | ||||||||||||||||||||
transaction: &Transaction, | ||||||||||||||||||||
platform_version: &PlatformVersion, | ||||||||||||||||||||
) -> Result<(), Error> { | ||||||||||||||||||||
match platform_version | ||||||||||||||||||||
.drive_abci | ||||||||||||||||||||
.methods | ||||||||||||||||||||
.withdrawals | ||||||||||||||||||||
.rebroadcast_expired_withdrawal_documents | ||||||||||||||||||||
{ | ||||||||||||||||||||
0 => self.rebroadcast_expired_withdrawal_documents_v0( | ||||||||||||||||||||
block_info, | ||||||||||||||||||||
last_committed_platform_state, | ||||||||||||||||||||
transaction, | ||||||||||||||||||||
platform_version, | ||||||||||||||||||||
), | ||||||||||||||||||||
1 => self.rebroadcast_expired_withdrawal_documents_v1( | ||||||||||||||||||||
block_info, | ||||||||||||||||||||
transaction, | ||||||||||||||||||||
platform_version, | ||||||||||||||||||||
), | ||||||||||||||||||||
version => Err(Error::Execution(ExecutionError::UnknownVersionMismatch { | ||||||||||||||||||||
method: "rebroadcast_expired_withdrawal_documents".to_string(), | ||||||||||||||||||||
known_versions: vec![0, 1], | ||||||||||||||||||||
received: version, | ||||||||||||||||||||
})), | ||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
use crate::error::Error; | ||
use crate::platform_types::platform_state::v0::PlatformStateV0Methods; | ||
use crate::platform_types::platform_state::PlatformState; | ||
use crate::{platform_types::platform::Platform, rpc::core::CoreRPCLike}; | ||
use dpp::block::block_info::BlockInfo; | ||
use dpp::version::PlatformVersion; | ||
use drive::grovedb::Transaction; | ||
|
||
impl<C> Platform<C> | ||
where | ||
C: CoreRPCLike, | ||
{ | ||
pub(super) fn rebroadcast_expired_withdrawal_documents_v0( | ||
&self, | ||
block_info: &BlockInfo, | ||
last_committed_platform_state: &PlatformState, | ||
transaction: &Transaction, | ||
platform_version: &PlatformVersion, | ||
) -> Result<(), Error> { | ||
// Currently Core only supports using the first 2 quorums (out of 24 for mainnet). | ||
// For us, we just use the latest quorum to be extra safe. | ||
let Some(position_of_current_quorum) = | ||
last_committed_platform_state.current_validator_set_position_in_list_by_most_recent() | ||
else { | ||
tracing::warn!("Current quorum not in current validator set, not making withdrawals"); | ||
return Ok(()); | ||
}; | ||
if position_of_current_quorum != 0 { | ||
tracing::debug!( | ||
"Current quorum is not most recent, it is in position {}, not making withdrawals", | ||
position_of_current_quorum | ||
); | ||
return Ok(()); | ||
} | ||
// Version 1 changes on Version 0, by not having the Core 2 Quorum limit. | ||
// Hence we can just use the v1 here after the extra logic of v0 | ||
self.rebroadcast_expired_withdrawal_documents_v1(block_info, transaction, platform_version) | ||
shumkov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,119 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use dpp::block::block_info::BlockInfo; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use dpp::data_contract::accessors::v0::DataContractV0Getters; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use dpp::data_contracts::withdrawals_contract::WithdrawalStatus; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use dpp::document::document_methods::DocumentMethodsV0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use dpp::document::{DocumentV0Getters, DocumentV0Setters}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use dpp::platform_value::btreemap_extensions::BTreeValueMapHelper; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use dpp::system_data_contracts::withdrawals_contract::v1::document_types::withdrawal; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use dpp::version::PlatformVersion; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use std::collections::BTreeSet; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use crate::{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
error::{execution::ExecutionError, Error}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
platform_types::platform::Platform, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rpc::core::CoreRPCLike, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use dpp::withdrawal::WithdrawalTransactionIndex; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use drive::grovedb::Transaction; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use drive::util::batch::DriveOperation; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
impl<C> Platform<C> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
where | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
C: CoreRPCLike, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/// Version 1 changes on Version 0, by not having the Core 2 Quorum limit. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/// We should switch to Version 1 once Core has fixed the issue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pub(super) fn rebroadcast_expired_withdrawal_documents_v1( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
&self, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
block_info: &BlockInfo, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
transaction: &Transaction, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
platform_version: &PlatformVersion, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) -> Result<(), Error> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let expired_withdrawal_documents_to_retry_signing = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.drive.fetch_oldest_withdrawal_documents_by_status( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
WithdrawalStatus::EXPIRED.into(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
platform_version | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.system_limits | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.retry_signing_expired_withdrawal_documents_per_block_limit, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
transaction.into(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
platform_version, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
)?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if expired_withdrawal_documents_to_retry_signing.is_empty() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Ok(()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Collecting unique withdrawal indices of expired documents | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let expired_withdrawal_indices: Vec<WithdrawalTransactionIndex> = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expired_withdrawal_documents_to_retry_signing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.iter() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.map(|document| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
document | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.properties() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.get_optional_u64(withdrawal::properties::TRANSACTION_INDEX)? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.ok_or(Error::Execution(ExecutionError::CorruptedDriveResponse( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"Can't get transaction index from withdrawal document".to_string(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
))) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.collect::<Result<BTreeSet<WithdrawalTransactionIndex>, Error>>()? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.into_iter() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.collect(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+47
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Simplify the collection of unique withdrawal transaction indices The current code for collecting You can streamline the code by directly collecting into a -let expired_withdrawal_indices: Vec<WithdrawalTransactionIndex> =
+let expired_withdrawal_indices: BTreeSet<WithdrawalTransactionIndex> =
expired_withdrawal_documents_to_retry_signing
.iter()
.map(|document| {
document
.properties()
.get_optional_u64(withdrawal::properties::TRANSACTION_INDEX)?
.ok_or_else(|| Error::Execution(ExecutionError::CorruptedDriveResponse(
- "Can't get transaction index from withdrawal document".to_string(),
+ format!("Can't get transaction index from withdrawal document with ID: {}", document.id()),
)))
})
- .collect::<Result<BTreeSet<WithdrawalTransactionIndex>, Error>>()?
- .into_iter()
- .collect();
+ .collect::<Result<_, _>>()?; This refactor eliminates unnecessary conversions and provides clearer error messages with the document ID included. 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need vector There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shumkov, understood. If a vector is required, then it makes sense to keep it as is. Thank you for the clarification. ✏️ Learnings added
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let mut drive_operations: Vec<DriveOperation> = vec![]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Collecting only documents that have been updated | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let mut documents_to_update = Vec::new(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
for mut document in expired_withdrawal_documents_to_retry_signing { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
document.set_u8( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
withdrawal::properties::STATUS, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
WithdrawalStatus::BROADCASTED as u8, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
document.set_updated_at(Some(block_info.time_ms)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
document.increment_revision().map_err(Error::Protocol)?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
documents_to_update.push(document); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if documents_to_update.is_empty() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Ok(()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.drive | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.move_broadcasted_withdrawal_transactions_back_to_queue_operations( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expired_withdrawal_indices, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
&mut drive_operations, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
platform_version, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
)?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+85
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Rename method for clarity regarding expired withdrawals The method Consider renaming the method to |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let withdrawals_contract = self.drive.cache.system_data_contracts.load_withdrawals(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.drive.add_update_multiple_documents_operations( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
&documents_to_update, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
&withdrawals_contract, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
withdrawals_contract | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.document_type_for_name(withdrawal::NAME) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.map_err(|_| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Error::Execution(ExecutionError::CorruptedCodeExecution( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"Can't fetch withdrawal data contract", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
})?, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
&mut drive_operations, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
&platform_version.drive, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
)?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.drive.apply_drive_operations( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
drive_operations, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
block_info, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
transaction.into(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
platform_version, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
)?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
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.
Clippy warnings