Skip to content

pallet_ethereum weights #531

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.

Sign up for GitHub

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

Merged
sorpaas merged 3 commits into from
Dec 2, 2021
Merged

pallet_ethereum weights #531

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91336ac
Account for `pallet_ethereum::on_initialize` weight
tgmichel Dec 2, 2021
bcd9b8b
Account for `pallet_ethereum::on_finalize` weight
tgmichel Dec 2, 2021
be9bcb2
Account for `pallet_ethereum::on_runtime_upgrade` weight
tgmichel Dec 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions frame/ethereum/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use frame_support::{
traits::{EnsureOrigin, Get},
weights::{Pays, PostDispatchInfo, Weight},
};
use frame_system::pallet_prelude::OriginFor;
use frame_system::{pallet_prelude::OriginFor, WeightInfo};
use pallet_evm::{BlockHashMapping, FeeCalculator, GasWeightMapping, Runner};
use sha3::{Digest, Keccak256};
use sp_runtime::{
Expand Down Expand Up @@ -209,6 +209,7 @@ pub mod pallet {

fn on_initialize(_: T::BlockNumber) -> Weight {
Pending::<T>::kill();
let mut weight = T::SystemWeightInfo::kill_storage(1);

// If the digest contain an existing ethereum block(encoded as PreLog), If contains,
// execute the imported block firstly and disable transact dispatch function.
Expand All @@ -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
Copy Markdown
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.

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
Copy Markdown
Member

@sorpaas sorpaas Dec 2, 2021

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
Copy Markdown
Contributor

@notlesh notlesh Dec 2, 2021 •
edited
Loading

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
Copy Markdown
Member

@sorpaas sorpaas Dec 2, 2021

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
Copy Markdown
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.

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

}
}

0
// Account for `on_finalize` weight:
// - read: frame_system::Pallet::<T>::digest()
// - read: frame_system::Pallet::<T>::block_number()
// - write: <Pallet<T>>::store_block()
// - write: <BlockHash<T>>::remove()
weight.saturating_add(T::DbWeight::get().reads_writes(2, 2))
}

fn on_runtime_upgrade() -> Weight {
Expand All @@ -236,7 +242,7 @@ pub mod pallet {
&EthereumStorageSchema::V2,
);

0
T::DbWeight::get().write
}
}

Expand Down