-
Notifications
You must be signed in to change notification settings - Fork 54
feat(dpp): add max_asset_lock_transaction_inputs limit to prevent stuck funds #3491
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
Changes from all commits
3656c69
c11c21a
b7bac81
71b65fb
cb62127
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,47 @@ | ||
| use crate::consensus::basic::BasicError; | ||
| use crate::consensus::ConsensusError; | ||
| use crate::errors::ProtocolError; | ||
| use platform_serialization_derive::{PlatformDeserialize, PlatformSerialize}; | ||
| use thiserror::Error; | ||
|
|
||
| use bincode::{Decode, Encode}; | ||
|
|
||
| #[derive( | ||
| Error, Debug, Clone, Encode, Decode, PlatformSerialize, PlatformDeserialize, PartialEq, | ||
| )] | ||
| #[error("Asset lock transaction has too many inputs: {actual_inputs} (max {max_inputs}). Consolidate UTXOs before creating the asset lock.")] | ||
| #[platform_serialize(unversioned)] | ||
| pub struct IdentityAssetLockTransactionTooManyInputsError { | ||
| /* | ||
|
|
||
| DO NOT CHANGE ORDER OF FIELDS WITHOUT INTRODUCING OF NEW VERSION | ||
|
|
||
| */ | ||
| max_inputs: u16, | ||
| actual_inputs: u16, | ||
| } | ||
|
|
||
| impl IdentityAssetLockTransactionTooManyInputsError { | ||
| pub fn new(actual_inputs: u16, max_inputs: u16) -> Self { | ||
| Self { | ||
| max_inputs, | ||
| actual_inputs, | ||
| } | ||
| } | ||
|
|
||
| pub fn max_inputs(&self) -> u16 { | ||
| self.max_inputs | ||
| } | ||
|
|
||
| pub fn actual_inputs(&self) -> u16 { | ||
| self.actual_inputs | ||
| } | ||
| } | ||
|
|
||
| impl From<IdentityAssetLockTransactionTooManyInputsError> for ConsensusError { | ||
| fn from(err: IdentityAssetLockTransactionTooManyInputsError) -> Self { | ||
| Self::BasicError(BasicError::IdentityAssetLockTransactionTooManyInputsError( | ||
| err, | ||
| )) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||||||||||||
| use crate::consensus::basic::identity::{ | ||||||||||||||||||||||||||
| IdentityAssetLockTransactionOutputNotFoundError, InvalidIdentityAssetLockTransactionError, | ||||||||||||||||||||||||||
| IdentityAssetLockTransactionOutputNotFoundError, | ||||||||||||||||||||||||||
| IdentityAssetLockTransactionTooManyInputsError, InvalidIdentityAssetLockTransactionError, | ||||||||||||||||||||||||||
| InvalidIdentityAssetLockTransactionOutputError, | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| use crate::validation::ConsensusValidationResult; | ||||||||||||||||||||||||||
|
|
@@ -11,7 +12,19 @@ use dashcore::{Transaction, TxOut}; | |||||||||||||||||||||||||
| pub(super) fn validate_asset_lock_transaction_structure_v0( | ||||||||||||||||||||||||||
| transaction: &Transaction, | ||||||||||||||||||||||||||
| output_index: u32, | ||||||||||||||||||||||||||
| max_inputs: u16, | ||||||||||||||||||||||||||
| ) -> ConsensusValidationResult<TxOut> { | ||||||||||||||||||||||||||
| let input_count = transaction.input.len(); | ||||||||||||||||||||||||||
|
Contributor
Author
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 please check if we don't need new version of validate_asset_lock_transaction_structure/v0/mod.rs |
||||||||||||||||||||||||||
| if input_count > max_inputs as usize { | ||||||||||||||||||||||||||
|
Comment on lines
+15
to
+18
Collaborator
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. 🟡 Suggestion:
💡 Suggested change
Suggested change
source: ['claude'] 🤖 Fix this with AI agents |
||||||||||||||||||||||||||
| return ConsensusValidationResult::new_with_error( | ||||||||||||||||||||||||||
| IdentityAssetLockTransactionTooManyInputsError::new( | ||||||||||||||||||||||||||
| u16::try_from(input_count).unwrap_or(u16::MAX), | ||||||||||||||||||||||||||
| max_inputs, | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| .into(), | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
|
Comment on lines
+15
to
+22
Collaborator
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. 🔴 Blocking: The new input-limit check still runs only after the asset lock transaction has already been broadcast This guard rejects oversized asset-lock transactions during source: ['codex'] 🤖 Fix this with AI agents |
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // It must be an Asset Lock Special Transaction | ||||||||||||||||||||||||||
| let Some(TransactionPayload::AssetLockPayloadType(ref payload)) = | ||||||||||||||||||||||||||
| transaction.special_transaction_payload | ||||||||||||||||||||||||||
|
|
@@ -41,3 +54,129 @@ pub(super) fn validate_asset_lock_transaction_structure_v0( | |||||||||||||||||||||||||
| ConsensusValidationResult::new_with_data(output.clone()) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| #[cfg(test)] | ||||||||||||||||||||||||||
| mod tests { | ||||||||||||||||||||||||||
| use super::*; | ||||||||||||||||||||||||||
| use dashcore::secp256k1::rand::thread_rng; | ||||||||||||||||||||||||||
| use dashcore::secp256k1::Secp256k1; | ||||||||||||||||||||||||||
| use dashcore::transaction::special_transaction::asset_lock::AssetLockPayload; | ||||||||||||||||||||||||||
| use dashcore::{Network, OutPoint, PrivateKey, ScriptBuf, TxIn, Txid}; | ||||||||||||||||||||||||||
| use std::str::FromStr; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| fn make_asset_lock_transaction(num_inputs: usize) -> Transaction { | ||||||||||||||||||||||||||
| let secp = Secp256k1::new(); | ||||||||||||||||||||||||||
| let mut rng = thread_rng(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let input_secret_key = dashcore::secp256k1::SecretKey::new(&mut rng); | ||||||||||||||||||||||||||
| let private_key = PrivateKey::new(input_secret_key, Network::Testnet); | ||||||||||||||||||||||||||
| let public_key = private_key.public_key(&secp); | ||||||||||||||||||||||||||
| let public_key_hash = public_key.pubkey_hash(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let secret_key = dashcore::secp256k1::SecretKey::new(&mut rng); | ||||||||||||||||||||||||||
| let one_time_private_key = PrivateKey::new(secret_key, Network::Testnet); | ||||||||||||||||||||||||||
| let one_time_public_key = one_time_private_key.public_key(&secp); | ||||||||||||||||||||||||||
| let one_time_key_hash = one_time_public_key.pubkey_hash(); | ||||||||||||||||||||||||||
|
lklimek marked this conversation as resolved.
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let base_txid = | ||||||||||||||||||||||||||
| Txid::from_str("a477af6b2667c29670467e4e0728b685ee07b240235771862318e29ddbe58458") | ||||||||||||||||||||||||||
| .unwrap(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let inputs: Vec<TxIn> = (0..num_inputs) | ||||||||||||||||||||||||||
| .map(|i| TxIn { | ||||||||||||||||||||||||||
| previous_output: OutPoint::new(base_txid, i as u32), | ||||||||||||||||||||||||||
| script_sig: ScriptBuf::new_p2pkh(&public_key_hash), | ||||||||||||||||||||||||||
| sequence: 0, | ||||||||||||||||||||||||||
| witness: Default::default(), | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| .collect(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let funding_output = TxOut { | ||||||||||||||||||||||||||
| value: 100_000_000, | ||||||||||||||||||||||||||
| script_pubkey: ScriptBuf::new_p2pkh(&one_time_key_hash), | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let burn_output = TxOut { | ||||||||||||||||||||||||||
| value: 100_000_000, | ||||||||||||||||||||||||||
| script_pubkey: ScriptBuf::new_op_return(&[]), | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let payload = TransactionPayload::AssetLockPayloadType(AssetLockPayload { | ||||||||||||||||||||||||||
| version: 0, | ||||||||||||||||||||||||||
| credit_outputs: vec![funding_output], | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Transaction { | ||||||||||||||||||||||||||
| version: 0, | ||||||||||||||||||||||||||
| lock_time: 0, | ||||||||||||||||||||||||||
| input: inputs, | ||||||||||||||||||||||||||
| output: vec![burn_output], | ||||||||||||||||||||||||||
| special_transaction_payload: Some(payload), | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||
| fn should_reject_transaction_with_too_many_inputs() { | ||||||||||||||||||||||||||
| let tx = make_asset_lock_transaction(101); | ||||||||||||||||||||||||||
| let result = validate_asset_lock_transaction_structure_v0(&tx, 0, 100); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| assert!(result.errors.len() == 1); | ||||||||||||||||||||||||||
| let error = &result.errors[0]; | ||||||||||||||||||||||||||
| match error { | ||||||||||||||||||||||||||
| crate::consensus::ConsensusError::BasicError( | ||||||||||||||||||||||||||
| crate::consensus::basic::BasicError::IdentityAssetLockTransactionTooManyInputsError( | ||||||||||||||||||||||||||
| e, | ||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||
| ) => { | ||||||||||||||||||||||||||
| assert_eq!(e.actual_inputs(), 101); | ||||||||||||||||||||||||||
| assert_eq!(e.max_inputs(), 100); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| other => { | ||||||||||||||||||||||||||
| panic!("Expected IdentityAssetLockTransactionTooManyInputsError, got: {other:?}") | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||
| fn should_accept_transaction_at_max_inputs() { | ||||||||||||||||||||||||||
| let tx = make_asset_lock_transaction(100); | ||||||||||||||||||||||||||
| let result = validate_asset_lock_transaction_structure_v0(&tx, 0, 100); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let has_too_many_inputs_error = result.errors.iter().any(|e| { | ||||||||||||||||||||||||||
|
lklimek marked this conversation as resolved.
|
||||||||||||||||||||||||||
| matches!( | ||||||||||||||||||||||||||
| e, | ||||||||||||||||||||||||||
| crate::consensus::ConsensusError::BasicError( | ||||||||||||||||||||||||||
| crate::consensus::basic::BasicError::IdentityAssetLockTransactionTooManyInputsError(_) | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| assert!( | ||||||||||||||||||||||||||
| !has_too_many_inputs_error, | ||||||||||||||||||||||||||
| "Should not reject exactly 100 inputs" | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
| assert!( | ||||||||||||||||||||||||||
| result.is_valid(), | ||||||||||||||||||||||||||
| "Transaction at max inputs should be valid" | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||
| fn should_accept_transaction_with_one_input() { | ||||||||||||||||||||||||||
| let tx = make_asset_lock_transaction(1); | ||||||||||||||||||||||||||
| let result = validate_asset_lock_transaction_structure_v0(&tx, 0, 100); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let has_too_many_inputs_error = result.errors.iter().any(|e| { | ||||||||||||||||||||||||||
| matches!( | ||||||||||||||||||||||||||
| e, | ||||||||||||||||||||||||||
| crate::consensus::ConsensusError::BasicError( | ||||||||||||||||||||||||||
| crate::consensus::basic::BasicError::IdentityAssetLockTransactionTooManyInputsError(_) | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| assert!(!has_too_many_inputs_error, "Should not reject single input"); | ||||||||||||||||||||||||||
| assert!( | ||||||||||||||||||||||||||
| result.is_valid(), | ||||||||||||||||||||||||||
| "Single input transaction should be valid" | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| use dpp::consensus::basic::identity::IdentityAssetLockTransactionTooManyInputsError; | ||
| use dpp::consensus::codes::ErrorWithCode; | ||
| use dpp::consensus::ConsensusError; | ||
|
|
||
| use wasm_bindgen::prelude::*; | ||
|
|
||
| #[wasm_bindgen(js_name=IdentityAssetLockTransactionTooManyInputsError)] | ||
| pub struct IdentityAssetLockTransactionTooManyInputsErrorWasm { | ||
| inner: IdentityAssetLockTransactionTooManyInputsError, | ||
| } | ||
|
|
||
| impl From<&IdentityAssetLockTransactionTooManyInputsError> | ||
| for IdentityAssetLockTransactionTooManyInputsErrorWasm | ||
| { | ||
| fn from(e: &IdentityAssetLockTransactionTooManyInputsError) -> Self { | ||
| Self { inner: e.clone() } | ||
| } | ||
| } | ||
|
|
||
| #[wasm_bindgen(js_class=IdentityAssetLockTransactionTooManyInputsError)] | ||
| impl IdentityAssetLockTransactionTooManyInputsErrorWasm { | ||
| #[wasm_bindgen(js_name=getMaxInputs)] | ||
| pub fn get_max_inputs(&self) -> u16 { | ||
| self.inner.max_inputs() | ||
| } | ||
|
|
||
| #[wasm_bindgen(js_name=getActualInputs)] | ||
| pub fn get_actual_inputs(&self) -> u16 { | ||
| self.inner.actual_inputs() | ||
| } | ||
|
|
||
| #[wasm_bindgen(js_name=getCode)] | ||
| pub fn get_code(&self) -> u32 { | ||
| ConsensusError::from(self.inner.clone()).code() | ||
| } | ||
|
|
||
| #[wasm_bindgen(getter)] | ||
| pub fn message(&self) -> String { | ||
| self.inner.to_string() | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.