Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Req/res optimization for statement distribution #2803

Merged
63 commits merged into from
Apr 9, 2021
Merged

Conversation

eskimor
Copy link
Member

@eskimor eskimor commented Apr 1, 2021

Status: Done. I am still working on some improvements, but those should not be blocking and can go in as a separate PR.

eskimor and others added 9 commits March 29, 2021 21:35
…des. (#2778)

* Better timeout values.

* Fix typo.

* Fix validator bandwidth.

* Fix compilation.
Most importantly code size is now 5 Meg, which is the limit we currently
want to support in statement distribution.
@rphmeier
Copy link
Contributor

rphmeier commented Apr 6, 2021

@eskimor triage?

@eskimor
Copy link
Member Author

eskimor commented Apr 8, 2021

I want to ensure that candidate backing only receives Valid statements about a candidate after receiving at least one Seconded statement about that candidate. Is that still the case?

I am afraid I hurried that part a bit too much - likely not. Will check & fix.

/// nodes within two hops, we will take about 2 seconds for transferring statements (data transfer
/// only). If necessary, we could be able to reduce this to 3 seconds. To consider: The lower the
/// riskier that we will not be able to include a candidate.
const PROPOSE_TIMEOUT: core::time::Duration = core::time::Duration::from_millis(4000);
Copy link
Member Author

Choose a reason for hiding this comment

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

What needs to happen after that 4 seconds and can it fit within the remaining two seconds?

Copy link
Contributor

Choose a reason for hiding this comment

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

Invoking the on_initialize, adding transactions, and then on_finalize of the block. 2 seconds is a bit of a squeeze, honestly.

When this timeout is hit, it means that no parachain stuff will be included which probably makes the 2 seconds more acceptable.

Comment on lines +178 to +182
/// Maximum compressed code size we support right now.
/// At the moment we have runtime upgrade on chain, which restricts scalability severly. If we want
/// to have bigger values, we should fix that first.
pub const MAX_CODE_SIZE: u32 = 3 * 1024 * 1024;

Copy link
Member

Choose a reason for hiding this comment

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

@rphmeier what do you think about this? I guess it'd have to be the upper limit for every chain that will have parachains?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest making it equal to whatever we have in #2854

Copy link
Member Author

Choose a reason for hiding this comment

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

max_code_size: MAX_CODE_SIZE,

@eskimor
Copy link
Member Author

eskimor commented Apr 9, 2021

bot merge

@ghost
Copy link

ghost commented Apr 9, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Apr 9, 2021

Merge aborted: Checks failed for e506d64

@ordian
Copy link
Member

ordian commented Apr 9, 2021

bot merge

@ghost
Copy link

ghost commented Apr 9, 2021

Waiting for commit status.

@ghost ghost merged commit 48d5b14 into master Apr 9, 2021
@ghost ghost deleted the rk-req-res-runtime branch April 9, 2021 21:30
@mmostafas mmostafas added D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Apr 30, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants