Remove without_storage_info for messages pallet by svyatonik · Pull Request #1487 · paritytech/parity-bridges-common

svyatonik · 2022-07-01T09:06:45Z

related to #1433

Some things are still need to be improved here + it needs testing => draft

serban300

Looks good ! I left a couple of comments, but I would be happy to implement them myself if they make sense. It would help me get more familiar with the project.

modules/messages/src/inbound_lane.rs

modules/messages/src/outbound_lane.rs

serban300 · 2022-07-15T11:32:43Z

primitives/messages/src/lib.rs

-	) -> Option<u32> {
-		let message_nonce_size = 8;
+	/// Returns `None` if size overflows `usize` limits.
+	pub fn encoded_size_hint(relayers_entries: usize, messages_count: usize) -> Option<usize>


It seems that we're converting this result to u32 very often. We could create a helper type (something like SizeHint(Option<usize>)) and define on it a method for bounding to u32.

I would rather add method encoded_size_hint_u32 (or something like that instead). But both options are acceptable :)

Yes, that also works.

serban300 · 2022-07-15T11:41:44Z

bin/millau/runtime/src/rialto_parachain_messages.rs

@@ -134,12 +138,9 @@ impl messages::ThisChainWithMessages for Millau {
 	}

 	fn estimate_delivery_confirmation_transaction() -> MessageTransaction<Weight> {


This method seems to have the same implementation everywhere. Would it make sense to define in on the trait ? Of course we would also need to add MAX_SINGLE_MESSAGE_DELIVERY_CONFIRMATION_TX_WEIGHT, EXTRA_STORAGE_PROOF_SIZE and TX_EXTRA_BYTES to the trait definition.

Yes, I think it worth to have single implementation, thanks! But probably let's not move all chain weight specific constants to the generic trait (ThisChainWithMessages). Some chains still may use different techniques to "estimate" this tx => may even not have such constants. WDYT about:

trait ThisChainWithMessages { ... type DeliveryTransactionEstimation: DeliveryTransactionEstimation; ... } trait DeliveryTransactionEstimation { fn estimate_delivery_confirmation_transaction() -> MessageTransaction<Weight>; } struct BasicDeliveryTransactionEstimation<const MaxDeliverTxWeight:Weight, const ExtraStorageProofSize: u32, const TxExtraBytes: u32>(PhantomData); impl<const MaxDeliverTxWeight:Weight, const ExtraStorageProofSize: u32, const TxExtraBytes: u32> DeliveryTransactionEstimation for BasicDeliveryTransactionEstimation<...> { fn estimate_delivery_confirmation_transaction() -> MessageTransaction<Weight> { // use consts here } }

Sounds good.

serban300 · 2022-07-15T12:00:35Z

modules/messages/src/lib.rs


 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
-	#[pallet::without_storage_info]


For the other pallets could we remove #[pallet::without_storage_info] and use #[pallet::unbounded] on a case by case basis ? I think it would be easier to track the storage variables that are unbounded.

TBH I don't know what are side effects of that :) So I'd wait and see how do they solve similar problem in the paras pallet (here: https://github.com/paritytech/polkadot/blob/master/runtime/parachains/src/paras/mod.rs#L572)

svyatonik · 2022-07-15T12:24:11Z

I left a couple of comments, but I would be happy to implement them myself if they make sense. It would help me get more familiar with the project.

Thanks you for the review! If you're keen on doing that, you're welcome to open PR, of course :) If not - I could do that myself :)

serban300 · 2022-07-15T12:28:29Z

I left a couple of comments, but I would be happy to implement them myself if they make sense. It would help me get more familiar with the project.

Thanks you for the review! If you're keen on doing that, you're welcome to open PR, of course :) If not - I could do that myself :)

Yes, I'd be happy to do that. I'll open a PR. Thanks !

serban300 · 2022-07-15T15:40:46Z

Posted #1511 as a result of the discussion here.

* Remove unused trait implementations Signed-off-by: Serban Iorga <serban@parity.io> * Define encoded_size_hint_u32() Signed-off-by: Serban Iorga <serban@parity.io> * Define TransactionEstimationParams trait Signed-off-by: Serban Iorga <serban@parity.io> * Rework TransactionEstimation Signed-off-by: Serban Iorga <serban@parity.io> * Docs + Renamings Signed-off-by: Serban Iorga <serban@parity.io>

serban300 · 2022-07-18T11:47:48Z

I can't mark this PR as approved, probably because it's already merged. But from my side we can consider it approved.

acatangiu · 2022-07-18T11:57:21Z

I can't mark this PR as approved, probably because it's already merged. But from my side we can consider it approved.

You could just push a commit here #1407 marking said PR as reviewed.

serban300 · 2022-07-18T12:00:45Z

I can't mark this PR as approved, probably because it's already merged. But from my side we can consider it approved.

You could just push a commit here #1407 marking said PR as reviewed.

Oh, ok, makes sense. I'll do that. Thanks !

* Bump jsonrpsee to v0.15.1 Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * Update Cargo.lock Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * Update cargo.lock Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * client: Upadte jsonrpsee-core from relay-chain-interface Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * update substrate & polkadot * update substrate & polkadot Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

* draft: remove without_storage_info for messages pallet * some cleanup

* Remove unused trait implementations Signed-off-by: Serban Iorga <serban@parity.io> * Define encoded_size_hint_u32() Signed-off-by: Serban Iorga <serban@parity.io> * Define TransactionEstimationParams trait Signed-off-by: Serban Iorga <serban@parity.io> * Rework TransactionEstimation Signed-off-by: Serban Iorga <serban@parity.io> * Docs + Renamings Signed-off-by: Serban Iorga <serban@parity.io>

* draft: remove without_storage_info for messages pallet * some cleanup

* Remove unused trait implementations Signed-off-by: Serban Iorga <serban@parity.io> * Define encoded_size_hint_u32() Signed-off-by: Serban Iorga <serban@parity.io> * Define TransactionEstimationParams trait Signed-off-by: Serban Iorga <serban@parity.io> * Rework TransactionEstimation Signed-off-by: Serban Iorga <serban@parity.io> * Docs + Renamings Signed-off-by: Serban Iorga <serban@parity.io>

draft: remove without_storage_info for messages pallet

5df8917

svyatonik added A-chores Something that has to be done, as part of regular maintenance P-Runtime PR-audit-maybe Would be good to have the PR audited, but it doesn't look critical. labels Jul 1, 2022

some cleanup

bdbd087

svyatonik marked this pull request as ready for review July 4, 2022 11:41

svyatonik merged commit 99a8178 into master Jul 4, 2022

svyatonik deleted the remove-without_storage_info-for-messages-pallet branch July 4, 2022 12:05

serban300 reviewed Jul 15, 2022

View reviewed changes

serban300 mentioned this pull request Jul 15, 2022

Follow-up on #1487 #1511

Merged

wuminzhe mentioned this pull request Aug 9, 2022

Sync upstream darwinia-network/darwinia-messages-substrate#174

Closed

30 tasks

serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Mar 27, 2024

Remove without_storage_info for messages pallet (paritytech#1487)

2f5bb0f

* draft: remove without_storage_info for messages pallet * some cleanup

serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024

Remove without_storage_info for messages pallet (paritytech#1487)

0c9fd68

* draft: remove without_storage_info for messages pallet * some cleanup

		@@ -134,12 +138,9 @@ impl messages::ThisChainWithMessages for Millau {
		}

		fn estimate_delivery_confirmation_transaction() -> MessageTransaction<Weight> {

Conversation

svyatonik commented Jul 1, 2022

Uh oh!

serban300 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svyatonik commented Jul 15, 2022

Uh oh!

serban300 commented Jul 15, 2022

Uh oh!

serban300 commented Jul 15, 2022

Uh oh!

serban300 commented Jul 18, 2022

Uh oh!

acatangiu commented Jul 18, 2022

Uh oh!

serban300 commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

serban300 commented Jul 18, 2022 •

edited

Loading