Add signature verification + additional validation in xcm-core-buyer

[ParthDesai]

Description

This PR adds signature verification in the xcm-core-buyer. So that only collator can purchase the core. Apart from that the pallet now only allows collator to buy core if it is inline with the slot frequency.

Changes are as follows:

1. xcm-core-buyer now takes into account slot frequency.
2. couple of hooks added in xcm-core-buyer to get current slot and latest author info from other pallets.
3. xcm-core-buyer now do signature verification in validate_unsigned for buy_core
4. two runtime api exposed: 1. Get slot frequency 2. Check whether a client is allowed to buy core or not which can be used by client to decide whether it should buy core or not.

Remaining:

• Benchmarks
• Tests

[tmpolaczyk]

Why do we need a nonce? Can't we use the block number as the nonce for example?

[ParthDesai]

Hmm. Yeah. We can use block number as nonce. But problem is that it is not guaranteed at which block this tx will be included and any tx signed with earlier block number can become invalid.

So, the idea is to have nonce just like a substrate account.

[tmpolaczyk]

I don't like that it's an extra storage map. But block number won't work, because the tx may not be included in the next block, you're right. Container chain block number (from author-noting pallet) also won't work, because in case of failure we won't be able to retry... There must be something that we can use though.

[ParthDesai]

It is not extra storage map. It's just one storage value at the moment.

I think it is nice trade off between amount of space and the uncertainity.

[tmpolaczyk]

Oh, really? Sorry, I haven't look into the code yet, but what will happen if two collators try to buy a core in the same block? Will it work or will it fail?

[ParthDesai]

One of them would succeed and other one would fail. That is sort of drawback. This is the reason why I wanted it originally as storage map.

On second thought, let me modify it as storage map first and then we can arrive at equivalent solution later on.

[tmpolaczyk]

Yeah I agree, let's work with a storage map now, write some tests to make sure everything works, and then we can think about how to remove this storage map.

[github-actions]

Coverage Report

(master)

@@                        Coverage Diff                        @@
##           master   add-parathread-support-part-2      +/-   ##
=================================================================
+ Coverage   69.19%                          69.31%   +0.12%     
+ Files         221                             223       +2     
+ Lines       39049                           39193     +144     
=================================================================
+ Hits        27017                           27165     +148     
- Misses      12032                           12028       -4     

Files Changed	Coverage
/client/consensus/src/lib.rs	28.68% (-0.23%)	🔽
/pallets/author-noting/src/lib.rs	86.57% (+0.19%)	🔼
/pallets/xcm-core-buyer/src/lib.rs	91.08% (+10.16%)	🔼
/pallets/xcm-core-buyer/src/weights.rs	6.34% (-0.92%)	🔽
/primitives/traits/src/lib.rs	58.14% (+11.71%)	🔼
/runtime/dancebox/src/lib.rs	89.30% (-0.09%)	🔽
/runtime/dancebox/src/weights/pallet_xcm_core_buyer.rs	49.30% (+7.36%)	🔼
/runtime/dancebox/src/xcm_config.rs	83.67% (+2.90%)	🔼

Coverage generated Thu May 30 11:45:02 UTC 2024

[tmpolaczyk]

I would make the fields public instead of having setters

[ParthDesai]

I only want to make the fields publicly accessible for testing. Can I do that?

Reason being is that if you mutate this struct, it will become invalid as either public key or signature changes.

[tmpolaczyk]

For types that can be deserialized from bytes, such as this one that implements Decode, you must assume they can be in an invalid state anyway, so I don't see much benefit from having private fields.

[ParthDesai]

Oh. I did not meant it for validation. Verify function will return false in that case anyway.

My idea was to indicate anybody who is using this structure in non-test environment to be able to just call new and get valid instance which they cannot modify as any change there can render the struct invalid and call to fail. So, basically reduce the error surface for anybody working with the struct.

[ParthDesai]

@tmpolaczyk Now, the fields are public. :)

[tmpolaczyk]

Cool, thank you

[girazoki]

Suggested change

	Slot::from(2u64),
	Slot::from(2u64),

this should be a config parameter probably no?

[ParthDesai]

yes. I have added to-do for that.

[girazoki]

I general I like the approach taken but I am missing a bunch of integration tests (with the xcm-emulator, if possible). Any other tests we can run @tmpolaczyk? I feel like any dev-test wont work so at most we can do the zombienet one you have already.

To adapt the zombienet test we are going to need to:

• register the parathread as a parathread in the relay (not as a parachain, as we are doing now)
• fund the account in the relay with tokens
• check the block is more or less being produced at a consistent rate.

Also I have not seen the client-change related to the change of the runtime-api (I think instead of slot_frequency it's called some other thing now), so we should probably modify that too

[tmpolaczyk]

I general I like the approach taken but I am missing a bunch of integration tests (with the xcm-emulator, if possible). Any other tests we can run @tmpolaczyk? I feel like any dev-test wont work so at most we can do the zombienet one you have already.

We don't have any zombienet tests? I see the one in test/suites/dev-tanssi/xcm-core-buyer/test_xcm_core_buyer.ts, which is a dev test, and we can only use it to test that the xcm message has been sent, (because dev test = no relay chain), unless we mock the response somehow.

[girazoki]

I think it looks great now, great job! I have a few questions though:

• We have in our xcm-emulator tests tests that use force_buy_core. How difficult would it be to implement one that uses buy_core? I am not sure which collators do we set there but if they are collators that are backed by a private key (e.g., Alice) we could do this right?

• The zombienet parathread test version can be changed once the node changes are implemented

[ParthDesai]

I think it looks great now, great job! I have a few questions though:

• We have in our xcm-emulator tests tests that use force_buy_core. How difficult would it be to implement one that uses buy_core? I am not sure which collators do we set there but if they are collators that are backed by a private key (e.g., Alice) we could do this right?

It is not much of effort to replace force_buy_core with buy_core. But there is no point as buy_core is equivalent to force_buy_core with extra validation for sender but has no effect on xcm execution.

• The zombienet parathread test version can be changed once the node changes are implemented

Sure.

[tmpolaczyk]

I added the block number to be able to do

.and_provides((block_number, para_id))

because without that, only a single unsigned transaction would ever be included by the block builder. Now that we have the nonce we probably don't need it. But I don't know a good alternative, (para_id, nonce) won't work because the nonce can be the same for different collators... So probably it's better to just leave it like it is. And unfortunately this is going to be very hard to test...