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

Domains: Limit domain's max extrinsic weight to max domain bundle weight #2801

Merged
merged 2 commits into from
May 30, 2024

Conversation

vedhavyas
Copy link
Member

@vedhavyas vedhavyas commented May 29, 2024

We introduced a regression while setting the maximum domain block limit where in individual domain extrinsic was equal to max domain block limit. This would lead to excessive extrinsic weight allowance on domain leading to transaction being in txpool forever since this value exceeds the bundle weight limit defined on the consensus chain.

This PR ensures to use max bundle limit as the max domain extrinsic weight with bundle slot probability to always set 1. The current domain weights post this change

{
  baseBlock: {
    refTime: 390,584,000
    proofSize: 0
  }
  maxBlock: {
    refTime: 18,446,744,073,709,551,615
    proofSize: 18,446,744,073,709,551,615
  }
  perClass: {
    normal: {
      baseExtrinsic: {
        refTime: 124,414,000
        proofSize: 0
      }
      maxExtrinsic: {
        refTime: 216,666,666,666
        proofSize: 3,407,872
      }
      maxTotal: {
        refTime: 18,446,744,073,709,551,615
        proofSize: 18,446,744,073,709,551,615
      }
      reserved: {
        refTime: 0
        proofSize: 0
      }
    }
    operational: {
      baseExtrinsic: {
        refTime: 124,414,000
        proofSize: 0
      }
      maxExtrinsic: {
        refTime: 216,666,666,666
        proofSize: 3,407,872
      }
      maxTotal: {
        refTime: 18,446,744,073,709,551,615
        proofSize: 18,446,744,073,709,551,615
      }
      reserved: {
        refTime: 0
        proofSize: 0
      }
    }
    mandatory: {
      baseExtrinsic: {
        refTime: 124,414,000
        proofSize: 0
      }
      maxExtrinsic: {
        refTime: 216,666,666,666
        proofSize: 3,407,872
      }
      maxTotal: {
        refTime: 18,446,744,073,709,551,615
        proofSize: 18,446,744,073,709,551,615
      }
      reserved: null
    }
  }
}

If the domain bundle slot probability is lower than 1, the value of the maximum domain block increases but each extrinsic max weight will be limited to 250,000,000,000 which is 250 ms execution time.

TLDR:

On Consensus Chain:
Max allowed proof size for Normal extrinsics: 3.75 MiB
Max bundle weight:
- Execution: 250 ms
- Proof size: 3.25 MiB to have room for additional storage coming as part of Fraud proof
This is the bundle weight that operator uses while proposing the bundles. So each bundle should include one or more transactions that meet this bundle weight.

On Domains:
Max Domain block weight:
- Execution: u64::MAX, this is set to ensure all the extrinsics from bundles are executed irrespective of execution time
- Proof Size: u64::MAX, total proof size for entire domain block will not be used for FP but rather each extrinsic within the domain block

Max Extrinsic block weight:
This is same as max bundle weight on consensus chain.
- Execution: 250 ms
- Proof Size: 3.25 MiB, Single extrinsic can use this size with consideration for Fraud proof size

Closes: #2387

cc: @dariolina

Code contributor checklist:

@dariolina
Copy link
Member

I agree, it's correct that extrinsic limit should be <= bundle limit. I still vouch for the bundle limit be in domain config (#2365) and deprecating calculate_max_bundle_weight. What is the hard limit then on bundle and extrinsic weight, if there was 1 bundle with 1 extrinsic in the block and we need to be able to fit a fraud proof of its invalid state transition?

NingLin-P
NingLin-P previously approved these changes May 30, 2024
Copy link
Member

@NingLin-P NingLin-P left a comment

Choose a reason for hiding this comment

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

Make sense

@vedhavyas
Copy link
Member Author

What is the hard limit then on bundle and extrinsic weight, if there was 1 bundle with 1 extrinsic in the block and we need to be able to fit a fraud proof of its invalid state transition?

The hard limit for 1 bundle and 1 big extrinsic would be 250 ms execution time. As per the proof size, this should be more than sufficient to fit that bit extrinsic in the fraud proof. Right now I'm checking with other exisiting para chains for their max POV size to be sure and then will merge once I'm even more confident

@NingLin-P
Copy link
Member

Plz resolve the conflict

@vedhavyas
Copy link
Member Author

vedhavyas commented May 30, 2024

@dariolina the proof size for each extrinsic is also updated to ensure it fits the consensus block during FP.
Do take a look again PR description for definitive numbers

@vedhavyas vedhavyas added this pull request to the merge queue May 30, 2024
Merged via the queue into main with commit 447299b May 30, 2024
11 checks passed
@vedhavyas vedhavyas deleted the extrinsic_weight branch May 30, 2024 19:02
@NingLin-P NingLin-P added the audit-P3 Low audit priority label Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-P3 Low audit priority need to audit This change needs to be audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The max_extrinsic weight limit for domain extrinsic seems to be too large
3 participants