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

Consensus should continue working in the presense of heavy transactions #4265

Closed
mversic opened this issue Feb 7, 2024 · 11 comments · Fixed by #4957
Closed

Consensus should continue working in the presense of heavy transactions #4265

mversic opened this issue Feb 7, 2024 · 11 comments · Fixed by #4957
Assignees
Labels
Consensus This issue is related to the Sumeragi consensus iroha2-dev The re-implementation of a BFT hyperledger in RUST

Comments

@mversic
Copy link
Contributor

mversic commented Feb 7, 2024

Invalid transactions take a lot of time to process. This is probably because they require cloning of wsv. When there is a batch of invalid transactions in a block the time to create a block takes long. If max_transactions_in_block is large or commit_time is low this can prevent consensus from operating because it will never create a new block in required time.

What I can suggest at this point is that we can implement some sort of backoff mechanism to reduce the amount of transactions in a block in such cases. We can also consider straightout rejecting transactions that take too long to process. The main thing is that consensus continues operating under all circumstances

@mversic mversic added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Feb 7, 2024
@mversic mversic added the Consensus This issue is related to the Sumeragi consensus label Jun 21, 2024
@Erigara
Copy link
Contributor

Erigara commented Jul 9, 2024

What I can suggest at this point is that we can implement some sort of backoff mechanism to reduce the amount of transactions in a block in such cases.

This might not help because even single transaction in theory can block consensus.

@Erigara
Copy link
Contributor

Erigara commented Jul 9, 2024

From discussion we had with @mversic before:

As i see problem right now we have two groups of parameters which are fighting with each other.
Like f(tx_in_block, max_isi_in_tx, wasm_limit) = derived_commit_time_limit and when this derived_commit_time_limit is higher than commit_time_limit in config we are in trouble.

@Erigara Erigara changed the title Consensus should continue working in the presense of invalid transactions Consensus should continue working in the presense of heavy transactions Jul 9, 2024
@Erigara
Copy link
Contributor

Erigara commented Jul 9, 2024

Another idea was to have backoff mechanism for commit_time_limit so it increase with every view change index.
Eventually it going to be height enough to commit block.

But this might have some other concerns:

  • like if at some high view change any node in validation set going to crash peers have to wait for significant amount of time before next view change index

@mversic
Copy link
Contributor Author

mversic commented Jul 9, 2024

What I can suggest at this point is that we can implement some sort of backoff mechanism to reduce the amount of transactions in a block in such cases.

This might not help because even single transaction in theory can block consensus.

yes, but you can also define max_instructions or fuel limits to reject those

@Erigara
Copy link
Contributor

Erigara commented Jul 9, 2024

yes, but you can also define max_instructions or fuel limits to reject those

i see, we just need to make sure that consensus commit_time_limit is large enough to produce block with this single transaction given limits on fuel and max_instructions.

@Erigara
Copy link
Contributor

Erigara commented Jul 9, 2024

A final important note is that blocks themselves are bounded in size. Each block has a target size of 15 million gas but the size of blocks will increase or decrease in accordance with network demands, up until the block limit of 30 million gas (2x target block size). The total amount of gas expended by all transactions in the block must be less than the block gas limit. This is important because it ensures that blocks can’t be arbitrarily large. If blocks could be arbitrarily large, then less performant full nodes would gradually stop being able to keep up with the network due to space and speed requirements. The larger the block, the greater the computing power required to process them in time for the next slot. This is a centralizing force, which is resisted by capping block sizes.

Looking at etherium: they add per block gas limit (link).

Our blocks have kinda have such limit as well, but it may be harder to derive because it's based on limit of transactions per block and limits per transaction.

@Mingela
Copy link
Contributor

Mingela commented Jul 11, 2024

I believe we should just introduce TooComplex transaction reject reason for cases where it takes more time to produce a block (with even such the only one included) than it's configured. I'm not sure how transactions are getting selected for the block proposal but in this case it seems those should be picked one by one from the queue. Again, not sure about performance implications. Alternatively there might be a 'prepick' batch with as many as possible to be included for a potential block. The rest from which are going to be returned to the queue if not included to the block.

Though in that case we must get rid of the cases where permission validation takes enormously much time (like in the scenario I referred to previously trying to unregister an entity) first so that even a couple of little instructions get processed for a long time.

@Erigara
Copy link
Contributor

Erigara commented Jul 11, 2024

I believe we should just introduce TooComplex transaction reject reason for cases where it takes more time to produce a block (with even such the only one included) than it's configured.

This may be problematic because different peers might have different processing power and if someone claims that transaction is TooComplex, but other nodes execute it without a problem or vice versa.

@Mingela
Copy link
Contributor

Mingela commented Jul 11, 2024

I believe we should just introduce TooComplex transaction reject reason for cases where it takes more time to produce a block (with even such the only one included) than it's configured.

This may be problematic because different peers might have different processing power and if someone claims that transaction is TooComplex, but other nodes execute it without a problem or vice versa.

In that case the complexity is to become tied to fuel and not the time limit/max instructions then (for both blocks and transactions)? Should that be enough to resolve the prolem?

@Erigara
Copy link
Contributor

Erigara commented Jul 19, 2024

In that case the complexity is to become tied to fuel and not the time limit/max instructions then

Tying to fuel might be tricky because for transactions/triggers we have wasm and isi execution mode, so we would need some way to measure amount of fuel each isi cost.
And since iroha isi is not very simple there cost may very a lot: e.g. unregistering domain delete all acounts within this domain. So the cost depends on amount of accounts.

Additionally it's not clear how to measure cost of smart-contract because they consume wasm fuel and as well execute instructions.
Should they be charged for burning wasm fuel and executing of instructions?

On it's own there is nothing wrong with limiting transaction by amount of instructions because despite time this limit does not depend on processing power of node.

But we can't use it to limit smart-contracts because user can loop forever inside transaction never calling any iroha isi.

Should that be enough to resolve the prolem?

I'm still not sure, additional research is required.
Because we still need to ensure that given block limit commit time is high enough that block is committed within given deadline.

@Mingela
Copy link
Contributor

Mingela commented Jul 19, 2024

way to measure amount of fuel each isi cost

I guess that's what we will inevitably do for the public mode anyway. Some references on Ethereum and Substrate:
https://github.com/crytic/evm-opcodes (Ethereum basic operations gas cost)
https://github.com/paritytech/substrate/blob/master/frame/transaction-payment/src/lib.rs (actual fuel calculation)
https://github.com/paritytech/substrate/blob/master/frame/identity/src/weights.rs (Substrate weights initialization example)

Should they be charged for burning wasm fuel and executing of instructions?

definitely both, right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Consensus This issue is related to the Sumeragi consensus iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants