Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: utxo consolidation works without prev key #4903

Merged
merged 4 commits into from
May 30, 2024
Merged

fix: utxo consolidation works without prev key #4903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
76c6406
fix: utxo consolidation works without prev key
msgmaxim May 27, 2024
fc000f6
fix: do not consolidate stale utxos
msgmaxim May 28, 2024
6dee7f1
test: update utxo consolidation tests
msgmaxim May 28, 2024
30070ec
Merge branch 'main' into fix/consolidate-no-prev-key
msgmaxim May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 20 additions & 38 deletions state-chain/chains/src/btc/utxo_selection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ pub fn select_utxos_from_pool(
/// Utxos that have a zero (or below) net value (amount - fees) are ignored. They will eventually
/// become stale if the fees do not come down, and is discarded when they are no longer usable.
pub fn select_utxos_for_consolidation(
current_key: [u8; 32],
previous_key: Option<[u8; 32]>,
available_utxos: &mut Vec<Utxo>,
fee_info: &BitcoinFeeInfo,
consolidation_parameter: ConsolidationParameters,
Expand All @@ -141,15 +141,20 @@ pub fn select_utxos_for_consolidation(
// Transfer old utxos into the current vault. Remaining utxos must be > consolidation_size.
// Utxos are pre-filtered. Only utxos that belong to current or previous vaults are
// remaining.
let old_utxos = spendable
.extract_if(|utxo: &mut Utxo| utxo.deposit_address.pubkey_x != current_key)
.take(consolidation_parameter.consolidation_size as usize)
.collect::<Vec<_>>();

let prev_key_utxos = previous_key
.map(|prev_key| {
spendable
.extract_if(|utxo: &mut Utxo| utxo.deposit_address.pubkey_x == prev_key)
.take(consolidation_parameter.consolidation_size as usize)
.collect::<Vec<_>>()
})
.unwrap_or_default();

available_utxos.append(&mut spendable);
available_utxos.append(&mut dust);

old_utxos
prev_key_utxos
}
}

Expand Down Expand Up @@ -334,11 +339,11 @@ mod tests {
}

#[test]
fn test_consolidation_path_1() {
fn should_consolidate_due_to_reaching_threshold() {
let key1 = [0xAA; 32];
let key2 = [0xBB; 32];
let fee_info = BitcoinFeeInfo { sats_per_kilobyte: 1_000 };
let parameter = ConsolidationParameters::new(4, 2);
let params = ConsolidationParameters::new(4, 2);

let mut utxos = vec![
build_utxo(1, 0, Some(key1)),
Expand All @@ -347,8 +352,9 @@ mod tests {
build_utxo(3_000, 0, Some(key1)),
build_utxo(4_000, 0, Some(key2)),
];

assert_eq!(
select_utxos_for_consolidation(key1, &mut utxos, &fee_info, parameter,),
select_utxos_for_consolidation(Some(key2), &mut utxos, &fee_info, params,),
vec![build_utxo(1_000, 0, Some(key1)), build_utxo(2_000, 0, Some(key2))]
);
assert_eq!(
Expand All @@ -362,34 +368,7 @@ mod tests {
}

#[test]
fn test_consolidation_no_consolidation() {
let key1 = [0xAA; 32];
let fee_info = BitcoinFeeInfo { sats_per_kilobyte: 1_000 };
let parameter = ConsolidationParameters::new(10, 2);

let mut utxos = vec![
build_utxo(1, 0, Some(key1)),
build_utxo(1_000, 0, Some(key1)),
build_utxo(2_000, 0, Some(key1)),
build_utxo(3_000, 0, Some(key1)),
build_utxo(4_000, 0, Some(key1)),
];
assert_eq!(select_utxos_for_consolidation(key1, &mut utxos, &fee_info, parameter,), vec![]);

assert_eq!(
utxos,
vec![
build_utxo(1_000, 0, Some(key1)),
build_utxo(2_000, 0, Some(key1)),
build_utxo(3_000, 0, Some(key1)),
build_utxo(4_000, 0, Some(key1)),
build_utxo(1, 0, Some(key1)),
]
);
}

#[test]
fn test_consolidation_partial_previous_utxos() {
fn should_consolidate_previous_utxo_without_reaching_threshold() {
let key1 = [0xAA; 32];
let key2 = [0xBB; 32];
let fee_info = BitcoinFeeInfo { sats_per_kilobyte: 1_000 };
Expand All @@ -402,11 +381,14 @@ mod tests {
build_utxo(3_000, 0, Some(key2)),
build_utxo(4_000, 0, Some(key2)),
];

// Only previous key utxos should be selected:
assert_eq!(
select_utxos_for_consolidation(key1, &mut utxos, &fee_info, parameter,),
select_utxos_for_consolidation(Some(key2), &mut utxos, &fee_info, parameter,),
vec![build_utxo(2_000, 0, Some(key2)), build_utxo(3_000, 0, Some(key2)),]
);

// Note that some of previous key utxos still remain (due to consolidation size limit):
assert_eq!(
utxos,
vec![
Expand Down
26 changes: 15 additions & 11 deletions state-chain/pallets/cf-environment/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,23 +509,27 @@ impl<T: Config> Pallet<T> {
UtxoSelectionType::SelectForConsolidation =>
BitcoinAvailableUtxos::<T>::mutate(|available_utxos| {
if let Some(cf_traits::EpochKey {
key: AggKey { previous: Some(prev_key), current: current_key },
key: AggKey { previous, current: current_key },
..
}) = T::BitcoinKeyProvider::active_epoch_key()
{
let stale = available_utxos
.extract_if(|utxo| {
utxo.deposit_address.pubkey_x != current_key &&
utxo.deposit_address.pubkey_x != prev_key
})
.collect::<Vec<_>>();

if !stale.is_empty() {
Self::deposit_event(Event::<T>::StaleUtxosDiscarded { utxos: stale });
if let Some(prev_key) = previous {
let stale = available_utxos
.extract_if(|utxo| {
utxo.deposit_address.pubkey_x != current_key &&
utxo.deposit_address.pubkey_x != prev_key
})
.collect::<Vec<_>>();

if !stale.is_empty() {
Self::deposit_event(Event::<T>::StaleUtxosDiscarded {
utxos: stale,
});
}
}

let selected_utxo = select_utxos_for_consolidation(
current_key,
previous,
available_utxos,
&bitcoin_fee_info,
Self::consolidation_parameters(),
Expand Down
72 changes: 24 additions & 48 deletions state-chain/pallets/cf-environment/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,19 +161,18 @@ fn can_discard_stale_utxos() {
let epoch_3 = [0xBB; 32];
let epoch_4 = [0xDD; 32];
new_test_ext().execute_with(|| {
// Do not discard utxo if `previous` key is not set.
MockBitcoinKeyProvider::add_key(AggKey { current: epoch_2, previous: None });
ConsolidationParameters::<Test>::set(utxo_selection::ConsolidationParameters {
consolidation_threshold: 5,
consolidation_size: 2,
});

// Does not discard UTXOs if previous key not set:
MockBitcoinKeyProvider::set_key(AggKey { current: epoch_2, previous: None });

BitcoinAvailableUtxos::<Test>::set(vec![
utxo_with_key(epoch_1),
utxo_with_key(epoch_1),
utxo_with_key(epoch_1),
utxo_with_key(epoch_3),
utxo_with_key(epoch_3),
utxo_with_key(epoch_2),
utxo_with_key(epoch_3),
]);

Expand All @@ -182,25 +181,32 @@ fn can_discard_stale_utxos() {
None
);

// No consolidation. Epoch 1 utxos are discarded
MockBitcoinKeyProvider::add_key(AggKey { current: epoch_3, previous: Some(epoch_2) });
// Have not reached threshold, but will still move previous epoch UTXO into the new vault
// as part of "consolidation". Epoch 1 utxos are discarded.
MockBitcoinKeyProvider::set_key(AggKey { current: epoch_3, previous: Some(epoch_2) });
assert_eq!(
Environment::select_and_take_bitcoin_utxos(UtxoSelectionType::SelectForConsolidation),
None
Environment::select_and_take_bitcoin_utxos(UtxoSelectionType::SelectForConsolidation,)
.unwrap()
.0,
vec![utxo_with_key(epoch_2)]
);

System::assert_has_event(RuntimeEvent::Environment(crate::Event::StaleUtxosDiscarded {
utxos: vec![utxo_with_key(epoch_1), utxo_with_key(epoch_1), utxo_with_key(epoch_1)],
utxos: vec![utxo_with_key(epoch_1), utxo_with_key(epoch_1)],
}));

// Can consolidate and discard at the same time
// Can "consolidate" and discard at the same time
BitcoinAvailableUtxos::<Test>::append(utxo_with_key(epoch_1));

MockBitcoinKeyProvider::add_key(AggKey { current: epoch_4, previous: Some(epoch_3) });
assert!(Environment::select_and_take_bitcoin_utxos(
UtxoSelectionType::SelectForConsolidation
)
.is_some());
MockBitcoinKeyProvider::set_key(AggKey { current: epoch_4, previous: Some(epoch_3) });

assert_eq!(
Environment::select_and_take_bitcoin_utxos(UtxoSelectionType::SelectForConsolidation,)
.unwrap()
.0,
vec![utxo_with_key(epoch_3)]
);

System::assert_has_event(RuntimeEvent::Environment(crate::Event::StaleUtxosDiscarded {
utxos: vec![utxo_with_key(epoch_1)],
}));
Expand All @@ -213,7 +219,7 @@ fn can_consolidate_current_and_prev_utxos() {
let epoch_2 = [0xBB; 32];
const CONSOLIDATION_SIZE: u32 = 4;
new_test_ext().execute_with(|| {
MockBitcoinKeyProvider::add_key(AggKey { current: epoch_2, previous: Some(epoch_1) });
MockBitcoinKeyProvider::set_key(AggKey { current: epoch_2, previous: Some(epoch_1) });
ConsolidationParameters::<Test>::set(utxo_selection::ConsolidationParameters {
consolidation_threshold: 5,
consolidation_size: CONSOLIDATION_SIZE,
Expand Down Expand Up @@ -254,7 +260,7 @@ fn can_consolidate_old_utxo_only() {

new_test_ext().execute_with(|| {
// Set current key to epoch 2, and transfer limit to 2 utxo at a time.
MockBitcoinKeyProvider::add_key(AggKey { current: epoch_2, previous: Some(epoch_1) });
MockBitcoinKeyProvider::set_key(AggKey { current: epoch_2, previous: Some(epoch_1) });
ConsolidationParameters::<Test>::set(utxo_selection::ConsolidationParameters {
consolidation_threshold: 10,
consolidation_size: CONSOLIDATION_SIZE,
Expand Down Expand Up @@ -317,36 +323,6 @@ fn do_nothing_with_no_key_set() {
None,
);
assert_eq!(crate::BitcoinAvailableUtxos::<Test>::decode_len(), Some(6));

// Set key for current vault.
MockBitcoinKeyProvider::add_key(AggKey { current: epoch_2, previous: None });

assert_eq!(
Environment::select_and_take_bitcoin_utxos(UtxoSelectionType::SelectForConsolidation),
None,
);

assert_eq!(crate::BitcoinAvailableUtxos::<Test>::decode_len(), Some(6));

// Only consolidate and discard stale utxos when previous key is available.
MockBitcoinKeyProvider::add_key(AggKey { current: epoch_3, previous: Some(epoch_2) });

// Utxos from epoch 1 are discarded, those from epoch 2 are consolidated.
assert_eq!(
Environment::select_and_take_bitcoin_utxos(UtxoSelectionType::SelectForConsolidation)
.unwrap()
.0,
vec![utxo_with_key(epoch_2), utxo_with_key(epoch_2),],
);

System::assert_has_event(RuntimeEvent::Environment(crate::Event::StaleUtxosDiscarded {
utxos: vec![utxo_with_key(epoch_1), utxo_with_key(epoch_1)],
}));

assert_eq!(
BitcoinAvailableUtxos::<Test>::get(),
vec![utxo_with_key(epoch_3), utxo_with_key(epoch_3),]
);
});
}

Expand Down
2 changes: 1 addition & 1 deletion state-chain/traits/src/mocks/key_provider.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ impl<C: ChainCrypto> MockPallet for MockKeyProvider<C> {
const EPOCH_KEY: &[u8] = b"EPOCH_KEY";

impl<C: ChainCrypto> MockKeyProvider<C> {
pub fn add_key(key: C::AggKey) {
pub fn set_key(key: C::AggKey) {
Self::put_value(EPOCH_KEY, EpochKey { key, epoch_index: Default::default() });
}
}
Expand Down
Loading