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

Provide cutoff to block proposer to avoid wasting work #1354

Closed
xfylsj opened this issue Jan 7, 2019 · 7 comments
Closed

Provide cutoff to block proposer to avoid wasting work #1354

xfylsj opened this issue Jan 7, 2019 · 7 comments
Assignees
Labels
I1-security 👮‍♀️ P5-sometimesoon Q5-substantial
Milestone

Comments

@xfylsj
Copy link

@xfylsj xfylsj commented Jan 7, 2019

Hi Polkadot and Substrate team, we're trying to test how many transactions can be handled in one block, and encountered the 'block production took too long' issue when injecting 100 transactions:
image

I'm running a single node network on my local machine (Macbook Pro 2017, Mac OS High Sierra)

We believe the block can handle more than 100 transaction.

Could you please advise us how to fix the issue? Thanks a lot.

@rphmeier
Copy link
Member

@rphmeier rphmeier commented Jan 7, 2019

Are you running in release or debug mode?

In Aura consensus we cannot author blocks that take longer than the block time to produce.

@rphmeier rphmeier added the Z0-unconfirmed label Jan 7, 2019
@xfylsj
Copy link
Author

@xfylsj xfylsj commented Jan 7, 2019

@gavofyork
Copy link
Member

@gavofyork gavofyork commented Jan 9, 2019

we're trying to test how many transactions can be handled in one block

I guess you found your answer :)

@xlc
Copy link
Contributor

@xlc xlc commented Jan 9, 2019

I think this is a bad performance issue that could impact the liveness of the network.

The block producer should be aware the amount of the time consumed while producing the block and propose the block before the timeout reached. i.e. Construct block with 60 transactions and propose it, instead of construct block with 100 transactions and failed to propose it within the time limit.

The trouble is the that currently only limitations on how many transactions can be put into a block is block size (and total gas fee for smart contracts), which does not consider the actual computation time of all other transactions. This is fine if all transactions are fast (i.e. impossible to causing block construction time > block proposer time limit), which I don't see how feasible to enforce that.

Therefore it is very possible that the block producer trying to put too many slow transactions (in this case, balance transfer, which shouldn't be slow at all) into the block and failed to construct the block within the time limit and causing liveness issue (if all validators are doing the same thing).

Otherwise it is just another attack vector to substrate-based network. Just need to find out the most effective computation expensive transaction (consider speed and tx size and tx fee to find most cost effective method) and spam it. If such tx exists, the network will be vulnerable.

I see two areas needs to improved

  1. Ensure block producer always construct and propose block within the time limit
  2. Optimize the block construction time

@rphmeier rphmeier added I7-optimisation and removed Z0-unconfirmed labels Jan 11, 2019
@rphmeier rphmeier changed the title Node stopped working when injected 100 transactions into one block Provide cutoff to block proposer to avoid wasting work Jan 11, 2019
@rphmeier
Copy link
Member

@rphmeier rphmeier commented Jan 11, 2019

The simple way to do this would be to extend the Proposer trait to take a Future (provided by the consensus engine) that resolves when the proposer should cease adding transactions to the block.

The main problem I see here is that sealing and generating a state root will take more time, the more transactions/work is done. Or that there may be large/bulky transactions that cannot be cut off asynchronously.

Releasing outside of the step is still likely a problem as long as the timestamp is set correctly, but probably not a huge one as long as it happens rarely. It will often mean that the block, although still authored, will be orphaned.

@rphmeier
Copy link
Member

@rphmeier rphmeier commented Jan 11, 2019

Actually, looking closer at the logs provided, what's happening is that import is doing the work again, so we could ~double throughput via #1232 .

Prior comment still stands.

@gavofyork
Copy link
Member

@gavofyork gavofyork commented Jan 14, 2019

In general, it should be up to the chain logic to figure out when there are "too many transactions" in the block. Ethereum does this with a gas limit, Bitcoin with a block size limit. Substrate shouldn't really prejudice what strategy be used, only that it's possible to state one. For now, it should probably just be an alternative error return variant in block_builder_api::BlockBuilder::apply_extrinsic. Eventually we'll likely want some more abstract API that exposes a linear per-transaction metric allowing much more efficient selection.

@gavofyork gavofyork added I1-security 👮‍♀️ P5-sometimesoon Q5-substantial and removed I7-optimisation labels Jan 14, 2019
@gavofyork gavofyork added this to the 1.0 (final) milestone Jan 14, 2019
@gavofyork gavofyork added this to To Do in Runtime via automation Jan 14, 2019
@tomusdrw tomusdrw self-assigned this Jan 18, 2019
Runtime automation moved this from To Do to Done Feb 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I1-security 👮‍♀️ P5-sometimesoon Q5-substantial
Projects
Runtime
  
Archive
SDK Node (deprecated)
  
Awaiting triage
Development

No branches or pull requests

5 participants