Skip to content
Snippets Groups Projects
  1. Jan 13, 2025
  2. Jan 09, 2025
  3. Jan 05, 2025
    • thiolliere's avatar
      Implement cumulus StorageWeightReclaim as wrapping transaction extension +... · 63c73bf6
      thiolliere authored
      Implement cumulus StorageWeightReclaim as wrapping transaction extension + frame system ReclaimWeight (#6140)
      
      (rebasing of https://github.com/paritytech/polkadot-sdk/pull/5234)
      
      ## Issues:
      
      * Transaction extensions have weights and refund weight. So the
      reclaiming of unused weight must happen last in the transaction
      extension pipeline. Currently it is inside `CheckWeight`.
      * cumulus storage weight reclaim transaction extension misses the proof
      size of logic happening prior to itself.
      
      ## Done:
      
      * a new storage `ExtrinsicWeightReclaimed` in frame-system. Any logic
      which attempts to do some reclaim must use this storage to avoid double
      reclaim.
      * a new function `reclaim_weight` in frame-system pallet: info and post
      info in arguments, read the already reclaimed weight, calculate the new
      unused weight from info and post info. do the more accurate reclaim if
      higher.
      * `CheckWeight` is unchanged and still reclaim the weight in post
      dispatch
      * `ReclaimWeight` is a new transaction extension in frame system. For
      solo chains it must be used last in the transactino extension pipeline.
      It does the final most accurate reclaim
      * `StorageWeightReclaim` is moved from cumulus primitives into its own
      pallet (in order to define benchmark) and is changed into a wrapping
      transaction extension.
      It does the recording of proof size and does the reclaim using this
      recording and the info and post info. So parachains don't need to use
      `ReclaimWeight`. But also if they use it, there is no bug.
      
          ```rust
        /// The TransactionExtension to the basic transaction logic.
      pub type TxExtension =
      cumulus_pallet_weight_reclaim::StorageWeightReclaim<
               Runtime,
               (
                       frame_system::CheckNonZeroSender<Runtime>,
                       frame_system::CheckSpecVersion<Runtime>,
                       frame_system::CheckTxVersion<Runtime>,
                       frame_system::CheckGenesis<Runtime>,
                       frame_system::CheckEra<Runtime>,
                       frame_system::CheckNonce<Runtime>,
                       frame_system::CheckWeight<Runtime>,
      pallet_transaction_payment::ChargeTransactionPayment<Runtime>,
                       BridgeRejectObsoleteHeadersAndMessages,
      
      (bridge_to_rococo_config::OnBridgeHubWestendRefundBridgeHubRococoMessages,),
      frame_metadata_hash_extension::CheckMetadataHash<Runtime>,
               ),
        >;
        ```
      
      ---------
      
      Co-authored-by: default avatarGitHub Action <action@github.com>
      Co-authored-by: default avatargeorgepisaltu <52418509+georgepisaltu@users.noreply.github.com>
      Co-authored-by: default avatarOliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
      Co-authored-by: default avatarSebastian Kunert <skunert49@gmail.com>
      Co-authored-by: command-bot <>
  4. Dec 20, 2024
    • Xavier Lau's avatar
      Reorder dependencies' keys (#6967) · a843d15e
      Xavier Lau authored
      
      It doesn't make sense to only reorder the features array.
      
      For example:
      
      This makes it hard for me to compare the dependencies and features,
      especially some crates have a really really long dependencies list.
      ```toml​
      [dependencies]
      c = "*"
      a = "*"
      b = "*"
      
      [features]
      std = [
        "a",
        "b",
        "c",
      ]
      ```
      
      This makes my life easier.
      ```toml​
      [dependencies]
      a = "*"
      b = "*"
      c = "*"
      
      [features]
      std = [
        "a",
        "b",
        "c",
      ]
      ```
      
      ---------
      
      Co-authored-by: default avatarBastian Köcher <git@kchr.de>
      Co-authored-by: command-bot <>
  5. Dec 19, 2024
  6. Dec 13, 2024
    • Alexandru Gheorghe's avatar
      Fix approval-voting canonicalize off by one (#6864) · 2dd2bb5a
      Alexandru Gheorghe authored
      
      Approval voting canonicalize is off by one that means if we are
      finalizing blocks one by one, approval-voting cleans it up every other
      block for example:
      
      - With 1, 2, 3, 4, 5, 6 blocks created, the stored range would be
      StoredBlockRange(1,7)
      - When block 3 is finalized the canonicalize works and StoredBlockRange
      is (4,7)
      - When block 4 is finalized the canonicalize exists early because of the
      `if range.0 > canon_number` break clause, so blocks are not cleaned up.
      - When block 5 is finalized the canonicalize works and StoredBlockRange
      becomes (6,7) and both block 4 and 5 are cleaned up.
      
      The consequences of this is that sometimes we keep block entries around
      after they are finalized, so at restart we consider this blocks and send
      them to approval-distribution.
      
      In most cases this is not a problem, but in the case when finality is
      lagging on restart approval-distribution will receive 4 as being the
      oldest block it needs to work on, and since BlockFinalized is never
      resent for block 4 after restart it won't get the opportunity to clean
      that up. Therefore it will end running approval-distribution aggression
      on block 4, because that is the oldest block it received from
      approval-voting for which it did not see a BlockFinalized signal.
      
      ---------
      
      Signed-off-by: default avatarAlexandru Gheorghe <alexandru.gheorghe@parity.io>
    • Tsvetomir Dimitrov's avatar
      Collation fetching fairness (#4880) · 5153e2b5
      Tsvetomir Dimitrov authored
      
      Related to https://github.com/paritytech/polkadot-sdk/issues/1797
      
      # The problem
      When fetching collations in collator protocol/validator side we need to
      ensure that each parachain has got a fair core time share depending on
      its assignments in the claim queue. This means that the number of
      collations fetched per parachain should ideally be equal to (but
      definitely not bigger than) the number of claims for the particular
      parachain in the claim queue.
      
      # Why the current implementation is not good enough
      The current implementation doesn't guarantee such fairness. For each
      relay parent there is a `waiting_queue` (PerRelayParent -> Collations ->
      waiting_queue) which holds any unfetched collations advertised to the
      validator. The collations are fetched on first in first out principle
      which means that if two parachains share a core and one of the
      parachains is more aggressive it might starve the second parachain. How?
      At each relay parent up to `max_candidate_depth` candidates are accepted
      (enforced in `fn is_seconded_limit_reached`) so if one of the parachains
      is quick enough to fill in the queue with its advertisements the
      validator will never fetch anything from the rest of the parachains
      despite they are scheduled. This doesn't mean that the aggressive
      parachain will occupy all the core time (this is guaranteed by the
      runtime) but it will deny the rest of the parachains sharing the same
      core to have collations backed.
      
      # How to fix it
      The solution I am proposing is to limit fetches and advertisements based
      on the state of the claim queue. At each relay parent the claim queue
      for the core assigned to the validator is fetched. For each parachain a
      fetch limit is calculated (equal to the number of entries in the claim
      queue). Advertisements are not fetched for a parachain which has
      exceeded its claims in the claim queue. This solves the problem with
      aggressive parachains advertising too much collations.
      
      The second part is in collation fetching logic. The collator will keep
      track on which collations it has fetched so far. When a new collation
      needs to be fetched instead of popping the first entry from the
      `waiting_queue` the validator examines the claim queue and looks for the
      earliest claim which hasn't got a corresponding fetch. This way the
      collator will always try to prioritise the most urgent entries.
      
      ## How the 'fair share of coretime' for each parachain is determined?
      Thanks to async backing we can accept more than one candidate per relay
      parent (with some constraints). We also have got the claim queue which
      gives us a hint which parachain will be scheduled next on each core. So
      thanks to the claim queue we can determine the maximum number of claims
      per parachain.
      
      For example the claim queue is [A A A] at relay parent X so we know that
      at relay parent X we can accept three candidates for parachain A. There
      are two things to consider though:
      1. If we accept more than one candidate at relay parent X we are
      claiming the slot of a future relay parent. So accepting two candidates
      for relay parent X means that we are claiming the slot at rp X+1 or rp
      X+2.
      2. At the same time the slot at relay parent X could have been claimed
      by a previous relay parent(s). This means that we need to accept less
      candidates at X or even no candidates.
      
      There are a few cases worth considering:
      1. Slot claimed by previous relay parent.
          CQ @ rp X: [A A A]
          Advertisements at X-1 for para A: 2
          Advertisements at X-2 for para A: 2
      Outcome - at rp X we can accept only 1 advertisement since our slots
      were already claimed.
      2. Slot in our claim queue already claimed at future relay parent
          CQ @ rp X: [A A A]
          Advertisements at X+1 for para A: 1
          Advertisements at X+2 for para A: 1
      Outcome: at rp X we can accept only 1 advertisement since the slots in
      our relay parents were already claimed.
      
      The situation becomes more complicated with multiple leaves (forks).
      Imagine we have got a fork at rp X:
      ```
      CQ @ rp X: [A A A]
      (rp X) -> (rp X+1) -> rp(X+2)
               \-> (rp X+1')
      ```
      Now when we examine the claim queue at RP X we need to consider both
      forks. This means that accepting a candidate at X means that we should
      have a slot for it in *BOTH* leaves. If for example there are three
      candidates accepted at rp X+1' we can't accept any candidates at rp X
      because there will be no slot for it in one of the leaves.
      
      ## How the claims are counted
      There are two solutions for counting the claims at relay parent X:
      1. Keep a state for the claim queue (number of claims and which of them
      are claimed) and look it up when accepting a collation. With this
      approach we need to keep the state up to date with each new
      advertisement and each new leaf update.
      2. Calculate the state of the claim queue on the fly at each
      advertisement. This way we rebuild the state of the claim queue at each
      advertisements.
      
      Solution 1 is hard to implement with forks. There are too many variants
      to keep track of (different state for each leaf) and at the same time we
      might never need to use them. So I decided to go with option 2 -
      building claim queue state on the fly.
      
      To achieve this I've extended `View` from backing_implicit_view to keep
      track of the outer leaves. I've also added a method which accepts a
      relay parent and return all paths from an outer leaf to it. Let's call
      it `paths_to_relay_parent`.
      
      So how the counting works for relay parent X? First we examine the
      number of seconded and pending advertisements (more on pending in a
      second) from relay parent X to relay parent X-N (inclusive) where N is
      the length of the claim queue. Then we use `paths_to_relay_parent` to
      obtain all paths from outer leaves to relay parent X. We calculate the
      claims at relay parents X+1 to X+N (inclusive) for each leaf and get the
      maximum value. This way we guarantee that the candidate at rp X can be
      included in each leaf. This is the state of the claim queue which we use
      to decide if we can fetch one more advertisement at rp X or not.
      
      ## What is a pending advertisement
      I mentioned that we count seconded and pending advertisements at relay
      parent X. A pending advertisement is:
      1. An advertisement which is being fetched right now.
      2. An advertisement pending validation at backing subsystem.
      3. An advertisement blocked for seconding by backing because we don't
      know on of its parent heads.
      
      Any of these is considered a 'pending fetch' and a slot for it is kept.
      All of them are already tracked in `State`.
      
      ---------
      
      Co-authored-by: default avatarMaciej <maciej.zyszkiewicz@parity.io>
      Co-authored-by: command-bot <>
      Co-authored-by: default avatarAlin Dima <alin@parity.io>
  7. Dec 12, 2024
  8. Dec 11, 2024
    • Alexandru Gheorghe's avatar
      Make approval-distribution aggression a bit more robust and less spammy (#6696) · 85dd228d
      Alexandru Gheorghe authored
      
      After finality started lagging on kusama around `2025-11-25 15:55:40`
      nodes started being overloaded with messages and some restarted with
      ```
      Subsystem approval-distribution-subsystem appears unresponsive when sending a message of type polkadot_node_subsystem_types::messages::ApprovalDistributionMessage. origin=polkadot_service::relay_chain_selection::SelectRelayChainInner<sc_client_db::Backend<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32, sp_runtime::traits::BlakeTwo256>, sp_runtime::OpaqueExtrinsic>>, polkadot_overseer::Handle>
      ```
      
      I think this happened because our aggression in the current form is way
      too spammy and create problems in situation where we already constructed
      blocks with a load of candidates to check which what happened around
      `#25933682` before and after. However aggression, does help in the
      nightmare scenario where the network is segmented and sparsely
      connected, so I tend to think we shouldn't completely remove it.
      
      The current configuration is:
      ```
      l1_threshold: Some(16),
      l2_threshold: Some(28),
      resend_unfinalized_period: Some(8),
      ```
      The way aggression works right now :
      1. After L1 is triggered all nodes send all messages they created to all
      the other nodes and all messages they would have they already send
      according to the topology.
      2. Because of resend_unfinalized_period for each block all messages at
      step 1) are sent every 8 blocks, so for example let's say we have blocks
      1 to 24 unfinalized, then at block 25, all messages for block 1, 9 will
      be resent, and consequently at block 26, all messages for block 2, 10
      will be resent, this becomes worse as more blocks are created if backing
      backpressure did not kick in yet. In total this logic makes that each
      node receive 3 * total_number_of messages_per_block
      3. L2 aggression is way too spammy, when L2 aggression is enabled all
      nodes sends all messages of a block on GridXY, that means that all
      messages are received and sent by node at least 2*sqrt(num_validators),
      so on kusama would be 66 * NUM_MESSAGES_AT_FIRST_UNFINALIZED_BLOCK, so
      even with a reasonable number of messages like 10K, which you can have
      if you escalated because of no shows, you end-up sending and receiving
      ~660k messages at once, I think that's what makes the
      approval-distribution to appear unresponsive on some nodes.
      4. Duplicate messages are received by the nodes which turn, mark the
      node as banned, which may create more no-shows.
      
      ## Proposed improvements:
      1. Make L2 trigger way later 28 blocks, instead of 64, this should
      literally the last resort, until then we should try to let the
      approval-voting escalation mechanism to do its things and cover the
      no-shows.
      2. On L1 aggression don't send messages for blocks too far from the
      first_unfinalized there is no point in sending the messages for block
      20, if block 1 is still unfinalized.
      3. On L1 aggression, send messages then back-off for 3 *
      resend_unfinalized_period to give time for everyone to clear up their
      queues.
      4. If aggression is enabled accept duplicate messages from validators
      and don't punish them by reducting their reputation which, which may
      create no-shows.
      
      ---------
      
      Signed-off-by: default avatarAlexandru Gheorghe <alexandru.gheorghe@parity.io>
      Co-authored-by: default avatarAndrei Sandu <54316454+sandreim@users.noreply.github.com>
  9. Dec 10, 2024
    • Alexandru Gheorghe's avatar
      Fix order of resending messages after restart (#6729) · 65a4e5ee
      Alexandru Gheorghe authored
      
      The way we build the messages we need to send to approval-distribution
      can result in a situation where is we have multiple assignments covered
      by a coalesced approval, the messages are sent in this order:
      
      ASSIGNMENT1, APPROVAL, ASSIGNMENT2, because we iterate over each
      candidate and add to the queue of messages both the assignment and the
      approval for that candidate, and when the approval reaches the
      approval-distribution subsystem it won't be imported and gossiped
      because one of the assignment for it is not known.
      
      So in a network where a lot of nodes are restarting at the same time we
      could end up in a situation where a set of the nodes correctly received
      the assignments and approvals before the restart and approve their
      blocks and don't trigger their assignments. The other set of nodes
      should receive the assignments and approvals after the restart, but
      because the approvals never get broacasted anymore because of this bug,
      the only way they could approve is if other nodes start broadcasting
      their assignments.
      
      I think this bug contribute to the reason the network did not recovered
      on `25-11-25 15:55:40` after the restarts.
      
      Tested this scenario with a `zombienet` where `nodes` are finalising
      blocks because of aggression and all nodes are restarted at once and
      confirmed the network lags and doesn't recover before and it does after
      the fix
      
      ---------
      
      Signed-off-by: default avatarAlexandru Gheorghe <alexandru.gheorghe@parity.io>
    • Joseph Zhao's avatar
      Remove AccountKeyring everywhere (#5899) · 311ea438
      Joseph Zhao authored
      
      Close: #5858
      
      ---------
      
      Co-authored-by: default avatarBastian Köcher <git@kchr.de>
  10. Dec 09, 2024
    • Alexandru Gheorghe's avatar
      Fix `Possible bug: Vote import failed` after aggression is enabled (#6690) · da953454
      Alexandru Gheorghe authored
      
      After finality started lagging on kusama around 025-11-25 15:55:40
      validators started seeing ocassionally this log, when importing votes
      covering more than one assignment.
      ```
      Possible bug: Vote import failed
      ```
      
      That happens because the assumption that assignments from the same
      validator would have the same required routing doesn't hold after you
      enabled aggression, so you might end up receiving the first assignment
      then you modify the routing for it in `enable_aggression` then your
      receive the second assignment and the vote covering both assignments, so
      the rouing for the first and second assingment wouldn't match and we
      would fail to import the vote.
      
      From the logs I've seen, I don't think this is the reason the network
      didn't fully recover until the failsafe kicked it, because the votes had
      been already imported in approval-voting before this error.
      
      ---------
      
      Signed-off-by: default avatarAlexandru Gheorghe <alexandru.gheorghe@parity.io>
  11. Dec 03, 2024
  12. Nov 29, 2024
  13. Nov 25, 2024
  14. Nov 19, 2024
  15. Nov 18, 2024
    • Tsvetomir Dimitrov's avatar
      Remove `ProspectiveParachainsMode` usage in backing subsystem (#6215) · 7d5d7202
      Tsvetomir Dimitrov authored
      
      Since async backing parameters runtime api is released on all networks
      the code in backing subsystem can be simplified by removing the usages
      of `ProspectiveParachainsMode` and keeping only the branches of the code
      under `ProspectiveParachainsMode::Enabled`.
      
      The PR does that and reworks the tests in mod.rs to use async backing.
      It's a preparation for
      https://github.com/paritytech/polkadot-sdk/issues/5079
      
      ---------
      
      Co-authored-by: default avatarAlin Dima <alin@parity.io>
      Co-authored-by: command-bot <>
  16. Nov 13, 2024
    • Stephane Gurgenidze's avatar
      backing: improve session buffering for runtime information (#6284) · e617d1d0
      Stephane Gurgenidze authored
      ## Issue
      [[#3421] backing: improve session buffering for runtime
      information](https://github.com/paritytech/polkadot-sdk/issues/3421)
      
      ## Description
      In the current implementation of the backing module, certain pieces of
      information, which remain unchanged throughout a session, are fetched
      multiple times via runtime API calls. The goal of this task was to
      introduce a local cache to store such session-stable information and
      perform the runtime API call only once per session.
      
      This PR implements caching specifically for the validators list, node
      features, executor parameters, minimum backing votes threshold, and
      validator-to-group mapping, which were previously fetched from the
      runtime or computed each time `PerRelayParentState` was built. Now, this
      information is cached and reused within the session.
      
      ## TODO
      * [X] Create a separate struct for per-session caches;
      * [X] Cache validators list;
      * [X] Cache node features;
      * [X] Cache executor parameters;
      * [X] Cache minimum backing votes threshold;
      * [X] Cache validator-to-group mapping;
      * [X] Update tests to reflect these changes;
      * [X] Add prdoc.
      
      ## For the next PR
      Cache validator groups and any other session-stable data (if present).
    • Andrei Eres's avatar
      Remove debug message about pruning active leaves (#6440) · 5aeaa664
      Andrei Eres authored
      
      # Description
      
      The debug message was added to identify a potential memory leak.
      However, recent observations show that pruning works as expected.
      Therefore, it is best to remove this line, as it generates quite
      annoying logs.
      
      
      ## Integration
      
      Doesn't affect downstream projects.
      
      ---------
      
      Co-authored-by: default avatarGitHub Action <action@github.com>
  17. Nov 11, 2024
    • Nazar Mokrynskyi's avatar
      Remove network starter that is no longer needed (#6400) · b601d57a
      Nazar Mokrynskyi authored
      
      # Description
      
      This seems to be an old artifact of the long closed
      https://github.com/paritytech/substrate/issues/6827 that I noticed when
      working on related code earlier.
      
      ## Integration
      
      `NetworkStarter` was removed, simply remove its usage:
      ```diff
      -let (network, system_rpc_tx, tx_handler_controller, start_network, sync_service) =
      +let (network, system_rpc_tx, tx_handler_controller, sync_service) =
          build_network(BuildNetworkParams {
      ...
      -start_network.start_network();
      ```
      
      ## Review Notes
      
      Changes are trivial, the only reason for this to not be accepted is if
      it is desired to not start network automatically for whatever reason, in
      which case the description of network starter needs to change.
      
      # Checklist
      
      * [x] My PR includes a detailed description as outlined in the
      "Description" and its two subsections above.
      * [ ] My PR follows the [labeling requirements](
      
      https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process
      ) of this project (at minimum one label for `T` required)
      * External contributors: ask maintainers to put the right label on your
      PR.
      
      ---------
      
      Co-authored-by: default avatarGitHub Action <action@github.com>
      Co-authored-by: default avatarBastian Köcher <git@kchr.de>
    • Alin Dima's avatar
      fix prospective-parachains best backable chain reversion bug (#6417) · 05ad5475
      Alin Dima authored
      
      Kudos to @EclesioMeloJunior for noticing it 
      
      Also added a regression test for it. The existing unit test was
      exercising only the case where the full chain is reverted
      
      ---------
      
      Co-authored-by: default avatarGitHub Action <action@github.com>
      Co-authored-by: default avatarBastian Köcher <git@kchr.de>
  18. Nov 07, 2024
    • Alin Dima's avatar
      make prospective-parachains debug logs less spammy (#6406) · 2680f20a
      Alin Dima authored
      Fixes https://github.com/paritytech/polkadot-sdk/issues/6172
    • Andrei Eres's avatar
      PVF: drop backing jobs if it is too late (#5616) · 6c8a347a
      Andrei Eres authored
      
      Fixes https://github.com/paritytech/polkadot-sdk/issues/5530
      
      This PR introduces the removal of backing jobs that have been back
      pressured for longer than `allowedAncestryLen`, as these candidates are
      no longer viable.
      
      It is reasonable to expect a result for a backing job execution within
      `allowedAncestryLen` blocks. Therefore, we set the job TTL as a relay
      block number and synchronize the validation host by sending activated
      leaves.
      
      ---------
      
      Co-authored-by: default avatarAndrei Sandu <54316454+sandreim@users.noreply.github.com>
      Co-authored-by: default avatarBranislav Kontur <bkontur@gmail.com>
    • Nazar Mokrynskyi's avatar
      Syncing strategy refactoring (part 3) (#5737) · 12d90524
      Nazar Mokrynskyi authored
      # Description
      
      This is a continuation of
      https://github.com/paritytech/polkadot-sdk/pull/5666 that finally fixes
      https://github.com/paritytech/polkadot-sdk/issues/5333.
      
      This should allow developers to create custom syncing strategies or even
      the whole syncing engine if they so desire. It also moved syncing engine
      creation and addition of corresponding protocol outside
      `build_network_advanced` method, which is something Bastian expressed as
      desired in
      https://github.com/paritytech/polkadot-sdk/issues/5#issuecomment-1700816458
      
      Here I replaced strategy-specific types and methods in `SyncingStrategy`
      trait with generic ones. Specifically `SyncingAction` is now used by all
      strategies instead of strategy-specific types with conversions.
      `StrategyKey` was an enum with a fixed set of options and now replaced
      with an opaque type that strategies create privately and send to upper
      layers as an opaque type. Requests and responses are now handled in a
      generic way regardless of the strategy, which reduced and simplified
      strategy API.
      
      `PolkadotSyncingStrategy` now lives in its dedicated module (had to edit
      .gitignore for this) like other strategies.
      
      `build_network_advanced` takes generic `SyncingService` as an argument
      alongside with a few other low-level types (that can probably be
      extracted in the future as well) without any notion of specifics of the
      way syncing is actually done. All the protocol and tasks are created
      outside and not a part of the network anymore. It still adds a bunch of
      protocols like for light client and some others that should eventually
      be restructured making `build_network_advanced` just building generic
      network and not application-specific protocols handling.
      
      ## Integration
      
      Just like https://github.com/paritytech/polkadot-sdk/pull/5666
      introduced `build_polkadot_syncing_strategy`, this PR introduces
      `build_default_block_downloader`, but for convenience and to avoid
      typical boilerplate a simpler high-level function
      `build_default_syncing_engine` is added that will take care of creating
      typical block downloader, syncing strategy and syncing engine, which is
      what most users will be using going forward. `build_network` towards the
      end of the PR was renamed to `build_network_advanced` and
      `build_network`'s API was reverted to
      pre-https://github.com/paritytech/polkadot-sdk/pull/5666, so most users
      will not see much of a difference during upgrade unless they opt-in to
      use new API.
      
      ## Review Notes
      
      For `StrategyKey` I was thinking about using something like private type
      and then storing `TypeId` inside instead of a static string in it, let
      me know if that would preferred.
      
      The biggest change happened to requests that different strategies make
      and how their responses are handled. The most annoying thing here is
      that block response decoding, in contrast to all other responses, is
      dependent on request. This meant request had to be sent throughout the
      system. While originally `Response` was `Vec<u8>`, I didn't want to
      re-encode/decode request and response just to fit into that API, so I
      ended up with `Box<dyn Any + Send>`. This allows responses to be truly
      generic and each strategy will know how to downcast it back to the
      concrete type when handling the response.
      
      Import queue refactoring was needed to move `SyncingEngine` construction
      out of `build_network` that awkwardly implemented for `SyncingService`,
      but due to `&mut self` wasn't usable on `Arc<SyncingService>` for no
      good reason. `Arc<SyncingService>` itself is of course useless, but
      refactoring to replace it with just `SyncingService` was unfortunately
      rejected in https://github.com/paritytech/polkadot-sdk/pull/5454
      
      As usual I recommend to review this PR as a series of commits instead of
      as the final diff, it'll make more sense that way.
      
      # Checklist
      
      * [x] My PR includes a detailed description as outlined in the
      "Description" and its two subsections above.
      * [x] My PR follows the [labeling requirements](
      
      https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process
      ) of this project (at minimum one label for `T` required)
      * External contributors: ask maintainers to put the right label on your
      PR.
      * [x] I have made corresponding changes to the documentation (if
      applicable)
  19. Nov 06, 2024
  20. Nov 05, 2024
  21. Nov 04, 2024
  22. Oct 31, 2024
  23. Oct 30, 2024
    • Sebastian Kunert's avatar
      Add overhead benchmark to frame-omni-bencher (#5891) · 40547f9f
      Sebastian Kunert authored
      
      # Benchmark Overhead Command for Parachains
      
      This implements the `benchmark overhead` command for parachains. Full
      context is available at:
      https://github.com/paritytech/polkadot-sdk/issues/5303. Previous attempt
      was this https://github.com/paritytech/polkadot-sdk/pull/5283, but here
      we have integration into frame-omni-bencher and improved tooling.
      
      ## Changes Overview
      
      Users are now able to use `frame-omni-bencher` to generate
      `extrinsic_weight.rs` and `block_weight.rs` files for their runtime. The
      core logic for generating these remains untouched; this PR provides
      mostly machinery to make it work for parachains at all.
      
      Similar to the pallet benchmarks, we gain the option to benchmark based
      on just a runtime:
      
      ```
      frame-omni-bencher v1 benchmark overhead --runtime {{runtime}}
      ```
      
      or with a spec:
      
      ```
      frame-omni-bencher v1 benchmark overhead --chain {{spec}} --genesis-builder spec
      ```
      
      In this case, the genesis state is generated from the runtime presets.
      However, it is also possible to use `--chain` and genesis builder `spec`
      to generate the genesis state from the chain spec.
      
      Additionally, we use metadata to perform some checks based on the
      pallets the runtime exposes:
      
      - If we see the `ParaInherent` pallet, we assume that we are dealing
      with a relay chain. This means that we don't need proof recording during
      import (since there is no storage weight).
      - If we detect the `ParachainSystem` pallet, we assume that we are
      dealing with a parachain and take corresponding actions like patching a
      para id into the genesis state.
      
      On the inherent side, I am currently supplying the standard inherents
      every parachain needs.
      
      In the current state, `frame-omni-bencher` supports all system chains.
      In follow-up PRs, we could add additional inherents to increase
      compatibility.
      
      Since we are building a block during the benchmark, we also need to
      build an extrinsic. By default, I am leveraging subxt to build the xt
      dynamically. If a chain is not compatible with the `SubstrateConfig`
      that comes with `subxt`, it can provide a custom extrinsic builder to
      benchmarking-cli. This requires either a custom bencher implementation
      or an integration into the parachains node.
      
      Also cumulus-test-runtime has been migrated to provide genesis configs.
      
      ## Chain Compatibility
      The current version here is compatible with the system chains and common
      substrate chains. The way to go for others would be to customize the
      frame-omni-bencher by providing a custom extrinsicbuilder. I did an
      example implementation that works for mythical:
      https://github.com/skunert/mythical-bencher
      
      ## Follow-Ups
      - After #6040 is finished, we should integrate this here to make the
      tooling truly useful. In the current form, the state is fairly small and
      not representative.
      
      ## How to Review
      I recommend starting from
      [here](https://github.com/paritytech/polkadot-sdk/pull/5891/files#diff-50830ff756b3ac3403b7739d66c9e3a5185dbea550669ca71b28d19c7a2a54ecR264),
      this method is the main entry point for omni-bencher and `polkadot`
      binary.
      
      TBD:
      - [x] PRDoc
      
      ---------
      
      Co-authored-by: default avatarMichal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
  24. Oct 25, 2024
  25. Oct 24, 2024
  26. Oct 22, 2024
    • tmpolaczyk's avatar
      Use bool::then instead of then_some with function calls (#6156) · 6418131a
      tmpolaczyk authored
      
      I noticed that hardware benchmarks are being run even though we pass the
      --no-hardware-benchmarks cli flag. After some debugging, the cause is an
      incorrect usage of the `then_some` method.
      
      From [std
      docs](https://doc.rust-lang.org/std/primitive.bool.html#method.then_some):
      
      > Arguments passed to then_some are eagerly evaluated; if you are
      passing the result of a function call, it is recommended to use
      [then](https://doc.rust-lang.org/std/primitive.bool.html#method.then),
      which is lazily evaluated.
      
      ```rust
      let mut a = 0;
      let mut function_with_side_effects = || { a += 1; };
      
      true.then_some(function_with_side_effects());
      false.then_some(function_with_side_effects());
      
      // `a` is incremented twice because the value passed to `then_some` is
      // evaluated eagerly.
      assert_eq!(a, 2);
      ```
      
      This PR fixes all the similar usages of the `then_some` method across
      the codebase.
      
      polkadot address: 138eUqXvUYT3o4GdbnWQfGRzM8yDWh5Q2eFrFULL7RAXzdWD
      
      ---------
      
      Signed-off-by: default avatarOliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
      Co-authored-by: default avatarShawn Tabrizi <shawntabrizi@gmail.com>
      Co-authored-by: default avatarOliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
    • Egor_P's avatar
      [Backport] Version bumps from stable2409-1 (#6153) · d1cf9963
      Egor_P authored
      This PR backports regular version bumps and prdocs reordering from the
      current stable release back to master
  27. Oct 18, 2024
    • georgepisaltu's avatar
      FRAME: Reintroduce `TransactionExtension` as a replacement for `SignedExtension` (#3685) · b76e91ac
      georgepisaltu authored
      
      Original PR https://github.com/paritytech/polkadot-sdk/pull/2280
      reverted in https://github.com/paritytech/polkadot-sdk/pull/3665
      
      This PR reintroduces the reverted functionality with additional changes,
      related effort
      [here](https://github.com/paritytech/polkadot-sdk/pull/3623).
      Description is copied over from the original PR
      
      First part of [Extrinsic
      Horizon](https://github.com/paritytech/polkadot-sdk/issues/2415)
      
      Introduces a new trait `TransactionExtension` to replace
      `SignedExtension`. Introduce the idea of transactions which obey the
      runtime's extensions and have according Extension data (né Extra data)
      yet do not have hard-coded signatures.
      
      Deprecate the terminology of "Unsigned" when used for
      transactions/extrinsics owing to there now being "proper" unsigned
      transactions which obey the extension framework and "old-style" unsigned
      which do not. Instead we have __*General*__ for the former and
      __*Bare*__ for the latter. (Ultimately, the latter will be phased out as
      a type of transaction, and Bare will only be used for Inherents.)
      
      Types of extrinsic are now therefore:
      - Bare (no hardcoded signature, no Extra data; used to be known as
      "Unsigned")
      - Bare transactions (deprecated): Gossiped, validated with
      `ValidateUnsigned` (deprecated) and the `_bare_compat` bits of
      `TransactionExtension` (deprecated).
        - Inherents: Not gossiped, validated with `ProvideInherent`.
      - Extended (Extra data): Gossiped, validated via `TransactionExtension`.
        - Signed transactions (with a hardcoded signature) in extrinsic v4.
      - General transactions (without a hardcoded signature) in extrinsic v5.
      
      `TransactionExtension` differs from `SignedExtension` because:
      - A signature on the underlying transaction may validly not be present.
      - It may alter the origin during validation.
      - `pre_dispatch` is renamed to `prepare` and need not contain the checks
      present in `validate`.
      - `validate` and `prepare` is passed an `Origin` rather than a
      `AccountId`.
      - `validate` may pass arbitrary information into `prepare` via a new
      user-specifiable type `Val`.
      - `AdditionalSigned`/`additional_signed` is renamed to
      `Implicit`/`implicit`. It is encoded *for the entire transaction* and
      passed in to each extension as a new argument to `validate`. This
      facilitates the ability of extensions to acts as underlying crypto.
      
      There is a new `DispatchTransaction` trait which contains only default
      function impls and is impl'ed for any `TransactionExtension` impler. It
      provides several utility functions which reduce some of the tedium from
      using `TransactionExtension` (indeed, none of its regular functions
      should now need to be called directly).
      
      Three transaction version discriminator ("versions") are now permissible
      (RFC [here](https://github.com/polkadot-fellows/RFCs/pull/84)) in
      extrinsic version 5:
      - 0b00000100 or 0b00000101: Bare (used to be called "Unsigned"):
      contains Signature or Extra (extension data). After bare transactions
      are no longer supported, this will strictly identify an Inherents only.
      Available in both extrinsic versions 4 and 5.
      - 0b10000100: Old-school "Signed" Transaction: contains Signature, Extra
      (extension data) and an extension version byte, introduced as part of
      [RFC99](https://github.com/polkadot-fellows/RFCs/blob/main/text/0099-transaction-extension-version.md).
      Still available as part of extrinsic v4.
      - 0b01000101: New-school "General" Transaction: contains Extra
      (extension data) and an extension version byte, as per RFC99, but no
      Signature. Only available in extrinsic v5.
      
      For the New-school General Transaction, it becomes trivial for authors
      to publish extensions to the mechanism for authorizing an Origin, e.g.
      through new kinds of key-signing schemes, ZK proofs, pallet state,
      mutations over pre-authenticated origins or any combination of the
      above.
      
      `UncheckedExtrinsic` still maintains encode/decode backwards
      compatibility with extrinsic version 4, where the first byte was encoded
      as:
      - 0b00000100 - Unsigned transactions
      - 0b10000100 - Old-school Signed transactions, without the extension
      version byte
      
      Now, `UncheckedExtrinsic` contains a `Preamble` and the actual call. The
      `Preamble` describes the type of extrinsic as follows:
      ```rust
      /// A "header" for extrinsics leading up to the call itself. Determines the type of extrinsic and
      /// holds any necessary specialized data.
      #[derive(Eq, PartialEq, Clone)]
      pub enum Preamble<Address, Signature, Extension> {
      	/// An extrinsic without a signature or any extension. This means it's either an inherent or
      	/// an old-school "Unsigned" (we don't use that terminology any more since it's confusable with
      	/// the general transaction which is without a signature but does have an extension).
      	///
      	/// NOTE: In the future, once we remove `ValidateUnsigned`, this will only serve Inherent
      	/// extrinsics and thus can be renamed to `Inherent`.
      	Bare(ExtrinsicVersion),
      	/// An old-school transaction extrinsic which includes a signature of some hard-coded crypto.
      	/// Available only on extrinsic version 4.
      	Signed(Address, Signature, ExtensionVersion, Extension),
      	/// A new-school transaction extrinsic which does not include a signature by default. The
      	/// origin authorization, through signatures or other means, is performed by the transaction
      	/// extension in this extrinsic. Available starting with extrinsic version 5.
      	General(ExtensionVersion, Extension),
      }
      ```
      
      ## Code Migration
      
      ### NOW: Getting it to build
      
      Wrap your `SignedExtension`s in `AsTransactionExtension`. This should be
      accompanied by renaming your aggregate type in line with the new
      terminology. E.g. Before:
      
      ```rust
      /// The SignedExtension to the basic transaction logic.
      pub type SignedExtra = (
      	/* snip */
      	MySpecialSignedExtension,
      );
      /// Unchecked extrinsic type as expected by this runtime.
      pub type UncheckedExtrinsic =
      	generic::UncheckedExtrinsic<Address, RuntimeCall, Signature, SignedExtra>;
      ```
      
      After:
      
      ```rust
      /// The extension to the basic transaction logic.
      pub type TxExtension = (
      	/* snip */
      	AsTransactionExtension<MySpecialSignedExtension>,
      );
      /// Unchecked extrinsic type as expected by this runtime.
      pub type UncheckedExtrinsic =
      	generic::UncheckedExtrinsic<Address, RuntimeCall, Signature, TxExtension>;
      ```
      
      You'll also need to alter any transaction building logic to add a
      `.into()` to make the conversion happen. E.g. Before:
      
      ```rust
      fn construct_extrinsic(
      		/* snip */
      ) -> UncheckedExtrinsic {
      	let extra: SignedExtra = (
      		/* snip */
      		MySpecialSignedExtension::new(/* snip */),
      	);
      	let payload = SignedPayload::new(call.clone(), extra.clone()).unwrap();
      	let signature = payload.using_encoded(|e| sender.sign(e));
      	UncheckedExtrinsic::new_signed(
      		/* snip */
      		Signature::Sr25519(signature),
      		extra,
      	)
      }
      ```
      
      After:
      
      ```rust
      fn construct_extrinsic(
      		/* snip */
      ) -> UncheckedExtrinsic {
      	let tx_ext: TxExtension = (
      		/* snip */
      		MySpecialSignedExtension::new(/* snip */).into(),
      	);
      	let payload = SignedPayload::new(call.clone(), tx_ext.clone()).unwrap();
      	let signature = payload.using_encoded(|e| sender.sign(e));
      	UncheckedExtrinsic::new_signed(
      		/* snip */
      		Signature::Sr25519(signature),
      		tx_ext,
      	)
      }
      ```
      
      ### SOON: Migrating to `TransactionExtension`
      
      Most `SignedExtension`s can be trivially converted to become a
      `TransactionExtension`. There are a few things to know.
      
      - Instead of a single trait like `SignedExtension`, you should now
      implement two traits individually: `TransactionExtensionBase` and
      `TransactionExtension`.
      - Weights are now a thing and must be provided via the new function `fn
      weight`.
      
      #### `TransactionExtensionBase`
      
      This trait takes care of anything which is not dependent on types
      specific to your runtime, most notably `Call`.
      
      - `AdditionalSigned`/`additional_signed` is renamed to
      `Implicit`/`implicit`.
      - Weight must be returned by implementing the `weight` function. If your
      extension is associated with a pallet, you'll probably want to do this
      via the pallet's existing benchmarking infrastructure.
      
      #### `TransactionExtension`
      
      Generally:
      - `pre_dispatch` is now `prepare` and you *should not reexecute the
      `validate` functionality in there*!
      - You don't get an account ID any more; you get an origin instead. If
      you need to presume an account ID, then you can use the trait function
      `AsSystemOriginSigner::as_system_origin_signer`.
      - You get an additional ticket, similar to `Pre`, called `Val`. This
      defines data which is passed from `validate` into `prepare`. This is
      important since you should not be duplicating logic from `validate` to
      `prepare`, you need a way of passing your working from the former into
      the latter. This is it.
      - This trait takes a `Call` type parameter. `Call` is the runtime call
      type which used to be an associated type; you can just move it to become
      a type parameter for your trait impl.
      - There's no `AccountId` associated type any more. Just remove it.
      
      Regarding `validate`:
      - You get three new parameters in `validate`; all can be ignored when
      migrating from `SignedExtension`.
      - `validate` returns a tuple on success; the second item in the tuple is
      the new ticket type `Self::Val` which gets passed in to `prepare`. If
      you use any information extracted during `validate` (off-chain and
      on-chain, non-mutating) in `prepare` (on-chain, mutating) then you can
      pass it through with this. For the tuple's last item, just return the
      `origin` argument.
      
      Regarding `prepare`:
      - This is renamed from `pre_dispatch`, but there is one change:
      - FUNCTIONALITY TO VALIDATE THE TRANSACTION NEED NOT BE DUPLICATED FROM
      `validate`!!
      - (This is different to `SignedExtension` which was required to run the
      same checks in `pre_dispatch` as in `validate`.)
      
      Regarding `post_dispatch`:
      - Since there are no unsigned transactions handled by
      `TransactionExtension`, `Pre` is always defined, so the first parameter
      is `Self::Pre` rather than `Option<Self::Pre>`.
      
      If you make use of `SignedExtension::validate_unsigned` or
      `SignedExtension::pre_dispatch_unsigned`, then:
      - Just use the regular versions of these functions instead.
      - Have your logic execute in the case that the `origin` is `None`.
      - Ensure your transaction creation logic creates a General Transaction
      rather than a Bare Transaction; this means having to include all
      `TransactionExtension`s' data.
      - `ValidateUnsigned` can still be used (for now) if you need to be able
      to construct transactions which contain none of the extension data,
      however these will be phased out in stage 2 of the Transactions Horizon,
      so you should consider moving to an extension-centric design.
      
      ---------
      
      Signed-off-by: default avatargeorgepisaltu <george.pisaltu@parity.io>
      Co-authored-by: default avatarGuillaume Thiolliere <gui.thiolliere@gmail.com>
      Co-authored-by: default avatarBranislav Kontur <bkontur@gmail.com>