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

Make Coordinator aware of transcript size #334

Merged
merged 5 commits into from
Sep 18, 2024
Merged

Conversation

cygnusv
Copy link
Member

@cygnusv cygnusv commented Sep 16, 2024

Type of PR:

  • Bugfix
  • Feature
  • Documentation
  • Other

Required reviews:

  • 1
  • 2
  • 3

What this does:

  • Transcript size can now be deterministically computed from DKG size and threshold. Coordinator validates this accordingly. This is a tiny step in the direction of on-chain validation.
  • Add calldata dynamic costs as part of the gas reimbursement calculation. Note that since we validate transcript sizes, this mechanism can't be abused by posting huge transcripts.

Issues fixed/closed:

Why it's needed:

Notes for reviewers:

  • These changes have been tested on Lynx successfully.
  • Some napkin math for postTranscript TXs in testnet DKG rituals (2-of-3):
    • Before: reimbursements were ~13500 gas short
    • After: reimbursements are ~5000 gas short
    • The difference is ~8500 gas, which is according to the expected, since the max gas for the calldata cost predicted by the formula in this PR is 8768 gas ( 8768 = (644-128)*16 + 128*4, where 644 is the calldata size for a postTranscript TX in a 2-of-3 ritual)
    • The remaining 5000 gas that are still missing is something that can be investigated further, especially using mainnet data

@cygnusv cygnusv marked this pull request as ready for review September 16, 2024 21:30
uint16 dkgSize,
uint16 threshold
) public pure returns (uint256) {
return 40 + (dkgSize + 1) * BLS12381.G2_POINT_SIZE + threshold * BLS12381.G1_POINT_SIZE;
Copy link
Member

Choose a reason for hiding this comment

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

Just a nitpick but 40 is the only unnamed component of this calculation. Perhaps it can be assigned to a constant similar to the BLS point sizes?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, but the thing is that I don't understand well enough where that 40 comes from. I mean, it's a fixed overhead that comes from rust serialization but I don't have a full explanation for it. I'll update this when I have more info.

Copy link
Member

@derekpierre derekpierre left a comment

Choose a reason for hiding this comment

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

🎸 .

Did you also want to verify your updated Coordinator contract on lynx, with our updated verify script? 😉

TIMEOUT = 1000
MAX_DKG_SIZE = 31
FEE_RATE = 42
ERC20_SUPPLY = 10**24
DURATION = 48 * 60 * 60
ONE_DAY = 24 * 60 * 60
Copy link
Member

@derekpierre derekpierre Sep 17, 2024

Choose a reason for hiding this comment

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

Thanks for this cleanup. We were repeating the RitualState enum and fixtures in a number of places.

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.

Static gas cost for ReimbursementPool doesn't include calldata cost
4 participants