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

Fix bug in fraud proof transaction fee value, and combine storage #3320

Merged
merged 2 commits into from
Dec 21, 2024

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Dec 18, 2024

The fraud proof was calculating the consensus transaction byte fee as 1 when dynamic cost of storage was disabled. But the consensus_chain_byte_fee() runtime API returned DOMAIN_STORAGE_FEE_MULTIPLIER in that case. This PR makes the values consistent by fixing the fraud proof calculation.

It also moves the fee into the combined fraud proof storage.

Part of ticket #3281.

Code contributor checklist:

@teor2345 teor2345 added bug Something isn't working consensus Something that impacts consensus execution Subspace execution labels Dec 18, 2024
@teor2345 teor2345 self-assigned this Dec 18, 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.

Good catch!

To further avoid such inconsistency I think we can use InvalidInherentExtrinsicData (perhaps need rename to InherentExtrinsicData) to construct the inherent extrinsic in domain CreateInherentDataProvider instead of querying these data through consensus runtime API one by one.

crates/pallet-domains/src/lib.rs Show resolved Hide resolved
Copy link
Member

@vedhavyas vedhavyas 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!

Refactor seems to be worth it!
Good to go once Ning's suggestion is included

@teor2345
Copy link
Contributor Author

To further avoid such inconsistency I think we can use InvalidInherentExtrinsicData (perhaps need rename to InherentExtrinsicData) to construct the inherent extrinsic in domain CreateInherentDataProvider instead of querying these data through consensus runtime API one by one.

I’d prefer to make this change after all these PRs merge, because it would make each of my 3 open PRs more complicated. It seems like a cleanup we could do once all the fraud proofs are combined.

NingLin-P
NingLin-P previously approved these changes Dec 19, 2024
vedhavyas
vedhavyas previously approved these changes Dec 19, 2024
@teor2345 teor2345 force-pushed the fraud-proof-data-timestamp branch from b3af00c to 797ab57 Compare December 19, 2024 21:15
@teor2345 teor2345 added the breaking-runtime This PR introduces breaking changes to the runtime label Dec 20, 2024
Base automatically changed from fraud-proof-data-timestamp to main December 20, 2024 21:07
@NingLin-P NingLin-P dismissed stale reviews from vedhavyas and themself December 20, 2024 21:07

The base branch was changed.

@NingLin-P NingLin-P added this pull request to the merge queue Dec 21, 2024
Merged via the queue into main with commit cdc769b Dec 21, 2024
8 checks passed
@NingLin-P NingLin-P deleted the fraud-proof-data-fee branch December 21, 2024 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-runtime This PR introduces breaking changes to the runtime bug Something isn't working consensus Something that impacts consensus execution Subspace execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants