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

minGasLimit/gasLimitBoundDivisor is not taken into account when creating pending block #3760

Closed
tomusdrw opened this issue Dec 9, 2016 · 8 comments
Assignees
Labels
F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M4-core ⛓ Core client code / Rust.

Comments

@tomusdrw
Copy link
Collaborator

tomusdrw commented Dec 9, 2016

Together with --gas-floor-target below minGasLimit leads the node to mine invalid blocks (that presumably are short-circuited into the chain without proper verification that would reject them)

Reported on gitter:

I was testing a private chain with extremely large (pi*10M) genesis block gas limit and a gas minimum equal to that. I mined blocks on one node and it worked just fine, but another node refused to sync the chain because block 1 gas limit was below minimum. I later checked that the gas limit indeed decreased. My guess is that the default value for --gas-floor-target made the mining node decrease the limit, then the client did not properly validate a block that it has just mined, and broadcast it, while the listening node tried to validate it and failed. Effectively the same thing happened when I next also set an effectively infinite gasLimitBoundDivisor to prevent the gas limit to move altogether as per (YP:45-46). I guess I can work around it all by setting a proper --gas-floor-target, but I think it should not "override" how the gas limit evolution is specified by the chain spec params and definitely Parity should not mine blocks that turn out to be invalid.

@tomusdrw tomusdrw added F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M4-core ⛓ Core client code / Rust. labels Dec 9, 2016
@lgpawel
Copy link

lgpawel commented Dec 9, 2016

It's in the description but not in the title, so I'd add: gasLimitBoundDivisor is not taken into account either.

@tomusdrw tomusdrw changed the title minGasLimit is not taken into account when creating pending block minGasLimit/gasLimitBoundDivisor is not taken into account when creating pending block Dec 9, 2016
@rphmeier rphmeier self-assigned this Dec 11, 2016
@rphmeier
Copy link
Contributor

@lgpawel I've addressed the gas-floor going below the specified minimum, but could you check that your issue with the bound divisor is also fixed? Otherwise I will reopen.

@rphmeier
Copy link
Contributor

BTW, this is a consensus issue which could manifest if for some crazy reason the mainnet miners voted the gas limit down to practically nothing.

@lgpawel
Copy link

lgpawel commented Dec 12, 2016

I can't confirm the minGasLimit is obeyed. I use a chain spec with both this and genesis block gas limit set to 0x1df5e76 (30M*pi). The mining node runs version Parity/v1.5.0-unstable-1db4647-20161212/x86_64-linux-gnu/rustc1.13.0 with no mining-related command-line options set, and mines away happily. The other node runs Parity/v1.5.0-unstable-1db4647-20161212/x86_64-linux-gnu/rustc1.12.0 (ie. only the rust version number is different) and fails to sync with

Stage 1 block verification failed for 1d44…12b5: Block(InvalidGasLimit(OutOfBounds { min: Some(31415926), max: None, found: 31385248 }))

(1d44…12b5 is obviously the first block).

Next I've edited the chain spec so that minGasLimit is negligible, but instead gasLimitBoundDivisor is 0x1df5e76, ie. by (YP:45-46) it should keep the gas limit fixed in place, and with this setting the blocks are synced. I've also looked into the block headers and indeed the gas limit stays constant. It might well be that this part of the bug report was a false positive, sorry.

But if I increment the divisor by one, so that by the same inequalities there is actually no valid gas limit, then again the mining node imports its own blocks, whereas the other fails with

Stage 3 block verification failed for #1 (a40a…4249)
Error: Block(InvalidGasLimit(OutOfBounds { min: Some(31415926), max: Some(31415926), found: 31415927 }))

So this kind of still stands, but it probably makes no sense to fix an issue that manifests itself only (?) for chain specs for which there exist no valid blocks.

@lgpawel
Copy link

lgpawel commented Dec 14, 2016

I accidentally tested it one more time, so I now can provide logs (with probably irrelevant level of detail) and chainspecs and stuff. Now both nodes run on the same machine, but the result does not hang upon that. I did

  • git pull, cargo build --release, version is now Parity/v1.5.0-unstable-1db4647-20161212/x86_64-linux-gnu/rustc1.12.0
  • ran first node as ./parity --author 3572e93aec5b3e32a6ef8da143abdc4d3ad2e163 --unlock 3572e93aec5b3e32a6ef8da143abdc4d3ad2e163 --password ~/password --db-path ~/.mglft1 --chain ~/minGasLimitFailureTest.json -l network=trace --log-file ~/miner.log
  • ran ethminer for a few seconds until some blocks appeared
  • ran second node as ./parity --db-path ~/.mglft2/ --chain ~/minGasLimitFailureTest.json --dapps-port 8081 --jsonrpc-port 8541 --port 30301 --bootnodes enode://01478325087360db8e4c23c37a8644eaa6a0c7976887457ab8947c77f4c997ebb078fc943c95f92c105a3b2e72db0a53da9b85d86cfcf6707bc1212119132fd1@192.168.0.128:30303 -l network=trace --log-file ~/listener.log

The chain spec and the logs are at https://gist.github.com/lgpawel/8c4476182031ad86c6fc33ce4702c0b4 – the offending message is in line 95 of listener.log.

@arkpar arkpar reopened this Dec 14, 2016
@tomusdrw
Copy link
Collaborator Author

I think it was solved with #3861, @lgpawel could you confirm?

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Jan 3, 2017

Seems that the only uncovered case is when minGasLimit > gas_ceil_target.
A similar line is required also for gas_range_target.1:
https://github.com/ethcore/parity/blob/9252ebf93d0a62d6306f3306ccc08fef28a57807/ethcore/src/block.rs#L269

@lgpawel
Copy link

lgpawel commented Jan 5, 2017

Retried the steps above and the behaviour is not reproduced any more, and the gas limit stays constantly equal to the minimum. 🎉

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.
Projects
None yet
Development

No branches or pull requests

4 participants