Set the block gas limit to the value returned by a contract call by vkomenda · Pull Request #10928 · openethereum/parity-ethereum

vkomenda · 2019-07-30T15:33:44Z

This PR allows to have programmable block gas limit.

See also:

original PR: Set gas limit according to TxPermission.blockGasLimit. poanetwork/parity-ethereum#140
original issue Reduced block gas limit when TxPermission.limitBlockGas() returns true poanetwork/parity-ethereum#119

parity-cla-bot · 2019-07-30T15:33:48Z

It looks like @vkomenda signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

vkomenda · 2019-08-27T12:43:28Z

I've rebased the PR. Please review :)

dvdplm · 2019-08-28T09:46:54Z

@vkomenda sorry about the churn on master recently. The test failure seem to be a simple import fix (Header from common-types).

dvdplm

My main concern here regards the changes to the Engine trait. I'm reluctant to add more code to it than we absolutely need. It seems like it's needed only in verification? Is it possible to coalesce the gas limit logic into a single fn gas_limits() -> (u64, u64) and have the default impl return (max, min) and Aura do the custom logic?

dvdplm · 2019-08-28T10:01:25Z

 	/// Like `call`, but with various defaults. Designed to be used for calling contracts.
 	fn call_contract(&self, id: BlockId, address: Address, data: Bytes) -> Result<Bytes, String>;
+
+	/// Makes a constant call to a contract, at the beginning of the block corresponding to the given header, i.e. with


Maybe dumb q: what is "constant" about the call? Isn't it called for every block of the override is in effect?

It's "constant" in the sense that it doesn't create a transaction and modify the blockchain's actual state. Not sure what the right word for this is.

Maybe "stateless" or "immutable"? But technically the contract ABI can allow a contract call that modifies the state. I think assuming that the call modifies the state would be more complete.

Not sure what the best wording is. In this blog post it's called "local" and "read-only call", as opposed to "verified" and "potentially state-changing transaction".

(To clarify: This can be used with calls that would modify the state. It's just that it throws away the modified state instead of creating a transaction which, when mined, would actually change the blockchain's EVM state.)

dvdplm · 2019-08-28T10:28:02Z

+			.map_err(|_| CallError::StatePruned.to_string())?;
+
+		let from = Address::default();
+		let transaction = transaction::Transaction {


I think this merits a comment: looks like it's a "fake" tx used to satisfy the type signature of do_virtual_call? Maybe it can be extracted to its own method to make the intent clear?

dvdplm · 2019-08-28T10:30:20Z

+			)));
+		}
+	} else {
+		let min_gas_limit = engine.params().min_gas_limit;


Seems like there should be an engine.min_gas_limit() method (or perhaps engine.gas_limits() -> (u64, u64)?).

I'll add a method although it's redundant because the field min_gas_limit is public.

min_gas_limit being public is… not that great! There's probably some historical reason for that.

The point I was trying to make was this: I'm a bit hesitant about this PR because it adds code that is useful only to Aura. Ideally we should architect things such that engine-specific behaviour stays in that engine's code. One idea I had was to have a single gas_limits() method on Engine which would let the Aura implementation to run the logic it needs without "polluting" other engines.

This can be done, I think.
@varasev, is it OK to move blockGasLimitContract from the general engine parameter set to the AuRa parameter set? I would also recommend renaming it as blockGasLimitContracts since we are changing it to support only a map.

Yes, it's OK to move, but don't rename, please, because the map still assumes that there is only one contract at a time.

If you move this parameter, please also move it in our https://github.com/poanetwork/parity-ethereum/tree/aura-pos branch.

@varasev OK, I'll copy that in aura-pos. Are you sure about the name? Wouldn't plural look better alongside blockRewardContractTransitions?

Then that should be named blockGasLimitContractTransitions

Makes sense.

Done in poanetwork#189.

tomusdrw · 2019-08-28T12:51:44Z

 			.map(|executed| executed.output)
 	}
+
+	fn call_contract_before(&self, header: &Header, address: Address, data: Bytes) -> Result<Bytes, String> {


Why is that even needed? Why can't use use call_contract and pass BlockId with parent_hash?

We can definitely use call later on in this function. I'm not sure about replacing call_contract_before with call_contract because state is defined differently in those two functions.

It looks like the use of mut state in call_contract_before is incorrect because it is then dropped. So whatever changes call makes to it are forgotten.

@afck, what are your thoughts on using call_contract instead of call_contract_before (which should fix the use of mutable state)?

You're probably right: We should just pass in the block number into block_gas_limit as a parameter.

Originally this was motivated by a smart contract (written before this Rust code) that had a limitBlockGas getter that returned whether the gas limit should be reduced for the current block. So we needed a ("not exactly sane") way to call this with a state that didn't exist yet: where the block number is the number of the block that we're currently creating. The new blockGasLimit implementation has a similar problem: It calls a few methods that use the current block number.
@varasev: Would it be feasible to just pass the block number into blockGasLimit as an argument instead?

It looks like the use of mut state in call_contract_before is incorrect because it is then dropped.

Client::call_contract drops the state as well: It ends up via call in do_virtual_call, where it is used to create an Executive instance and a diff. It's not persisted, however, and that's intended: These methods are meant to run calls locally and return the call's output. They don't create transactions and thus can't modify the actual EVM state.

Would it be feasible to just pass the block number into blockGasLimit as an argument instead?

No, the blockGasLimit calls other getters of other contracts to determine the gas limit for the current block. The returned values of those getters depend on the current state of the current block.

tomusdrw · 2019-08-28T12:52:56Z

-	if let Some(limit) = engine.maximum_gas_limit() {
-		if header.gas_limit() > &limit {
-			return Err(From::from(BlockError::InvalidGasLimit(OutOfBounds { min: None, max: Some(limit), found: *header.gas_limit() })));
+	if let Some(gas_limit) = engine.gas_limit_override(header) {


I'm pretty sure it won't really work correctly. What guarantees do we have that parent_hash is already imported? State-touching checks are only allowed further down in verification afair.

tomusdrw · 2019-08-28T12:55:22Z

-	let max_gas = parent_gas_limit + parent_gas_limit / gas_limit_divisor;
-	if header.gas_limit() <= &min_gas || header.gas_limit() >= &max_gas {
-		return Err(From::from(BlockError::InvalidGasLimit(OutOfBounds { min: Some(min_gas), max: Some(max_gas), found: *header.gas_limit() })));
+	if engine.gas_limit_override(header).is_none() {


That's pretty wasteful to call the contract twice during verification (one for verify_header and second time here), why not move the other gas_limit verification in here?

vkomenda · 2019-08-28T17:21:40Z

@vkomenda sorry about the churn on master recently. The test failure seem to be a simple import fix (Header from common-types).

@dvdplm, which module has the error? I couldn't find it with cargo test.

dvdplm · 2019-08-28T17:42:57Z

which module has the error? I couldn't find it with cargo test.

oh I bet you don't see the CI output right? It's here:

error[E0412]: cannot find type `Header` in this scope
   --> ethcore/private-tx/src/key_server_keys.rs:164:44
    |
164 |         fn call_contract_before(&self, _header: &Header, _address: Address, _data: Bytes) -> Result<Bytes, String> {
    |                                                  ^^^^^^ not found in this scope
help: possible candidates are found in other modules, you can import them into scope
    |
140 |     use ethjson::blockchain::Header;
    |
140 |     use ethjson::blockchain::header::Header;
    |
140 |     use types::encoded::Header;
    |
140 |     use types::header::Header;
    |

error: aborting due to previous error

For more information about this error, try `rustc --explain E0412`.
error: Could not compile `ethcore-private-tx`.

FYI you might want to run cargo test --all --release in the repo root to make sure you run all tests. :)

afck · 2019-08-30T09:22:40Z

The test was failing because it expected verify_block_basic to check the gas limit, which we moved to verify_block_family now. (And because of a missing header = good.clone();.)
Is it okay for verify_block_basic to not check the gas limit anymore?

Otherwise we'd have to add something like fn use_gas_limit_contract(&self, block: &BlockNumber) -> bool to Engine that doesn't call the contract and only looks into the configuration to find out whether it would use the contract at that block number, and check the gas limit in verify_header_params (used in verify_block_basic) only if use_gas_limit_contract returns false.

@varasev: The meaning of blockGasLimit has changed now. It is supposed to return the next block's gas limit. Is that practical in posdao-contracts?

varasev · 2019-08-30T10:16:40Z

The meaning of blockGasLimit has changed now. It is supposed to return the next block's gas limit. Is that practical in posdao-contracts?

Ok

varasev · 2019-08-30T11:12:25Z

The meaning of blockGasLimit has changed now. It is supposed to return the next block's gas limit. Is that practical in posdao-contracts?

What about block #1? As far as I understand, the engine will call blockGasLimit to get the gas limit for the next block. It means that when it is called on the block #1, the blockGasLimit will return a limit for block #2. What limit will be for block #1 then?

afck · 2019-08-30T11:35:38Z

It will actually be called "after the genesis block", I think. So it will be called at the point where the block.number is still 0 and the EVM state is the state between blocks 0 and 1.

VladLupashevskyi · 2019-09-16T13:30:26Z

 	}

+	/// Get a reference to the transaction filter, if present.
+	pub fn tx_filter(&self) -> Option<&Arc<TransactionFilter>> {


Why do we need to have it here, since it's not used anywhere, or have I missed smth?

Sorry, this is perhaps an unwanted effect of a rebase.

VladLupashevskyi · 2019-09-16T13:30:42Z

 	}

+	/// Get a reference to the contract address.
+	pub fn contract_address(&self) -> &Address {


The same here

Agreed. This method can be removed as well as Machine::tx_filter.

VladLupashevskyi

Don't we need to let the miner know about gas limit transitions? It seems to me that following the logic of having contract for gas limit configurations should mean that the miner would have to change the limit on the fly, otherwise you cannot be sure that transactions will get mined after such a transition until the validator node configuration is updated.

vkomenda · 2019-09-16T14:30:16Z

Don't we need to let the miner know about gas limit transitions? It seems to me that following the logic of having contract for gas limit configurations should mean that the miner would have to change the limit on the fly, otherwise you cannot be sure that transactions will get mined after such a transition until the validator node configuration is updated.

We set the gas limit once for every block at the time of header construction in Engine::populate_from_parent. Do you think the miner can mine transactions in a block before its header has been constructed?

VladLupashevskyi · 2019-09-16T15:22:07Z

@vkomenda I just have noticed that gas_range_target is not updated with the value from contract and it's used in block opening here:

https://github.com/paritytech/parity-ethereum/blob/bceb1d569147cf113520b904bf84255d100b4d1d/ethcore/src/client/client.rs#L2332

Which would lead to issues that miner wouldn't allow tx to be included to the block if its gas cost is more then it's set for OpenBlock , however I have checked and tx is included without any issues.

afck

I removed some unused imports, and the unused functions as discussed, rebased on master and squashed the commits (because a lot of the changes in them have been reverted now).

I just have noticed that gas_range_target is not updated

I'm not sure whether it should be: To my mind, the periods where the contract returns Some(value) should be considered exceptions where we suspend the normal gas limit policy and use value instead. After that, maybe it would be best to return to the previous behavior, with the previous target range?

afck · 2019-09-26T10:31:08Z

-		if header.gas_limit() > &limit {
-			return Err(From::from(BlockError::InvalidGasLimit(OutOfBounds { min: None, max: Some(limit), found: *header.gas_limit() })));
-		}
-	}


Would it be better to keep these checks, whenever the gas limit contract returns None?

They've moved to line 331 below.

afck · 2019-09-27T06:59:07Z

~~I can't reproduce that test failure. For me, cargo test --release --all works.~~
~~(Rebasing fixed it.)~~
~~(Rebasing again broke it again.)~~

afck · 2019-10-15T12:04:02Z

I rebased again, due to merge conflicts. Please let me know if there are any further changes you would like me to make to the PR.

dvdplm

lgtm (but do consider my comment about adding fn gas_limits().

dvdplm · 2019-10-15T13:53:41Z


+	/// Overrides the block gas limit. Whenever this returns `Some` for a header, the next block's gas limit must be
+	/// exactly that value.
+	fn gas_limit_override(&self, _header: &Header) -> Option<U256> {


So I think I made this suggestion before but perhaps it got lost.

In order to keep the Engine trait from growing ever fatter, how about we have a fn gas_limits(&self) -> Option<(U256, U256)> that return the min/max limit. The default impl could return (self.params().min_gas_limit, self.params().max_gas_limit) and then Aura can provide its own impl with the limit override logic tucked away nicely. This would require refactoring other code around the project but lets us remove maximum_gas_limit(&self) and saves us from adding two new methods.

Not sure if you tried this and it doesn't work or if you feel it's a bad idea?

Sorry, I missed that!

I'm not sure about this idea: When the engine changes the gas limit, we don't want to do these checks:

At the moment, it seems to be impossible to produce a valid block if the interval based on the gas limit divisor doesn't overlap with the interval based on the engine's min and max gas limits.
Maybe engines that use a gas limit contract should just use a gas limit divisor of 1, but that would still prevent them from more than doubling the gas limit from one block to the next.

But we could make gas_limits return a bool as well, that tells us whether it's an override. Or it could return an enum?

enum GasLimits { MinMax(U256, U256), Override(U256), }

Hmm, so why do you need to know that there was an override? Why isn't it enough to know what the bounds must be at that block? In this PR there are 3 error cases: gas too low, too high, overridden but mismatching – why is it important to know that the error came from an override?

There seem to be two different intervals for the block gas limit:

the engine's min/max and

the range calculated using the gas limit divisor, which allows us to change the limit only by a certain percentage compared to the previous block.

The second limitation can conflict with the first, if the engine makes sudden changes to the gas limit. So I think it should be disabled if there is an override.

I'm not really sure at all what's the best way to approach this. To give you some context, the gist is:
In POSDAO, there will be rather expensive implicit calls to the governance contracts once per week, to distribute rewards and select new validators. While these are ongoing, we want to limit the gas available for transactions, so that the total CPU usage per block remains roughly constant. (For details see: poanetwork#119 (comment))

debris

I reviewed the code carefully. It look good, just a few minor grumbles

debris · 2019-10-18T10:20:59Z

+					OutOfBounds { min: None, max: Some(limit), found: *header.gas_limit() }
+				).into());
+			}
+		}


this function is called verify_parent, but this logic has nothing to do with the parent header

what if you move this verification to engine's fn verify_block_basic? If you do that you will not need to add additional methods to the Engine trait

Done.
I also moved the checks back to verify_header_params that were originally there.
Since I'm now calling gas_limit_override three times instead of once, I added a cache to memoize the results, to avoid executing the same contract call multiple times.

Thanks for changes, but that's not exactly what I wanted 😅

I believe that files:

ethcore/verification/src/verification.rs

ethcore/engine/src/engine.rs

can and should not be modified by this pr.

All the changes can be encapsulated in ethcore/engines/authority-round/src/lib.rs file

How could this be achieved without modifying ethcore/verification/src/verification.rs? After all, it's an exception to block verification to allow for gas limits set by the engine.

What we could do is move all the gas limit related verification code from verification.rs to the engines, and make the current behavior the Engine trait's default implementation. Do you think that would be better?

dvdplm

A few questions and comments. I agree with @debris though: anything that is engine specific (and this is) and can be done in the engine, definitely should. I know this can be tricky at times.

varasev · 2019-10-28T14:12:49Z

Hi guys, please consider @afck's last answers. Are there still some things we need to improve in this PR?

afck · 2019-10-29T10:56:51Z

Another test failure that I can't reproduce locally.

ordian · 2020-01-10T15:24:47Z

It seems that the code is reviewed well. Please resolve the merge conflicts, so that we can merge the PR :)

Lower gas limit if TxPermission.limitBlockGas. Call blockGasLimit before every block. Make the block gas limit contract a separate config option. Add `info` level logging of block gas limit switching block-gas-limit subcrate and responses to review comments simplified call_contract_before moved block_gas_limit_contract_transitions to AuRa params removed call_contract_before Update and fix test_verify_block. Remove some unused imports and functions.

afck · 2020-01-11T11:08:34Z

Thanks! I rebased it.

jam10o-new added A0-pleasereview 🤓 Pull request needs code review. M2-config 📂 Chain specifications and node configurations. M4-core ⛓ Core client code / Rust. labels Jul 30, 2019

jam10o-new added this to the 2.7 milestone Jul 30, 2019

vkomenda force-pushed the poanetwork-block-gas-limit branch 2 times, most recently from 4c2477a to 17eb64d Compare August 27, 2019 12:14

dvdplm reviewed Aug 28, 2019

View reviewed changes

tomusdrw reviewed Aug 28, 2019

View reviewed changes

vkomenda force-pushed the poanetwork-block-gas-limit branch from 17eb64d to e5d88fd Compare August 28, 2019 17:22

vkomenda force-pushed the poanetwork-block-gas-limit branch from 443ffb9 to c03c4ba Compare August 29, 2019 11:13

vkomenda mentioned this pull request Aug 29, 2019

Move blockGasLimitContract to AuRa params and rename it poanetwork/parity-ethereum#189

Merged

VladLupashevskyi reviewed Sep 16, 2019

View reviewed changes

afck force-pushed the poanetwork-block-gas-limit branch from e8d282c to 1f96ce0 Compare September 26, 2019 10:24

afck reviewed Sep 26, 2019

View reviewed changes

afck force-pushed the poanetwork-block-gas-limit branch from 1f96ce0 to ea63954 Compare September 30, 2019 08:05

afck force-pushed the poanetwork-block-gas-limit branch 2 times, most recently from ea9f4b6 to 6644b56 Compare October 8, 2019 14:03

afck force-pushed the poanetwork-block-gas-limit branch from 6644b56 to 52c53c7 Compare October 15, 2019 12:03

dvdplm approved these changes Oct 15, 2019

View reviewed changes

debris reviewed Oct 18, 2019

View reviewed changes

dvdplm reviewed Oct 22, 2019

View reviewed changes

Comment thread ethcore/engines/authority-round/src/lib.rs

Comment thread ethcore/engines/authority-round/src/lib.rs

Comment thread ethcore/engines/authority-round/src/lib.rs

afck force-pushed the poanetwork-block-gas-limit branch from 77bc863 to ab9e912 Compare October 29, 2019 10:00

afck mentioned this pull request Dec 10, 2019

Allow Engine to count expensive system calls towards block gas limit. #11320

Closed

varasev mentioned this pull request Jan 6, 2020

Error(Engine(NotProposer(Mismatch and Block(InvalidGasLimit(OutOfBounds poanetwork/parity-ethereum#203

Open

afck added 2 commits January 10, 2020 20:52

Move gas limit override check to verify_block_basic.

4d52c90

afck force-pushed the poanetwork-block-gas-limit branch from ab9e912 to 4d52c90 Compare January 11, 2020 11:07

ordian approved these changes Jan 13, 2020

View reviewed changes

ordian merged commit 73354d8 into openethereum:master Jan 13, 2020

ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 13, 2020

afck deleted the poanetwork-block-gas-limit branch January 13, 2020 10:32

LukaszRozmej mentioned this pull request Feb 7, 2020

Block GasLimit from contract NethermindEth/nethermind#1416

Closed

rakita mentioned this pull request Dec 7, 2020

Backport xDAI support openethereum/openethereum#118

Closed

varasev mentioned this pull request May 25, 2021

Sunce86/eip 1559 openethereum/openethereum#393

Merged

5 tasks

Conversation

vkomenda commented Jul 30, 2019

Uh oh!

parity-cla-bot commented Jul 30, 2019

Uh oh!

vkomenda commented Aug 27, 2019

Uh oh!

dvdplm commented Aug 28, 2019

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkomenda commented Aug 28, 2019

Uh oh!

dvdplm commented Aug 28, 2019

Uh oh!

afck commented Aug 30, 2019

Uh oh!

varasev commented Aug 30, 2019

Uh oh!

varasev commented Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

afck commented Aug 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

varasev commented Aug 30, 2019 •

edited

Loading

afck commented Sep 27, 2019 •

edited

Loading