Temp pr to review pubsub more efficiently #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Work in progress pubsub for cross-chain state monitoring. Quick and dirty approach to validate e2e flow viability. Needs polish: tests, benchmarks, error handling, cleanup.
…kadot-sdk into feat/pubsub-polish
Anny0nn
commented
Oct 7, 2025
Comment on lines
+1
to
+33
| // Copyright (C) Parity Technologies (UK) Ltd. | ||
| // This file is part of Polkadot. | ||
|
|
||
| // Polkadot is free software: you can redistribute it and/or modify | ||
| // it under the terms of the GNU General Public License as published by | ||
| // the Free Software Foundation, either version 3 of the License, or | ||
| // (at your option) any later version. | ||
|
|
||
| // Polkadot is distributed in the hope that it will be useful, | ||
| // but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| // GNU General Public License for more details. | ||
|
|
||
| // You should have received a copy of the GNU General Public License | ||
| // along with Polkadot. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| //! Traits for publish/subscribe operations in the broadcaster pallet. | ||
| use alloc::vec::Vec; | ||
| use polkadot_primitives::Id as ParaId; | ||
| use sp_runtime::DispatchResult; | ||
|
|
||
| /// Trait for handling publish and subscribe operations for parachains. | ||
| /// | ||
| /// This trait provides the interface for parachains to publish key-value data | ||
| /// and manage subscriptions to other parachains' published data. | ||
| pub trait PublishSubscribe { | ||
| /// Publish key-value data for a specific parachain. | ||
| fn publish_data(publisher: ParaId, data: Vec<(Vec<u8>, Vec<u8>)>) -> DispatchResult; | ||
|
|
||
| /// Toggle subscription to a publisher's data. | ||
| fn toggle_subscription(subscriber: ParaId, publisher: ParaId) -> DispatchResult; | ||
| } |
Author
There was a problem hiding this comment.
all good with placing the trait in traits.rs under src quite a few crates do the same and most of them are also just trait definitions with no default implementations.
./substrate/frame/conviction-voting/src/traits.rs
./substrate/frame/support/src/traits.rs
./substrate/frame/election-provider-support/src/traits.rs
./substrate/primitives/core/src/traits.rs
./substrate/primitives/arithmetic/src/traits.rs
./substrate/primitives/application-crypto/src/traits.rs
./substrate/primitives/npos-elections/src/traits.rs
./substrate/client/network/src/service/traits.rs
./bridges/snowbridge/primitives/inbound-queue/src/v2/traits.rs
./bridges/relays/client-substrate/src/client/traits.rs
./polkadot/xcm/src/v5/traits.rs
./polkadot/xcm/src/v4/traits.rs
./polkadot/xcm/src/v3/traits.rs
./polkadot/runtime/common/src/traits.rs
Anny0nn
commented
Oct 7, 2025
Comment on lines
+55
to
+62
| /// - `p`: Number of publishers with changed data | ||
| /// - `k`: Number of key-value pairs per publisher | ||
| /// - `v`: Size of each value in bytes | ||
| #[benchmark] | ||
| fn process_published_data( | ||
| p: Linear<1, 100>, | ||
| k: Linear<1, 16>, | ||
| v: Linear<1, 1024>, |
Author
There was a problem hiding this comment.
Are they supposed to stay hard coded or become configs?
Anny0nn
commented
Oct 8, 2025
Comment on lines
+1373
to
+1379
| let should_update = match previous_roots.get(publisher) { | ||
| Some(prev_root) => match current_roots_map.get(publisher) { | ||
| Some(curr_root) if prev_root == curr_root => false, | ||
| _ => true, | ||
| }, | ||
| None => true, | ||
| }; |
Author
There was a problem hiding this comment.
Just an idea to simplify the nested match statements.
let should_update = previous_roots
.get(publisher)
.and_then(|prev| current_roots_map.get(publisher).map(|curr| prev != curr))
.unwrap_or(true);
Anny0nn
commented
Oct 8, 2025
| }; | ||
|
|
||
| if should_update { | ||
| let result = PublishedData::<T>::clear_prefix(publisher, u32::MAX, None); |
Author
There was a problem hiding this comment.
is it fine to have a u32::MAX be the limit limit? can't it cause weight issues with large datasets?
Anny0nn
commented
Oct 8, 2025
Comment on lines
1356
to
1364
| let k = published_data.values() | ||
| .map(|entries| entries.len() as u32) | ||
| .max() | ||
| .unwrap_or(0); | ||
| let v = published_data.values() | ||
| .flat_map(|entries| entries.iter()) | ||
| .map(|(_, value)| value.len() as u32) | ||
| .max() | ||
| .unwrap_or(0); |
Author
There was a problem hiding this comment.
This iterates stuff twice because .max() consumes the iterator, using fold you can remove one of the iterations while still getting the same result.
let (k, v) = published_data.values().fold((0u32, 0u32), |(max_k, max_v), entries| {
let k = entries.len() as u32;
let v = entries.iter().map(|(_, value)| value.len() as u32).max().unwrap_or(0);
(max_k.max(k), max_v.max(v))
});
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.