feat: store prewitnessed deposits with id #4496

j4m1ef0rd · 2024-02-07T05:54:11Z

Pull Request

Closes: PRO-1141

Checklist

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

I am confident that the code works.
I have updated documentation where appropriate.

Summary

id counter for prewitnessed deposits
new extrinsic for prewitnessing and executing the call. used only for prewinessing deposits.
- new origin for executing a prewitness call
Change the processing of witnesses to check the origin and either add it as a prewitness or process the witness as before.
Storing the prewitnessed deposits in a double map
Cleanup the prewitnessed deposits in on_idle and when witnessed
Changed some test code to now use the process_deposits extrinsic anymore due to issues with the origin. Instead they just call the process_deposit_witnesses function directly.

codecov · 2024-02-07T06:02:23Z

Codecov Report

Attention: Patch coverage is 72.72727% with 117 lines in your changes are missing coverage. Please review.

Project coverage is 72%. Comparing base (38f4443) to head (89aa3e2).
Report is 10 commits behind head on main.

Files	Patch %	Lines
...ate-chain/pallets/cf-ingress-egress/src/weights.rs	28%	58 Missing ⚠️
state-chain/pallets/cf-witnesser/src/weights.rs	46%	26 Missing ⚠️
state-chain/pallets/cf-witnesser/src/lib.rs	17%	25 Missing ⚠️
engine/src/witness/start.rs	0%	5 Missing ⚠️
state-chain/pallets/cf-ingress-egress/src/lib.rs	97%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #4496    +/-   ##
======================================
- Coverage     73%     72%    -0%     
======================================
  Files        401     401            
  Lines      66210   66580   +370     
  Branches   66210   66580   +370     
======================================
+ Hits       48089   48261   +172     
- Misses     15802   15977   +175     
- Partials    2319    2342    +23

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

engine/src/witness/btc/btc_deposits.rs

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

engine/src/witness/btc/btc_deposits.rs

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

state-chain/TROUBLESHOOTING.md

engine/src/witness/btc.rs

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

engine/src/witness/btc/btc_deposits.rs

kylezs · 2024-02-15T15:01:48Z

Dm'd this to Jamie, but thought I'd post here for visibility too:
I was chatting with @AlastairHolmes about this, and he suggested a nice solution to this if statement in the CFE problem we were trying to avoid, and I agree it's better.
Basically, the CFE code could remain more or less unchanged, and so it would still look like process_call(process_deposits) inside the btc deposits adapter, as it was before.
In the same way the code was before, where we wrap the calls in the prewitness extrinsics, we can create another origin within the witnesser pallet.
Currently we have:

    pub enum RawOrigin {
        HistoricalActiveEpochWitnessThreshold,
        CurrentEpochWitnessThreshold,
    }

Then we can add PrewitnessThreshold or something. What this does is call the extrinsic it wraps, with the PrewitnessThreshold origin - we do something like this already, see dispatch_call in the witness lib.rs . Then inside the process_deposits extrinisc it can do a check if origin is prewitness else if origin is witness etc.

This then keeps the nice CFE structure and makes it clear that there's one set of data we witness (the process_deposits extrinsic). And the rest of the SC is pretty much the same. The two paths can be pulled into separate functions for easier testing and benchmarking and what not.

kylezs

Looks good, let me know when it's cleaned up for a final look over

engine/src/witness/btc/btc_deposits.rs

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

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

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

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

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

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

- Added cleanup when witnessed - Added another test

kylezs

Nice work, few minor comments

kylezs · 2024-02-26T08:47:20Z

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

+
+		#[block]
+		{
+			let _old_deposits = PrewitnessedDeposits::<T, I>::clear_prefix(0, u32::MAX, None);


Better to pull the benchmarked part into a function rather than effectively duping this here and where it's actually called in the running code

and then we can add a simple check that they were removed too

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

kylezs · 2024-02-26T08:51:50Z

Be good if @msgmaxim can review this too - as he'll be building off it, make sure the assumptions are aligned

msgmaxim · 2024-02-27T00:43:37Z

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

@@ -566,12 +590,15 @@ pub mod pallet {
 	impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {
 		/// Recycle addresses if we can
 		fn on_idle(_n: BlockNumberFor<T>, remaining_weight: Weight) -> Weight {
-			let read_write_weight =
-				frame_support::weights::constants::RocksDbWeight::get().reads_writes(1, 1);
+			let mut used_weight = Weight::zero();


I know can_and_cannot_recycle already existed, but can we rename it to something resembling what it does a bit closer? How about take_recyclable_addresses? Also the maximum_recyclable_number paramter could be renamed to maximum_blocks_to_recycle (for example, right now it is not clear whether we are talking about addreses or blocks). can_recycle can also be renamed to addresses_to_recycle so it doesn't look like a boolean.

msgmaxim · 2024-02-27T01:02:07Z

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

-				frame_support::weights::constants::RocksDbWeight::get().reads_writes(1, 1);
+			let mut used_weight = Weight::zero();
+
+			let cleanup_weight = frame_support::weights::constants::RocksDbWeight::get()


Should this be recycle_weigth_per_block? Also, maybe we could comments where reads_writes(2, 2) comes from?

msgmaxim · 2024-02-27T01:11:43Z

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

@@ -582,6 +609,10 @@ pub mod pallet {
 					T::ChainTracking::get_block_height(),
 				)
 			});
+			used_weight = used_weight.saturating_add(


Does reading n elements really counts as n reads? I'm sure, but I would imagine this is more to do with how many lookups by key we do (1 in this case) rather than the size of the data read. (If we do need to count elements, should this be can_recycle elements written, or the number of elements left?)

This used_weight addition is not just for the above mutate, it is for the for loop below, thats why it uses n read/writes. Ill add a comment.

msgmaxim · 2024-02-27T01:25:01Z

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

+			});
+
+			let deposit_channel_details =
+				DepositChannelLookup::<T, I>::get(deposit_address.clone())


You can use a reference here.

msgmaxim · 2024-02-27T01:30:29Z

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

 					}
+					// Cleanup the deposit data


Does having items here indicate that a deposit was pre-witnessed, but never fully witnessed? I wonder if we want to log this somehow.

Yes it does. I think it might only matter if it was boosted.

msgmaxim · 2024-02-27T01:43:45Z

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

+		assert_eq!(
+			PrewitnessedDeposits::<Test>::get(channel_id, prewitnessed_deposit_id),
+			Some(PrewitnessedDeposit { asset: ASSET, amount: DEPOSIT_AMOUNT, deposit_address })
+		);


Is the test until here exactly copies handle_prewitness_deposit? If so, maybe the first test is redundant? Alternatively, maybe you could extract some of the common code into a helper function?

msgmaxim · 2024-02-27T01:50:33Z

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

+		BlockHeightProvider::<MockEthereum>::set_block_height(expiry_block);
+
+		// Run the cleanup
+		IngressEgress::on_idle(1, Weight::MAX);


Looks like 1 isn't used in on_idle, but it is meant to be block height, which you are setting to expiry_block, so I think it would be more correct to use expiry_block here too.

expiry_block is the eth block, but the 1 is the SC block. So it does not need to be the same value. I will set it to default to show that it does not matter for the test.

msgmaxim · 2024-02-27T01:52:09Z

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

+			deposit_details: (),
+		};
+		let deposit_witnesses = vec![deposit_detail.clone()];
+		assert_ok!(IngressEgress::add_prewitnessed_deposits(deposit_witnesses.clone()));


We could cover more logic if we use deposit with different amounts and check that the correct one gets removed.

…ero-liquidity * origin/main: fix: disable try-state checks (#4576) chore: debug solana in CI 🐛 (#4580) refactor: pass tx_ref as an extrinsic parameter (#4579) fix: remove bounded balance check (#4575) Solana: update image to latest tag (#4574) feat: add boost lp account to bouncer and fund it on setup_swaps (#4552) feat: Expose tx_hash on BroadcastSuccess event (#4561) feat: Relative Slippage Limits (PRO-1207) (#4547) chore: disable localnet solana in CI ⏱️ (#4569) feat: store prewitnessed deposits with id (#4496) Feat: Scheduled Swaps Subscription (#4525) # Conflicts: # state-chain/pallets/cf-pools/src/tests.rs

…utxo * origin/main: feat: expose command for broker fee withdrawal (#4581) Chore/fix arbitrum deployment (#4570) Added Range order pool price to the pool_price_v2 rpc call (#4548) fix: just check that the balance after is greater than before (#4587) chore: add origin to ccm failed (#4586) fix: runtime upgrade state check uses AllPalletsWithoutSystem (#4583) chore: add zellic audit to repo (#4585) fix: disable try-state checks (#4576) chore: debug solana in CI 🐛 (#4580) refactor: pass tx_ref as an extrinsic parameter (#4579) fix: remove bounded balance check (#4575) Solana: update image to latest tag (#4574) feat: add boost lp account to bouncer and fund it on setup_swaps (#4552) feat: Expose tx_hash on BroadcastSuccess event (#4561) feat: Relative Slippage Limits (PRO-1207) (#4547) chore: disable localnet solana in CI ⏱️ (#4569) feat: store prewitnessed deposits with id (#4496) Feat: Scheduled Swaps Subscription (#4525)

j4m1ef0rd added CFE State Chain labels Feb 7, 2024

j4m1ef0rd self-assigned this Feb 7, 2024

j4m1ef0rd requested review from a team, jerryafr and niklasnatter and removed request for a team, jerryafr and niklasnatter February 7, 2024 05:55

j4m1ef0rd requested a review from msgmaxim February 7, 2024 08:30

kylezs reviewed Feb 7, 2024

View reviewed changes

engine/src/witness/btc/btc_deposits.rs Outdated Show resolved Hide resolved

state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved

state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved

feat: store prewitnessed deposits with id

79614de

j4m1ef0rd force-pushed the feat/prewitness-deposit-id2 branch from c192a5b to 06ad2f8 Compare February 9, 2024 05:54

kylezs reviewed Feb 9, 2024

View reviewed changes

state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved

state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved

AlastairHolmes reviewed Feb 9, 2024

View reviewed changes

engine/src/witness/btc/btc_deposits.rs Outdated Show resolved Hide resolved

feat: on_idle cleanup and test

5ad7814

j4m1ef0rd force-pushed the feat/prewitness-deposit-id2 branch from 06ad2f8 to 5ad7814 Compare February 12, 2024 06:10

j4m1ef0rd added 2 commits February 13, 2024 11:00

Merge branch 'main' into feat/prewitness-deposit-id2

88ce085

feat: add new events to ingress-egress-tracker

62ca390

j4m1ef0rd commented Feb 13, 2024

View reviewed changes

state-chain/pallets/cf-ingress-egress/src/weights.rs Show resolved Hide resolved

j4m1ef0rd commented Feb 13, 2024

View reviewed changes

state-chain/TROUBLESHOOTING.md Outdated Show resolved Hide resolved

j4m1ef0rd added 3 commits February 13, 2024 17:02

feat: test and benchmark

396af1e

Merge branch 'main' into feat/prewitness-deposit-id2

368f9b2

fix: on_idle used weight

9636772

j4m1ef0rd marked this pull request as ready for review February 15, 2024 00:04

j4m1ef0rd requested a review from a team as a code owner February 15, 2024 00:04

j4m1ef0rd requested review from jerryafr and GabrielBuragev and removed request for a team February 15, 2024 00:04

msgmaxim reviewed Feb 15, 2024

View reviewed changes

AlastairHolmes reviewed Feb 15, 2024

View reviewed changes

engine/src/witness/btc/btc_deposits.rs Outdated Show resolved Hide resolved

kylezs changed the title ~~store prewitnessed deposits with id~~ feat: store prewitnessed deposits with id Feb 15, 2024

j4m1ef0rd marked this pull request as draft February 16, 2024 03:54

j4m1ef0rd added 2 commits February 16, 2024 15:06

refactor: on idle

e0243af

refactor: use custom origin

ad5e789

kylezs reviewed Feb 19, 2024

View reviewed changes

tests: fixing some more tests

ad4059c

j4m1ef0rd commented Feb 20, 2024

View reviewed changes

state-chain/pallets/cf-witnesser/src/lib.rs Show resolved Hide resolved

kylezs reviewed Feb 20, 2024

View reviewed changes

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

j4m1ef0rd added 4 commits February 23, 2024 13:10

Merge branch 'main' into feat/prewitness-deposit-id2

074066d

fix: correct prewitness weight

e763040

test: cleanup on idle test

8c7016c

refactor: boostable -> prewitnessed

d5d38de

- Added cleanup when witnessed - Added another test

j4m1ef0rd marked this pull request as ready for review February 26, 2024 05:47

kylezs approved these changes Feb 26, 2024

View reviewed changes

msgmaxim reviewed Feb 27, 2024

View reviewed changes

refactor: address PR comments

8cc0734

msgmaxim approved these changes Feb 27, 2024

View reviewed changes

fix: ignore clear_prefix returned values

89aa3e2

j4m1ef0rd force-pushed the feat/prewitness-deposit-id2 branch from 3cd3bde to 89aa3e2 Compare February 27, 2024 07:15

j4m1ef0rd merged commit 4d6c7b3 into main Feb 27, 2024
43 checks passed

j4m1ef0rd deleted the feat/prewitness-deposit-id2 branch February 27, 2024 08:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: store prewitnessed deposits with id #4496

feat: store prewitnessed deposits with id #4496

j4m1ef0rd commented Feb 7, 2024 •

edited

Loading

codecov bot commented Feb 7, 2024 •

edited

Loading

kylezs commented Feb 15, 2024

kylezs left a comment

kylezs left a comment

kylezs Feb 26, 2024

kylezs Feb 26, 2024

kylezs commented Feb 26, 2024

msgmaxim Feb 27, 2024

msgmaxim Feb 27, 2024

msgmaxim Feb 27, 2024

j4m1ef0rd Feb 27, 2024

msgmaxim Feb 27, 2024

msgmaxim Feb 27, 2024

j4m1ef0rd Feb 27, 2024

msgmaxim Feb 27, 2024

msgmaxim Feb 27, 2024

j4m1ef0rd Feb 27, 2024

msgmaxim Feb 27, 2024

feat: store prewitnessed deposits with id #4496

feat: store prewitnessed deposits with id #4496

Conversation

j4m1ef0rd commented Feb 7, 2024 • edited Loading

Pull Request

Checklist

Summary

codecov bot commented Feb 7, 2024 • edited Loading

Codecov Report

kylezs commented Feb 15, 2024

kylezs left a comment

Choose a reason for hiding this comment

kylezs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylezs commented Feb 26, 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

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

j4m1ef0rd commented Feb 7, 2024 •

edited

Loading

codecov bot commented Feb 7, 2024 •

edited

Loading