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

pallet_ethereum weights #531

Merged
merged 3 commits into from
Dec 2, 2021

Conversation

tgmichel
Copy link
Contributor

@tgmichel tgmichel commented Dec 2, 2021

No description provided.

@tgmichel tgmichel requested a review from sorpaas as a code owner December 2, 2021 15:32
@@ -223,11 +224,16 @@ pub mod pallet {
Self::validate_transaction_in_block(source, &transaction).expect(
"pre-block transaction verification failed; the block cannot be built",
);
Self::apply_validated_transaction(source, transaction);
let r = Self::apply_validated_transaction(source, transaction);
weight = weight.saturating_add(r.actual_weight.unwrap_or(0 as Weight));
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this would cause us to double-account for weight and have the effect of lowering throughput. The base Ethereum txn fee is a sort of fudge for handling transaction inclusion, which could be considered to cover this.

What is complex about this is that it happens in different blocks... it makes sense to account for the weight of work done in the block it occurs in. But again, I think this might have the net effect of us lowering txn throughput.

One idea we could explore to deal with this is reducing the weight (but not the gas) charged for txns in a block with the expectation that that weight is charged in on_initialize() in the following block.

Copy link
Member

Choose a reason for hiding this comment

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

This is for wrapped blocks and you are not using this code path at all on a Substrate chain (Moonbeam, Moonriver)!

Regarding tx throughput in general -- adding weight will not reduce it. It just provides information and avoids spams. You can arbitrarily increase overall block weight to a really large value if the chain can handle it.

Copy link
Contributor

@notlesh notlesh Dec 2, 2021

Choose a reason for hiding this comment

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

Thanks for clarifying. wrapped block is new to me, are you referring to this or something Frontier-specific?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah! This part of the code is to allow the runtime eventually handle processing an existing Ethereum-like blockchain (so that we can support migrating an Ethereum-like network over to Substrate, and add Substrate specific functionalities).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, @tgmichel brought this to my attention (re: "wrapped block"): #257

@sorpaas sorpaas merged commit 1ae0859 into polkadot-evm:master Dec 2, 2021
@tgmichel tgmichel deleted the tgm-weights branch April 1, 2022 09:55
abhijeetbhagat pushed a commit to web3labs/frontier that referenced this pull request Jan 11, 2023
* Account for `pallet_ethereum::on_initialize` weight

* Account for `pallet_ethereum::on_finalize` weight

* Account for `pallet_ethereum::on_runtime_upgrade` weight
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