Update `pallet-relayer` to use Contraints V2 #433

HCastano · 2023-10-17T21:36:21Z

This updates the Relayer pallet to use the new Contraints API.

I split this PR out from #428 since I figured it would be better to ensure
that the new flow was working first and then we could go ahead and get
rid of the old code.

Otherwise this will just fail once we try and write the program to the contraint pallet storage.

vercel · 2023-10-17T21:36:25Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
entropy-core	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 20, 2023 5:52pm

pallets/relayer/src/lib.rs

This is now scaled on the length of the submitted program

HCastano · 2023-10-18T18:59:42Z

pallets/relayer/src/weights.rs

+	/// Storage: Constraints AllowedToModifyConstraints (r:0 w:1)
+	/// Proof Skipped: Constraints AllowedToModifyConstraints (max_values: None, max_size: None, mode: Measured)
+	/// The range of component `c` is `[0, 2]`.
+	fn confirm_register_registered(_c: u32, ) -> Weight {


Don't know why c isn't used here anymore...I didn't touch these benches 🤔

HCastano · 2023-10-18T19:01:49Z

runtime/src/weights/pallet_relayer.rs

@@ -59,13 +60,15 @@ impl<T: frame_system::Config> pallet_relayer::WeightInfo for WeightInfo<T> {
 	/// Storage: StakingExtension SigningGroups (r:1 w:0)
 	/// Proof Skipped: StakingExtension SigningGroups (max_values: None, max_size: None, mode: Measured)
 	/// The range of component `c` is `[0, 2]`.
-	fn confirm_register_registering(_c: u32, ) -> Weight {
+	fn confirm_register_registering(c: u32, ) -> Weight {


Hmm but c is used in this bench...ya not sure what's going on here

Jesse suggested this hack...

HCastano · 2023-10-18T21:56:07Z

crypto/server/src/user/tests.rs

@@ -461,7 +461,7 @@ async fn test_store_share() {
    let alice_account_id: <EntropyConfig as Config>::AccountId = alice.to_account_id().into();
    let registered_query = entropy::storage().relayer().registered(alice_account_id);
    for _ in 0..10 {
-        std::thread::sleep(std::time::Duration::from_millis(500));
+        std::thread::sleep(std::time::Duration::from_millis(1000));


@JesseAbram lol this actually fixed the CI 🙈

Filed #438

* Remove `is_swapping` field from registration details * Remove related benchmarking code * Update node metadata * Apply same sleep hack as in #433 🙈

HCastano · 2023-10-20T16:52:55Z

pallets/relayer/src/lib.rs

-                (evm_acl_len, btc_acl_len) = ConstraintsPallet::<T>::constraint_weight_values(constraints);
-            }
-            <T as Config>::WeightInfo::register(evm_acl_len, btc_acl_len)
+            <T as Config>::WeightInfo::register(initial_program.len() as u32)
        })]
        pub fn register(


FYI @frankiebee @jawndiego

this requires we pass an initial program at registration?

Maybe (pending the discussion below), but mostly that there's a change in field names and the type of a program (Option vs. Vec<u8>).

ameba23

This is perfect in a that it achieves exactly what it set out to do, replacing constraints v1 with program bytecode in the relayer pallet.

But it brings up a design decision which we need to be clear on. Is setting a program optional or required?

Previously we allowed Option<Constraints> and now we require a Vec<u8> but it may be invalid bytecode, eg: empty vector.

At the point of signing, a valid program is currently required. Our tests pass here even though we set an empty program when registering, because we update it with a valid one before signing.

Personally i think setting a program should be optional, but that we should be explicit and use Option<Vec<u8>>. If given, it should be a valid program.

I think we should merge this as it is for now though. Maybe i can make an issue about optional vs. required programs.

JesseAbram

looks good just the one comment

JesseAbram · 2023-10-23T14:34:51Z

pallets/relayer/src/lib.rs

-                        &constraints,
-                    );
-                }
+                ConstraintsPallet::<T>::set_program_unchecked(


I don't like using set unchecked, as in set program we charge a deposit for length of program, here I believe people can post an arbitrary sized program for free

From what I can tell this doesn't change the previous deposit/weight behaviour for this extrinsic.

So the user calling the register extrinsic does get charged by program length (before that was being done with the evm_acl and btc_alc related functions).

The validators never got charged this deposit since Constraints::set_constraints_unchecked() doesn't require a deposit, and the confirm_* weight functions don't account for this either.

If we do want to charge for this some questions come up:

Who pays for this?

In the current workflow it would seem like the validators would, although maybe want the sig_req_account to

Which pallet should handle this?

It should be the Constraints pallet imo, but it might make the workflow a bit trickier

And technically the wouldn't be arbitrarily length program, they can only go up to T::MaxV2BytecodeLength 😛

hmmmm ya that does need to be solved, but we can charge them the deposit in the register extrinsic and then ya do set unchecked later on

HCastano · 2023-10-23T14:45:25Z

But it brings up a design decision which we need to be clear on. Is setting a program optional or required?

Previously we allowed Option<Constraints> and now we require a Vec<u8> but it may be invalid bytecode, eg: empty vector.

My understanding here was that an empty program would be equivalent to having a None previously. This makes sense to me since you can still "run" an empty program, but it would just end up being a no-op (said another way, we can just go straight to signature generation).

At the point of signing, a valid program is currently required. Our tests pass here even though we set an empty program when registering, because we update it with a valid one before signing.

Can you point me to the checks were we validate a program? Like is an empty program considered invalid?

Personally i think setting a program should be optional, but that we should be explicit and use Option<Vec<u8>>. If given, it should be a valid program.

I thought about changing the type to an Option<Vec<u8>>, but then what would the difference between None and a Some(vec![]) be in this case?

I think we should merge this as it is for now though. Maybe i can make an issue about optional vs. required programs.

I'm okay discussing it here or in an issue. Maybe we can also get some feedback from @jakehemmerle.

JesseAbram · 2023-10-23T14:58:14Z

I thought about changing the type to an Option<Vec<u8>>, but then what would the difference between None and a Some(vec![]) be in this case?

When reviewing I thought the same as first, but then I thought about it and you write can do the empty program is the same thing makes sense.

ameba23 · 2023-10-23T15:49:21Z

I thought about changing the type to an Option<Vec<u8>>, but then what would the difference between None and a Some(vec![]) be in this case?

When reviewing I thought the same as first, but then I thought about it and you write can do the empty program is the same thing makes sense.

I just tried running the test with an empty program, and it is not a noop. runtime.evaluate returns an error InvalidByteCode. But we could make it that we only attempt to evaluate the program if the length is greater than zero.

HCastano · 2023-10-23T16:01:04Z

@ameba23 can you tell me what test you're trying to run specifically?

Maybe this is something that needs to be accounted for upstream in the constraints crate

ameba23 · 2023-10-23T16:04:20Z

@ameba23 can you tell me what test you're trying to run specifically?

Maybe this is something that needs to be accounted for upstream in the constraints crate

entropy-core/crypto/server/src/user/tests.rs

Line 96 in 0dbcf5d

async fn test_sign_tx_no_chain() {

jakehemmerle · 2023-10-24T16:25:15Z

@HCastano

Programs can be optional; I think it's fine if users can register without requiring one.

HCastano · 2023-10-24T17:17:33Z

@JesseAbram do you mind if we merge this? I have another PR coming for the storage deposit fixes which I think would be better off standalone instead of being added here.

I'll also create an issue about running empty programs (edit: made entropyxyz/programs#36).

HCastano added 8 commits October 17, 2023 12:06

Use contraints V2 code in Relayer::confirm_register workflow

d6212ad

Update relayer tests

2f451c4

Update server to use new register API

2c35d89

Update Propagation pallet tests

f5e39a5

Clean up docs a bit

c7076ce

Clean up wording

067a5d4

Check that the program length is fine in the relayer pallet.

4eb1842

Otherwise this will just fail once we try and write the program to the contraint pallet storage.

Appease Clippy

8a7ebe3

HCastano commented Oct 17, 2023

View reviewed changes

pallets/relayer/src/lib.rs Outdated Show resolved Hide resolved

RustFmt

cf0ace9

vercel bot deployed to Preview October 17, 2023 22:35 View deployment

HCastano added 4 commits October 18, 2023 13:44

Update field naming in benchmarking

b4a0c49

Update Relayer::register benchmark

6d17b7a

This is now scaled on the length of the submitted program

Update weights.rs file

9f4d82f

Update the other set of weights

444eef8

HCastano commented Oct 18, 2023

View reviewed changes

vercel bot deployed to Preview October 18, 2023 19:27 View deployment

Update time between registrations

996c34d

Jesse suggested this hack...

vercel bot deployed to Preview October 18, 2023 21:54 View deployment

HCastano commented Oct 18, 2023

View reviewed changes

Remove TODO in favour of issue

4f941aa

Filed #438

HCastano marked this pull request as ready for review October 19, 2023 19:32

HCastano requested a review from JesseAbram October 19, 2023 19:32

HCastano added a commit that referenced this pull request Oct 19, 2023

Apply same sleep hack as in #433 🙈

c1e0c19

vercel bot deployed to Preview October 19, 2023 20:32 View deployment

HCastano requested a review from ameba23 October 19, 2023 20:51

HCastano added a commit that referenced this pull request Oct 20, 2023

Remove is_swapping from registration details (#437)

2cb40a5

* Remove `is_swapping` field from registration details * Remove related benchmarking code * Update node metadata * Apply same sleep hack as in #433 🙈

Merge branch 'master' into hc-swap-relayer-to-constraints-v2

1822952

Updates benchmarks

272b24e

HCastano added the Breaks API label Oct 20, 2023

HCastano commented Oct 20, 2023

View reviewed changes

vercel bot deployed to Preview October 20, 2023 17:52 View deployment

ameba23 approved these changes Oct 23, 2023

View reviewed changes

JesseAbram requested changes Oct 23, 2023

View reviewed changes

HCastano mentioned this pull request Oct 24, 2023

Add support for running empty programs entropyxyz/programs#36

Closed

JesseAbram approved these changes Oct 24, 2023

View reviewed changes

HCastano merged commit 6c507dd into master Oct 24, 2023
5 checks passed

HCastano deleted the hc-swap-relayer-to-constraints-v2 branch October 24, 2023 18:12

HCastano mentioned this pull request Oct 24, 2023

Take a storage deposit for programs during registration #447

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `pallet-relayer` to use Contraints V2 #433

Update `pallet-relayer` to use Contraints V2 #433

HCastano commented Oct 17, 2023 •

edited

Loading

vercel bot commented Oct 17, 2023 •

edited

Loading

HCastano Oct 18, 2023

HCastano Oct 18, 2023

HCastano Oct 18, 2023

HCastano Oct 20, 2023

jawndiego Oct 21, 2023

HCastano Oct 23, 2023

ameba23 left a comment •

edited

Loading

JesseAbram left a comment

JesseAbram Oct 23, 2023

HCastano Oct 23, 2023

JesseAbram Oct 23, 2023

HCastano commented Oct 23, 2023

JesseAbram commented Oct 23, 2023

ameba23 commented Oct 23, 2023

HCastano commented Oct 23, 2023

ameba23 commented Oct 23, 2023

jakehemmerle commented Oct 24, 2023

HCastano commented Oct 24, 2023 •

edited

Loading

Update pallet-relayer to use Contraints V2 #433

Update pallet-relayer to use Contraints V2 #433

Conversation

HCastano commented Oct 17, 2023 • edited Loading

vercel bot commented Oct 17, 2023 • 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

ameba23 left a comment • edited Loading

Choose a reason for hiding this comment

JesseAbram left a 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

HCastano commented Oct 23, 2023

JesseAbram commented Oct 23, 2023

ameba23 commented Oct 23, 2023

HCastano commented Oct 23, 2023

ameba23 commented Oct 23, 2023

jakehemmerle commented Oct 24, 2023

HCastano commented Oct 24, 2023 • edited Loading

Update `pallet-relayer` to use Contraints V2 #433

Update `pallet-relayer` to use Contraints V2 #433

HCastano commented Oct 17, 2023 •

edited

Loading

vercel bot commented Oct 17, 2023 •

edited

Loading

ameba23 left a comment •

edited

Loading

HCastano commented Oct 24, 2023 •

edited

Loading