Mark transaction as tainted #5310

Janislav · 2024-10-02T14:41:48Z

Pull Request

Closes: PRO-1664

Checklist

Please conduct a thorough self-review before opening the PR.

I am confident that the code works.
I have written sufficient tests.
I have written and tested required migrations.
I have updated documentation where appropriate.

Summary

Added the functionality to mark a transaction as tainted as a broker or LP.
Check transactions when we process the ingress and stop if the channel is marked as tainted.
Saving tainted transaction details to refund it later.

Non-Breaking changes

If this PR includes non-breaking changes, select the non-breaking label. On merge, CI will automatically cherry-pick the commit to a PR against the release branch.

- Extended DepositChannelDetails with owner field - Added extrinsic to mark transaction as tainted - Handling deposit and save details for a refund if tx is tainted

- Added tests to verify that tainted transactions can get detected for all possible swap types - Added tests to check that txs marked by other brokers are getting ignored

codecov · 2024-10-02T14:54:00Z

Codecov Report

Attention: Patch coverage is 87.54209% with 37 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (33eb5eb) to head (e9ae4a7).

Files with missing lines	Patch %	Lines
state-chain/pallets/cf-ingress-egress/src/lib.rs	66%	15 Missing and 2 partials ⚠️
...ate-chain/pallets/cf-ingress-egress/src/weights.rs	0%	14 Missing ⚠️
state-chain/runtime/src/chainflip.rs	0%	4 Missing ⚠️
...ess/src/migrations/add_owner_to_channel_details.rs	99%	1 Missing ⚠️
state-chain/pallets/cf-lp/src/lib.rs	94%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #5310    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        490     491     +1     
  Lines      85209   85327   +118     
  Branches   85209   85327   +118     
======================================
+ Hits       60659   60682    +23     
- Misses     21839   21922    +83     
- Partials    2711    2723    +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dandanlen

Some early comments.

dandanlen · 2024-10-04T08:46:53Z

state-chain/pallets/cf-ingress-egress/src/lib.rs

+		<T::TargetChain as Chain>::DepositDetails,
+		TaintedTransactionDetails<T::AccountId>,


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

I think we might need a double-map.

state-chain/pallets/cf-ingress-egress/src/lib.rs

state-chain/traits/src/liquidity.rs

- Taking refund address if we open a deposit channel as lp - Extended ChannelAction to take an optional refund address - Removed all dependencies regarding the LpRegistration trait (added last commit) - Refactored tests, benchmarks, etc

Janislav · 2024-10-07T13:26:10Z

Still working on the benchmark. It's written, but there is something odd with the way we generate the weights.

dandanlen · 2024-10-10T09:00:35Z

state-chain/traits/src/lib.rs

@@ -724,6 +724,7 @@ pub trait DepositApi<C: Chain> {
 		lp_account: Self::AccountId,
 		source_asset: C::ChainAsset,
 		boost_fee: BasisPoints,
+		refund_address: Option<ForeignChainAddress>,


This should not be optional.

If the LP hasn't registered a refund address, they can't open a channel, so we should always have this.

state-chain/pallets/cf-lp/src/lib.rs

state-chain/pallets/cf-ingress-egress/src/lib.rs

dandanlen · 2024-10-10T09:19:01Z

state-chain/pallets/cf-ingress-egress/src/lib.rs

+		let is_lp =
+			T::AccountRoleRegistry::has_account_role(&account_id, AccountRole::LiquidityProvider);
+		ensure!(is_broker || is_lp, BadOrigin);


Actually I'm not sure we want/need LPs to mark transactions as tainted. We need their deposit address in case a tx is marked, but in general they won't be marking their own.

We will probably add another account role or similar for the 'reporter' role, but that can be in a future PR. For now we can remove the LP here.

I'd also suggest unwrapping the origin and doing permission checks in the extrinsic rather than in this inner method. Might make testing/benchmarking easier.

"Inner" was a more temporary solution since we only wanted to support BTC for now and that would open a whole new number of problems with tests and benchmarks. Since I think it could make sense to support at least ETH as well, we can do this properly and the inner function warp can disappear. In general, I think we should try to benchmark as close as possible to be on the safe side.

dandanlen · 2024-10-10T09:25:24Z

state-chain/pallets/cf-ingress-egress/src/lib.rs

+			broker: account_id.clone(),
+			refund_address: None,
+			deposit_witness: None,
+			expires_at: <frame_system::Pallet<T>>::block_number()
+				.saturating_add(BlockNumberFor::<T>::from(TAINTED_TX_EXPIRATION_BLOCKS)),
+			marked_for_refund: false,


I think it would be clearer/safer/more efficient if we split this into two storage items:

// in a map of AccountId -> TxId -> TaintedTransactionDetails struct TaintedTransactionDetails { // I don't think we need this, it's already in the key of the map: // reporter: AccountId, expires_at: BlockNumber, } // in a value of Vec<RejectionDetails> struct RejectionDetails { refund_address: Address, deposit_witness: DepositWitness, }

This avoids having to deal with the Options and filter against marked_for_refund etc.

I don't think we need DepositWitness tbh. Definitely not for BTC (tx_id is enough here) and for ETH we need other stuff I (refund_address, asset, amount). Because we need to pass this information to the API call, I think we're going to end up to define a new type for that anyway. I would go for now with something static that works for ETH/BTC and extend the type as we go or make generic for the chain.

state-chain/pallets/cf-ingress-egress/src/lib.rs

dandanlen · 2024-10-10T10:27:40Z

Also please remember to rebase against main to pick up the new compiler version.

Janislav · 2024-10-14T10:31:30Z

state-chain/pallets/cf-ingress-egress/src/lib.rs

+#[derive(RuntimeDebug, PartialEq, Eq, Encode, Decode, TypeInfo, CloneNoBound)]
+#[scale_info(skip_type_params(T, I))]
+pub struct TaintedTransactionDetails<T: Config<I>, I: 'static> {
+	pub refund_address: Option<ForeignChainAddress>,


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

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?

I don't 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, instead, 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 go with an option here and replace it after @msgmaxim has solved this in his PR.

dandanlen

Some minor nits but I think it's nearly ready.

Don't worry about the version bump CI check - should go away when #5327 is merged.

dandanlen · 2024-10-14T15:05:26Z

state-chain/pallets/cf-ingress-egress/src/lib.rs

+			for (account, tx_id, expire_block) in TaintedTransactions::<T, I>::iter() {
+				if expire_block <= now {


I don't like having to iterate through all of the TaintedTransactions just to see if any have expired (99% of the time this will return nothing). It would be good to think of a better way to do this.

dandanlen · 2024-10-14T15:14:16Z

state-chain/pallets/cf-ingress-egress/src/lib.rs

+		let channel_owner = deposit_channel_details.owner.clone();
+
+		if TaintedTransactions::<T, I>::take(channel_owner.clone(), deposit_details.clone())


Suggested change

let channel_owner = deposit_channel_details.owner.clone();

if TaintedTransactions::<T, I>::take(channel_owner.clone(), deposit_details.clone())

if TaintedTransactions::<T, I>::take(&channel_owner, &deposit_details)

dandanlen · 2024-10-14T15:15:50Z

state-chain/pallets/cf-ingress-egress/src/lib.rs

+#[derive(RuntimeDebug, PartialEq, Eq, Encode, Decode, TypeInfo, CloneNoBound)]
+#[scale_info(skip_type_params(T, I))]
+pub struct TaintedTransactionDetails<T: Config<I>, I: 'static> {
+	pub refund_address: Option<ForeignChainAddress>,


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?

dandanlen · 2024-10-14T15:22:33Z

state-chain/pallets/cf-ingress-egress/src/lib.rs

+		_,
+		Identity,
+		T::AccountId,
+		Twox64Concat,


Suggested change

Twox64Concat,

Blake2_128Concat,

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

dandanlen · 2024-10-14T15:29:06Z

state-chain/pallets/cf-ingress-egress/src/migrations/add_owner_to_channel_details.rs

+		for (account, channel_details) in old::DepositChannelLookup::<T, I>::drain() {
+			let dummy_account = T::AccountId::decode(&mut &[0u8; 32][..]).unwrap();


Prefer to use translate rather than drain+insert. According to the docs, inserting during iteration can cause unwanted side effects.

Janislav added 5 commits October 1, 2024 15:32

feat: Handling of tainted transactions

8dcf3b1

- Extended DepositChannelDetails with owner field - Added extrinsic to mark transaction as tainted - Handling deposit and save details for a refund if tx is tainted

tests: Added tests

97967bf

- Added tests to verify that tainted transactions can get detected for all possible swap types - Added tests to check that txs marked by other brokers are getting ignored

chore: Extended LpRegistration trait + Added to Ingress/Egress config

6072fee

feat: Getting LP refund address

89caffd

feature: ensure by lp and broker + added more tests

8706bd8

dandanlen reviewed Oct 4, 2024

View reviewed changes

Janislav added 5 commits October 4, 2024 12:58

refactor: Don't error if tx is tainted

4cbcde1

refactor: Using DoubleMap instead of Map

cb6d4c3

refactor: Using BadOrigin + Added unit test

ca6f813

refactor: Inline code + Add Deposit Witness to struct

84d5182

refactor: Extended lp deposit with refund address

161be64

- Taking refund address if we open a deposit channel as lp - Extended ChannelAction to take an optional refund address - Removed all dependencies regarding the LpRegistration trait (added last commit) - Refactored tests, benchmarks, etc

Janislav marked this pull request as ready for review October 7, 2024 11:12

Janislav requested a review from kylezs as a code owner October 7, 2024 11:12

Janislav added 2 commits October 7, 2024 14:39

feature: Added benchmark

4ba1ecb

chore: Changes to benchmark

ec4ce2d

Janislav added 4 commits October 8, 2024 10:59

chore: generated mock weights

0af1b50

chore: only allow mark transactions for BTC

df4529e

feature: expire tainted transaction

8f835eb

test: refactored tests

8b18adf

dandanlen reviewed Oct 10, 2024

View reviewed changes

Janislav added 7 commits October 10, 2024 15:49

chore: Removed unused events

961b0af

chore: Ensure only by broker

46757df

chore: removed broker from tainted tx struct

d61390c

chore: Only clone owner

3e4d901

chore: Moved tx tainted check

c76fd40

Merge branch 'main' into feature/pro-1664/close-deposit-channel

6373909

feature: Added migration for DepositChannelLookup

19feaf3

Janislav added 3 commits October 11, 2024 18:09

refactor: Changed data structure + fixed migrations

506db26

chore: Handle LP refund address as requirement

8d628ec

chore: Made clippy happy 🙂

2cb9bfd

Janislav commented Oct 14, 2024

View reviewed changes

Janislav added 6 commits October 14, 2024 13:02

chore: don't manipulate storage in place in iteration 🙅‍♂️

07e76f6

test: Added migration test 🧪

a7ad89e

Merge branch 'main' into feature/pro-1664/close-deposit-channel

d290643

chore: changed pallet storage version 📀

b75c29c

Merge branch 'main' into feature/pro-1664/close-deposit-channel

8f2b133

chore: bumped pallet storage version (again)

e9ae4a7

dandanlen reviewed Oct 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark transaction as tainted #5310

Mark transaction as tainted #5310

Janislav commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 2, 2024 •

edited

Loading

dandanlen left a comment

dandanlen Oct 4, 2024

Janislav Oct 4, 2024

Janislav commented Oct 7, 2024

dandanlen Oct 10, 2024

dandanlen Oct 10, 2024

dandanlen Oct 10, 2024

Janislav Oct 10, 2024

dandanlen Oct 10, 2024

Janislav Oct 10, 2024

dandanlen commented Oct 10, 2024

Janislav Oct 14, 2024

dandanlen Oct 14, 2024

Janislav Oct 15, 2024

dandanlen left a comment •

edited

Loading

dandanlen Oct 14, 2024

dandanlen Oct 14, 2024

dandanlen Oct 14, 2024

dandanlen Oct 14, 2024

dandanlen Oct 14, 2024

		<T::TargetChain as Chain>::DepositDetails,
		TaintedTransactionDetails<T::AccountId>,

		for (account, tx_id, expire_block) in TaintedTransactions::<T, I>::iter() {
		if expire_block <= now {

		let channel_owner = deposit_channel_details.owner.clone();

		if TaintedTransactions::<T, I>::take(channel_owner.clone(), deposit_details.clone())

		for (account, channel_details) in old::DepositChannelLookup::<T, I>::drain() {
		let dummy_account = T::AccountId::decode(&mut &[0u8; 32][..]).unwrap();

Mark transaction as tainted #5310

Are you sure you want to change the base?

Mark transaction as tainted #5310

Conversation

Janislav commented Oct 2, 2024 • edited Loading

Pull Request

Checklist

Summary

Non-Breaking changes

codecov bot commented Oct 2, 2024 • edited Loading

Codecov Report

dandanlen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janislav commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandanlen commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandanlen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janislav commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 2, 2024 •

edited

Loading

dandanlen left a comment •

edited

Loading