Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ProposerFactory limits to the cli #893

Merged
merged 6 commits into from
Mar 29, 2023
Merged

Add ProposerFactory limits to the cli #893

merged 6 commits into from
Mar 29, 2023

Conversation

0x7CFE
Copy link
Contributor

@0x7CFE 0x7CFE commented Mar 26, 2023

Pull Request Summary

Check list

  • added or updated unit tests
  • updated Astar official documentation
  • added OnRuntimeUpgrade hook for precompile revert code registration
  • updated spec version
  • updated semver

@0x7CFE 0x7CFE requested a review from akru March 26, 2023 11:35

/// Proposer's maximum block size limit in bytes
#[clap(long)]
pub proposer_block_size_limit: Option<usize>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What’s about to have default values (based on relay expectations)?

Copy link
Contributor Author

@0x7CFE 0x7CFE Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For DEFAULT_BLOCK_SIZE_LIMIT this is doable. Unfortunately DEFAULT_SOFT_DEADLINE_PERCENT is not public. Of course, I can set the default_value by hand, but it would not be great IMO.

Copy link
Contributor Author

@0x7CFE 0x7CFE Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, sure, we can do something like this:

    /// Proposer's maximum block size limit in bytes
    #[clap(long, default_value = sc_basic_authorship::DEFAULT_BLOCK_SIZE_LIMIT)]
    pub proposer_block_size_limit: usize,

    /// Proposer's soft deadline in percents of block size
    #[clap(long, default_value = 50)]
    pub proposer_soft_deadline_percent: u8,

But that can be problematic if Parity would change the constant under the hood (say to 75) but we would still pass the original value by default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good up to me, let's use it

@@ -1,6 +1,6 @@
[package]
name = "astar-collator"
version = "5.1.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should bump all the runtime versions too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0x7CFE please fix it, double check and it looks collator version keep 5.1 while runtimes are 5.2

bin/collator/src/command.rs Outdated Show resolved Hide resolved
@akru
Copy link
Contributor

akru commented Mar 28, 2023

@0x7CFE please call cargo update before finish it

@akru
Copy link
Contributor

akru commented Mar 28, 2023

Because of frontier patch update please bump spec versions of runtimes too

@akru akru merged commit 6ba5f3c into master Mar 29, 2023
@akru akru deleted the feature/proposer-limits branch March 29, 2023 02:07
@0x7CFE
Copy link
Contributor Author

0x7CFE commented Mar 29, 2023

@akru why did you merge the branch? I still neither updated the versions nor dependencies 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants