Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Reduce gas limit in estimateGas from 1 Trillion to 10 * block.gas_limit #6840

Closed
jbentke opened this issue Oct 20, 2017 · 14 comments
Closed
Labels
F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M4-core ⛓ Core client code / Rust. P7-nicetohave 🐕 Issue is worth doing eventually. Q2-easy 💃 Can be fixed by copy and pasting from StackOverflow. Q3-medium A fair chunk of work, not necessarily very hard but not trivial either
Milestone

Comments

@jbentke
Copy link

jbentke commented Oct 20, 2017

I'm running:

  • Parity version: 1.7.4
  • Operating system: MacOS
  • And installed: homebrew

Following two contracts are being deployed:

pragma solidity ^0.4.13;

contract Assigner {

    address[] listA;
    address[] listB;

    function finalizeChange() {
        listA = listB;
    }
}
pragma solidity ^0.4.13;

contract Delegate {

    address[] listA;

    address assignerContract = *address of Assigner contract*;
    
    function finalizeChange() {
        require(assignerContract.delegatecall(msg.data));
    }
}

(I stripped the example contracts down to a bare minimum.)

steps to reproduce:

  1. deploy the Assigner contract
  2. deploy the Delegate contract (adjusting the assignerContract address)
  3. run Delegate.finalizeChange().

expected-behavior:
Because listB is not mapped in the Delegate contract, parity should throw an error.

actual-behavior:
Parity will run in some sort of endless loop while consuming 100% CPU in the activity monitor and can not be stopped via ctrl+c.

@rphmeier rphmeier self-assigned this Oct 21, 2017
@rphmeier
Copy link
Contributor

rphmeier commented Oct 21, 2017

I notice the similarity to the ValidatorSet interface. Is finalizeChange being called in a regular transaction or is this a validator contract which is automatically called by the system?

In the latter case, please note that there is no limit on the amount of computation finalizeChange can do, as the code is essentially included into the consensus process. An infinite loop or similar is generally a bad idea because it will spin forever.

The fact that CTRL-C doesn't kill is definitely a problem, but can probably be solved pretty easily. Right now the IO Workers are waited for on shutdown, where really they could be detached after a certain interval.

@rphmeier rphmeier removed their assignment Oct 21, 2017
@5chdn 5chdn added F2-bug 🐞 The client fails to follow expected behavior. M4-core ⛓ Core client code / Rust. P5-sometimesoon 🌲 Issue is worth doing soon. labels Oct 21, 2017
@5chdn 5chdn added this to the 1.9 milestone Oct 21, 2017
@jbentke
Copy link
Author

jbentke commented Oct 23, 2017

Yes, I came across that error while doing a validatorSet contract, but I also tried it as a normal transaction with the contracts above. Parity "crashed" in both cases.

@jbentke
Copy link
Author

jbentke commented Oct 23, 2017

I just did some more testing running a ethash network with geth and parity. Sending the transaction through geth does not influence parity. When trying to send the transaction through parity, the cpu usage goes up a lot but does not influence the synching of the client. However it prevent the client from sending any further transactions (tested through ui). Hope this helps narrow down the issue 👍

@rphmeier
Copy link
Contributor

Can you specify more details of the transaction? For example, how much gas was provided, what the gas limit of the chain was at that time, and what the nonce of the transaction and account were?

@rphmeier rphmeier added Z0-unconfirmed 🤔 Issue might be valid, but it’s not yet known. and removed F2-bug 🐞 The client fails to follow expected behavior. P5-sometimesoon 🌲 Issue is worth doing soon. labels Oct 24, 2017
@jbentke
Copy link
Author

jbentke commented Oct 24, 2017

Random. I tried it a bunch of times. DId it with dev chain, with ethash and validatorSet (with validatorSet it crashes on chain creation).

@rphmeier rphmeier added F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. and removed Z0-unconfirmed 🤔 Issue might be valid, but it’s not yet known. labels Oct 24, 2017
@rphmeier
Copy link
Contributor

Can reproduce, but just to make a note first:

with validatorSet it crashes on chain creation

that's intended behavior I would say. introducing an infinite loop or panic into the consensus code, whether at the Rust or EVM level is going to do that. We can fix Rust issues here, but correctness of ValidatorSets are the responsibility of their authors.


Seems that estimateGas stalls when presented with an infinite loop like the one above and a minimal one here:

contract InfiniteLoop {
    function infiniteLoop() public {
        while (true) {}
    }
}

because it first tries to execute the transaction with block.gas_limit gas and if that fails then tries to execute with 1 TRILLION gas. Obviously that will spin for a long time.

We should have a more sensible fallback after block.gas_limit fails. I understand the motivation to produce estimates beyond the block gas limit, but maybe a constant multiple of N * block.gas_limit as a maximum would be safer.

@arkpar
Copy link
Collaborator

arkpar commented Oct 24, 2017

The high boundary used to be 50 million. Maybe just revert to that? N would be about 10

@5chdn 5chdn added P5-sometimesoon 🌲 Issue is worth doing soon. P7-nicetohave 🐕 Issue is worth doing eventually. and removed P5-sometimesoon 🌲 Issue is worth doing soon. labels Nov 13, 2017
@5chdn 5chdn modified the milestones: 1.9, 1.10 Nov 13, 2017
@5chdn 5chdn modified the milestones: 1.10, 1.11 Jan 23, 2018
@5chdn 5chdn modified the milestones: 1.11, 1.12 Mar 1, 2018
@5chdn 5chdn modified the milestones: 1.12, 1.13 Apr 24, 2018
@folsen folsen changed the title Parity crashes on illegal assignment Reduce gas limit in estimateGas from 1 Trillion to 10 * block.gas_limit May 19, 2018
@folsen folsen added Q2-easy 💃 Can be fixed by copy and pasting from StackOverflow. Q3-medium A fair chunk of work, not necessarily very hard but not trivial either labels May 19, 2018
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 0.16 ETH (92.13 USD @ $575.79/ETH) attached to it.

@gitcoinbot
Copy link

gitcoinbot commented May 27, 2018

Issue Status: 1. Open 2. Cancelled


Work has been started.

These users each claimed they can complete the work by 8 months, 2 weeks ago.
Please review their action plans below:

1) omar408 has started work.

O.C

Learn more on the Gitcoin Issue Details page.

@rphmeier
Copy link
Contributor

@vs77bb
Copy link

vs77bb commented May 28, 2018

@omar408 Could you please clarify your approach here?

Also thanks for the thought @rphmeier - if it's already implemented, we can pull the bounty here and re-allocate it elsewhere 🙂

@vs77bb
Copy link

vs77bb commented May 30, 2018

@folsen Would you mind confirming if this is already complete? If not, I will make sure this gets back in front of Gitcoiner's who may want to pick it up. 😄

@rphmeier
Copy link
Contributor

closed in #7075

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Cancelled


The funding of 0.16 ETH (89.62 USD @ $560.13/ETH) attached to this issue has been cancelled by the bounty submitter

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M4-core ⛓ Core client code / Rust. P7-nicetohave 🐕 Issue is worth doing eventually. Q2-easy 💃 Can be fixed by copy and pasting from StackOverflow. Q3-medium A fair chunk of work, not necessarily very hard but not trivial either
Projects
None yet
Development

No branches or pull requests

7 participants