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.

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: tainted transaction reporting #5310

Merged
merged 51 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
8dcf3b1
feat: Handling of tainted transactions
Janislav Oct 1, 2024
97967bf
tests: Added tests
Janislav Oct 2, 2024
6072fee
chore: Extended LpRegistration trait + Added to Ingress/Egress config
Janislav Oct 2, 2024
89caffd
feat: Getting LP refund address
Janislav Oct 2, 2024
8706bd8
feature: ensure by lp and broker + added more tests
Janislav Oct 2, 2024
4cbcde1
refactor: Don't error if tx is tainted
Janislav Oct 4, 2024
cb6d4c3
refactor: Using DoubleMap instead of Map
Janislav Oct 4, 2024
ca6f813
refactor: Using BadOrigin + Added unit test
Janislav Oct 4, 2024
84d5182
refactor: Inline code + Add Deposit Witness to struct
Janislav Oct 7, 2024
161be64
refactor: Extended lp deposit with refund address
Janislav Oct 7, 2024
4ba1ecb
feature: Added benchmark
Janislav Oct 7, 2024
ec4ce2d
chore: Changes to benchmark
Janislav Oct 7, 2024
0af1b50
chore: generated mock weights
Janislav Oct 8, 2024
df4529e
chore: only allow mark transactions for BTC
Janislav Oct 9, 2024
8f835eb
feature: expire tainted transaction
Janislav Oct 9, 2024
8b18adf
test: refactored tests
Janislav Oct 10, 2024
961b0af
chore: Removed unused events
Janislav Oct 10, 2024
46757df
chore: Ensure only by broker
Janislav Oct 10, 2024
d61390c
chore: removed broker from tainted tx struct
Janislav Oct 10, 2024
3e4d901
chore: Only clone owner
Janislav Oct 10, 2024
c76fd40
chore: Moved tx tainted check
Janislav Oct 10, 2024
6373909
Merge branch 'main' into feature/pro-1664/close-deposit-channel
Janislav Oct 11, 2024
19feaf3
feature: Added migration for DepositChannelLookup
Janislav Oct 11, 2024
506db26
refactor: Changed data structure + fixed migrations
Janislav Oct 11, 2024
8d628ec
chore: Handle LP refund address as requirement
Janislav Oct 14, 2024
2cb9bfd
chore: Made clippy happy 🙂
Janislav Oct 14, 2024
07e76f6
chore: don't manipulate storage in place in iteration 🙅‍♂️
Janislav Oct 14, 2024
a7ad89e
test: Added migration test 🧪
Janislav Oct 14, 2024
d290643
Merge branch 'main' into feature/pro-1664/close-deposit-channel
Janislav Oct 14, 2024
b75c29c
chore: changed pallet storage version 📀
Janislav Oct 14, 2024
8f2b133
Merge branch 'main' into feature/pro-1664/close-deposit-channel
Janislav Oct 14, 2024
e9ae4a7
chore: bumped pallet storage version (again)
Janislav Oct 14, 2024
16ddff9
refactor: Changed accounting of expired transactions
Janislav Oct 16, 2024
9299e6e
refactor: using translate for migration
Janislav Oct 16, 2024
44db88a
refactor: using append, refactored test
Janislav Oct 16, 2024
6ddcb2f
feature: Added handling of boost channels
Janislav Oct 17, 2024
041b26c
feature: Marking txs when prewitness and reject when we process the depo
Janislav Oct 18, 2024
d3e78ca
feat: pre-witnessed rejection handling
dandanlen Oct 18, 2024
825c6a0
chore: Fixed logic + added tests
Janislav Oct 18, 2024
13e8841
Merge branch 'main' into feature/pro-1664/close-deposit-channel
Janislav Oct 19, 2024
9c9d636
tests: Refactor/Rearranged tests
Janislav Oct 19, 2024
53edef2
chore: Using SECONDS_PER_BLOCK instead of static block seconds
Janislav Oct 19, 2024
95e88f3
chore: Addressed comments
Janislav Oct 21, 2024
6a70d60
chore: Fixed clippy in CI
Janislav Oct 21, 2024
315b278
chore: update comments
msgmaxim Oct 22, 2024
10cdcdf
fix: don't allow report overwrite
Janislav Oct 22, 2024
326d304
chore: Renamed event
Janislav Oct 22, 2024
af46925
feat: improvements:
dandanlen Oct 22, 2024
7ef3189
Merge branch 'main' into feature/pro-1664/close-deposit-channel
Janislav Oct 22, 2024
083ebfa
fix: migration
Janislav Oct 24, 2024
c45ec0f
fix: made clippy happy
Janislav Oct 24, 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
1 change: 1 addition & 0 deletions engine/src/witness/btc/deposits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ pub mod tests {
address: ScriptPubkey,
) -> DepositChannelDetails<state_chain_runtime::Runtime, BitcoinInstance> {
DepositChannelDetails::<_, BitcoinInstance> {
owner: AccountId32::new([0xab; 32]),
opened_at: 1,
expires_at: 10,
deposit_channel: DepositChannel {
Expand Down
2 changes: 2 additions & 0 deletions state-chain/pallets/cf-ingress-egress/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ mod benchmarks {
DepositChannelLookup::<T, I>::insert(
&deposit_address,
DepositChannelDetails {
owner: account("doogle", 0, 0),
opened_at: block_number,
expires_at: block_number,
deposit_channel:
Expand Down Expand Up @@ -102,6 +103,7 @@ mod benchmarks {
let block_number = TargetChainBlockNumber::<T, I>::benchmark_value();
let mut channel =
DepositChannelDetails::<T, I> {
owner: account("doogle", 0, 0),
opened_at: block_number,
expires_at: block_number,
deposit_channel: DepositChannel::generate_new::<
Expand Down
113 changes: 103 additions & 10 deletions state-chain/pallets/cf-ingress-egress/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,17 @@ use cf_chains::{
SwapOrigin, TransferAssetParams,
};
use cf_primitives::{
Asset, AssetAmount, BasisPoints, Beneficiaries, BoostPoolTier, BroadcastId, ChannelId,
DcaParameters, EgressCounter, EgressId, EpochIndex, ForeignChain, PrewitnessedDepositId,
SwapRequestId, ThresholdSignatureRequestId, TransactionHash, SWAP_DELAY_BLOCKS,
AccountRole, Asset, AssetAmount, BasisPoints, Beneficiaries, BoostPoolTier, BroadcastId,
ChannelId, DcaParameters, EgressCounter, EgressId, EpochIndex, ForeignChain,
PrewitnessedDepositId, SwapRequestId, ThresholdSignatureRequestId, TransactionHash,
SWAP_DELAY_BLOCKS,
};
use cf_runtime_utilities::log_or_panic;
use cf_traits::{
impl_pallet_safe_mode, AccountRoleRegistry, AdjustedFeeEstimationApi, AssetConverter,
AssetWithholding, BalanceApi, BoostApi, Broadcaster, Chainflip, DepositApi, EgressApi,
EpochInfo, FeePayment, FetchesTransfersLimitProvider, GetBlockHeight, IngressEgressFeeApi,
IngressSink, IngressSource, NetworkEnvironmentProvider, OnDeposit, PoolApi,
IngressSink, IngressSource, LpRegistration, NetworkEnvironmentProvider, OnDeposit, PoolApi,
ScheduledEgressDetails, SwapLimitsProvider, SwapRequestHandler, SwapRequestType,
};
use frame_support::{
Expand Down Expand Up @@ -122,6 +123,15 @@ pub enum DepositIgnoredReason {
NotEnoughToPayFees,
}

/// Holds information about a tainted transaction.
#[derive(RuntimeDebug, Eq, PartialEq, Clone, Encode, Decode, TypeInfo)]
pub struct TaintedTransactionDetails<AccountId> {
/// The broker that created the tainted transaction.
pub broker: AccountId,
/// The address we use for refunding.
pub refund_address: Option<ForeignChainAddress>,
dandanlen marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until refund parameters are optional, this has to stay optional. We can change it after this is done.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we can't refund anything if there is no refund address?

Doesn't it make more sense to make it required here, and handle the case where we don't have it before storing this value?

Copy link
Contributor Author

@Janislav Janislav Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it doesn't make any difference other than having to deal with less complex code if we go with the option variant. In the point of the swap flow where we have (or have not) the refund address, it's already too late to do anything else than saving what we have and exit the swap logic. What do you want to save if there is just no address? The solution is to make the refund parameters a requirement, so this can not happen, but since this is not done yet, we should go with an option here and replace it after @msgmaxim has solved this in his PR.

Copy link
Contributor

@msgmaxim msgmaxim Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my preference is that we keep the option in this PR. Then once I change the interface to always require refund address, I will update this code too (and the code won't get compiled if I forget to do this).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just linking this (a suggestion to keep refund address optional). https://linear.app/chainflip/issue/PRO-1702/make-refund-params-not-optional#comment-f29a2edb. If we all agree that this is the right way to do it, would it make sense to this runtime check (if input asset is BTC) in this PR (considering that now would be a local change, and also the fact that we want to release this before vault contract swaps)?

}

/// Cross-chain messaging requests.
#[derive(RuntimeDebug, Eq, PartialEq, Clone, Encode, Decode, TypeInfo, MaxEncodedLen)]
pub(crate) struct CrossChainMessage<C: Chain> {
Expand Down Expand Up @@ -252,14 +262,15 @@ pub mod pallet {
#[derive(CloneNoBound, RuntimeDebug, PartialEq, Eq, Encode, Decode, TypeInfo)]
#[scale_info(skip_type_params(T, I))]
pub struct DepositChannelDetails<T: Config<I>, I: 'static> {
/// The owner of the deposit channel.
pub owner: T::AccountId,
Janislav marked this conversation as resolved.
Show resolved Hide resolved
pub deposit_channel: DepositChannel<T::TargetChain>,
/// The block number at which the deposit channel was opened, expressed as a block number
/// on the external Chain.
pub opened_at: TargetChainBlockNumber<T, I>,
/// The last block on the target chain that the witnessing will witness it in. If funds are
/// sent after this block, they will not be witnessed.
pub expires_at: TargetChainBlockNumber<T, I>,

/// The action to be taken when the DepositChannel is deposited to.
pub action: ChannelAction<T::AccountId>,
/// The boost fee
Expand Down Expand Up @@ -420,6 +431,9 @@ pub mod pallet {

/// For checking if the CCM message passed in is valid.
type CcmValidityChecker: CcmValidityCheck;

/// For accessing the LP refund address.
type LpRefundAddress: LpRegistration<AccountId = Self::AccountId>;
}

/// Lookup table for addresses to corresponding deposit channels.
Expand Down Expand Up @@ -524,6 +538,16 @@ pub mod pallet {
pub type PrewitnessedDepositIdCounter<T: Config<I>, I: 'static = ()> =
StorageValue<_, PrewitnessedDepositId, ValueQuery>;

/// Stores the details of tainted transactions against the deposit details.
#[pallet::storage]
pub type TaintedTransactions<T: Config<I>, I: 'static = ()> = StorageMap<
_,
Twox64Concat,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Twox64Concat,
Blake2_128Concat,

DepositDetails can be freely set by the caller, so we need to be defensive here.

<T::TargetChain as Chain>::DepositDetails,
TaintedTransactionDetails<T::AccountId>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider what happens if same tx is reported twice by different parties.

I think we might need a double-map.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

OptionQuery,
>;

#[pallet::event]
#[pallet::generate_deposit(pub(super) fn deposit_event)]
pub enum Event<T: Config<I>, I: 'static = ()> {
Expand Down Expand Up @@ -701,6 +725,14 @@ pub mod pallet {
InvalidDcaParameters,
/// CCM parameters from a contract swap failed validity check.
InvalidCcm,
/// The deposit channel does not exist.
DepositChannelDoesNotExist,
/// Unauthorized call
Unauthorized,
/// The deposit channel has been tainted and is no longer usable.
DepositChannelTainted,
/// Transaction tainted
TransactionTainted,
}

#[pallet::hooks]
Expand Down Expand Up @@ -1212,6 +1244,26 @@ pub mod pallet {

Ok(())
}

#[pallet::call_index(12)]
#[pallet::weight(10_000)]
pub fn mark_transaction_as_tainted(
origin: OriginFor<T>,
tx_id: <T::TargetChain as Chain>::DepositDetails,
) -> DispatchResult {
let account_id = ensure_signed(origin)?;
let is_broker =
T::AccountRoleRegistry::has_account_role(&account_id, AccountRole::Broker);
let is_lp = T::AccountRoleRegistry::has_account_role(
&account_id,
AccountRole::LiquidityProvider,
);
ensure!(is_broker || is_lp, Error::<T, I>::Unauthorized);
Janislav marked this conversation as resolved.
Show resolved Hide resolved
let tainted_transaction =
TaintedTransactionDetails { broker: account_id, refund_address: None };
TaintedTransactions::<T, I>::insert(tx_id, tainted_transaction);
Ok(())
}
}
}

Expand Down Expand Up @@ -1764,6 +1816,38 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
Ok(action)
}

/// Checks if the transaction is tainted and returns the refund details if it is.
fn check_if_tx_is_tainted(
maybe_tainted_transaction: Option<TaintedTransactionDetails<T::AccountId>>,
deposit_channel_details: &DepositChannelDetails<T, I>,
) -> Result<(), TaintedTransactionDetails<T::AccountId>> {
if let Some(tainted_transaction) = maybe_tainted_transaction {
if tainted_transaction.broker == deposit_channel_details.owner {
let maybe_refund_address = match &deposit_channel_details.action {
ChannelAction::Swap { refund_params, .. } => refund_params
.as_ref()
.map(|refund_params| refund_params.refund_address.clone()),
ChannelAction::CcmTransfer { refund_params, .. } => refund_params
.as_ref()
.map(|refund_params| refund_params.refund_address.clone()),
ChannelAction::LiquidityProvision { .. } =>
T::LpRefundAddress::get_liquidity_refund_address(
&deposit_channel_details.owner,
deposit_channel_details.deposit_channel.asset.into(),
),
};
Err(TaintedTransactionDetails {
broker: tainted_transaction.broker,
refund_address: maybe_refund_address,
})
} else {
Ok(())
}
} else {
Ok(())
}
}

/// Completes a single deposit request.
#[transactional]
fn process_single_deposit(
Expand All @@ -1776,8 +1860,21 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
let deposit_channel_details = DepositChannelLookup::<T, I>::get(&deposit_address)
.ok_or(Error::<T, I>::InvalidDepositAddress)?;

ensure!(
deposit_channel_details.deposit_channel.asset == asset,
Error::<T, I>::AssetMismatch
);

let channel_id = deposit_channel_details.deposit_channel.channel_id;

if let Err(tainted_transaction_details) = Self::check_if_tx_is_tainted(
TaintedTransactions::<T, I>::take(&deposit_details),
&deposit_channel_details,
) {
TaintedTransactions::<T, I>::insert(deposit_details, tainted_transaction_details);
return Err(Error::<T, I>::TransactionTainted.into())
Janislav marked this conversation as resolved.
Show resolved Hide resolved
}

if DepositChannelPool::<T, I>::get(channel_id).is_some() {
log_or_panic!(
"Deposit channel {} should not be in the recycled address pool if it's active",
Expand All @@ -1787,11 +1884,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
return Err(Error::<T, I>::InvalidDepositAddress.into())
}

ensure!(
deposit_channel_details.deposit_channel.asset == asset,
Error::<T, I>::AssetMismatch
);

// TODO: only apply this check if the deposit hasn't been boosted
// already (in case MinimumDeposit increases after some small deposit
// is boosted)?
Expand Down Expand Up @@ -2016,6 +2108,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
DepositChannelLookup::<T, I>::insert(
&deposit_address,
DepositChannelDetails {
owner: requester.clone(),
deposit_channel,
opened_at: current_height,
expires_at: expiry_height,
Expand Down
3 changes: 2 additions & 1 deletion state-chain/pallets/cf-ingress-egress/src/mock_btc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use cf_traits::{
api_call::{MockBitcoinApiCall, MockBtcEnvironment},
asset_converter::MockAssetConverter,
asset_withholding::MockAssetWithholding,
balance_api::MockBalance,
balance_api::{MockBalance, MockLpRegistration},
broadcaster::MockBroadcaster,
chain_tracking::ChainTracker,
fee_payment::MockFeePayment,
Expand Down Expand Up @@ -130,6 +130,7 @@ impl pallet_cf_ingress_egress::Config for Test {
type SafeMode = MockRuntimeSafeMode;
type SwapLimitsProvider = MockSwapLimitsProvider;
type CcmValidityChecker = cf_chains::ccm_checker::CcmValidityChecker;
type LpRefundAddress = MockLpRegistration;
}

impl_test_helpers! {
Expand Down
3 changes: 2 additions & 1 deletion state-chain/pallets/cf-ingress-egress/src/mock_eth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use cf_traits::{
api_call::{MockEthereumApiCall, MockEvmEnvironment},
asset_converter::MockAssetConverter,
asset_withholding::MockAssetWithholding,
balance_api::MockBalance,
balance_api::{MockBalance, MockLpRegistration},
broadcaster::MockBroadcaster,
chain_tracking::ChainTracker,
fee_payment::MockFeePayment,
Expand Down Expand Up @@ -136,6 +136,7 @@ impl crate::Config for Test {
type SafeMode = MockRuntimeSafeMode;
type SwapLimitsProvider = MockSwapLimitsProvider;
type CcmValidityChecker = cf_chains::ccm_checker::CcmValidityChecker;
type LpRefundAddress = MockLpRegistration;
}

pub const ALICE: <Test as frame_system::Config>::AccountId = 123u64;
Expand Down
Loading
Loading