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.

Sign up for GitHub

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

Jump to bottom

feat: Introduce max_cycles_limit of a Godwoken block #767

Merged
merged 23 commits into from
Jul 28, 2022
Merged

feat: Introduce max_cycles_limit of a Godwoken block #767

merged 23 commits into from
Jul 28, 2022

Conversation

zeroqn
Copy link
Contributor

@zeroqn zeroqn commented Jul 19, 2022 •
edited
Loading

Summary

This pr adds a new cycles limit max_cycles_limit to mem block. If mem pool's available cycles is not enough, transactions will be packaged in next block. If a transaction's cycles exceeded max_cycles_limit, it will be discarded.

Motivation

A dynamic configurable way to manage number of block transaction.

Specification

Split transaction's cycles into two parts: execution cycles and virtual cycles. Only syscalls listed in SyscallCyclesConfig contribute to transaction's virtual cycles.

SyscallCyclesConfig

pub struct SyscallCyclesConfig {
    pub sys_store_cycles: u64,
    pub sys_load_cycles: u64,
    pub sys_create_cycles: u64,
    pub sys_load_account_script_cycles: u64,
    pub sys_store_data_cycles: u64,
    pub sys_load_data_cycles: u64,
    pub sys_get_block_hash_cycles: u64,
    pub sys_recover_account_cycles: u64,
    pub sys_log_cycles: u64,
}

Different between L2TX_MAX_CYCLES and block max_cycles_limit

L2TX_MAX_CYCLES Block max cycles limit
execution cycles execution cycles + virtual cycles
failed transaction discard

Implementation

  • new(config): add max_cycles_limit and syscall_cycles: SyscallCyclesConfig to MemBlockConfig
  • feat(generator): TransactionError::ExceededMaxBlockCycles and TransactionError::InsufficientPoolCycles
  • feat(rpc-server): stop package tx into mem block when mem pool reaches max_cycles_limit
  • feat(rpc-server): discard tx if it's cycles limit exceed block max_cycles_limit
  • feat(rpc-server): new TransactionError::ExceededMaxBlockCycles for gw_execute_l2transacton and gw_execute_raw_l2transaction

@gw-bot

This comment was marked as outdated.

Sign in to view
@zeroqn zeroqn force-pushed the feat-mem-pool-max-block-cycles-limit branch from e698660 to ba61a5e Compare July 19, 2022 13:07
@gw-bot

This comment was marked as outdated.

Sign in to view
@Flouse Flouse requested a review from jjyr July 19, 2022 15:40
@zeroqn zeroqn force-pushed the feat-mem-pool-max-block-cycles-limit branch from ba61a5e to 433399d Compare July 20, 2022 08:09
@gw-bot

This comment was marked as outdated.

Sign in to view
@zeroqn zeroqn force-pushed the feat-mem-pool-max-block-cycles-limit branch from 433399d to 157a62b Compare July 20, 2022 13:55
@gw-bot

This comment was marked as outdated.

Sign in to view
@zeroqn zeroqn marked this pull request as ready for review July 20, 2022 13:57
@gw-bot

This comment was marked as outdated.

Sign in to view
@gw-bot

This comment was marked as outdated.

Sign in to view
@Flouse Flouse requested review from magicalne and blckngm and removed request for blckngm July 21, 2022 06:15
@gw-bot

This comment was marked as outdated.

Sign in to view
@godwokenrises godwokenrises deleted a comment from gw-bot bot Jul 21, 2022
@zeroqn

This comment was marked as outdated.

Sign in to view
@gw-bot

This comment was marked as outdated.

Sign in to view
@zeroqn

This comment was marked as outdated.

Sign in to view
@gw-bot

This comment was marked as outdated.

Sign in to view
@zeroqn

This comment was marked as outdated.

Sign in to view
@gw-bot

This comment was marked as outdated.

Sign in to view
@gw-bot

This comment was marked as outdated.

Sign in to view
@zeroqn

This comment was marked as outdated.

Sign in to view
@gw-bot

This comment was marked as outdated.

Sign in to view
@gw-bot

This comment was marked as outdated.

Sign in to view
jjyr
jjyr requested changes Jul 22, 2022
View reviewed changes
crates/generator/src/generator.rs Outdated Show resolved Hide resolved
crates/generator/src/generator.rs Outdated Show resolved Hide resolved
crates/generator/src/generator.rs Outdated Show resolved Hide resolved
crates/generator/src/generator.rs Outdated Show resolved Hide resolved
crates/generator/src/generator.rs Outdated Show resolved Hide resolved
crates/generator/src/generator.rs Outdated Show resolved Hide resolved
crates/generator/src/syscalls/mod.rs Show resolved Hide resolved
@Flouse Flouse changed the title feat: max block cycles limit feat: Introduce max_cycles_limit of a Godwoken block Jul 25, 2022
zeroqn added 11 commits July 27, 2022 09:45
@zeroqn
refactor(generator): rename TransactionError::CyclesLimitReached
dce4b89 
Rename to TransactionError::BlockCyclesLimitReached
@zeroqn
fix(generator): tx exceed block max cycles also returns BlockCyclesLi…
e3d5755 
…mitReached
@zeroqn
refactor(rpc-server): log block cycles limit reached for tx
2e5db1b
@zeroqn
fix(rpc-server): log error on tx cycles limit exceed block max cycles
735cfd7 
Use log::info instead
@zeroqn
fix(config): u64::MAX deserialize failure for max_cycles_limit
282fee5
@zeroqn
fix(registry): remove outdated comment about queue _handle
da4cfab
@zeroqn
fix(generator): modify cycles config affect history tx run result
207efb4
@zeroqn
feat(generator): interrupt execution eariler if exceeded block max cy…
feeaed3 
…cles
@zeroqn
refactor(types): remove unused used_cycles in RunResult
7e8b618 
replace by cycles.execution
@zeroqn
refactor(generator): base on review
42d8b82 
rename BlockCyclesLimitReached to InsufficientPoolCycles
remove CyclesPool::none, use None instead
rename check_sub_cycles to consume_cycles
@zeroqn
test: only virtual cycles exceeded block max cycles
fade15e 
Execution cycles < block max cycles
Execution cycles < L2TX_MAX_CYCLES
Execution + virtual cycles > block max cycles
@zeroqn zeroqn force-pushed the feat-mem-pool-max-block-cycles-limit branch from 1ab088b to fade15e Compare July 27, 2022 01:52
@gw-bot

This comment was marked as outdated.

Sign in to view
@gw-bot

This comment was marked as outdated.

Sign in to view
@zeroqn
fix(tests): submit_withdrawal_request random failure
c14ac52 
Withdrawal request is pushed to mem pool immediately
https://github.com/nervosnetwork/godwoken/runs/7533962286?check_suite_focus=true#step:9:741

In this case, we should produce new block with withdrawal request to
update state.
@gw-bot

This comment was marked as outdated.

Sign in to view
jjyr
jjyr previously approved these changes Jul 27, 2022
View reviewed changes
@jjyr jjyr requested a review from blckngm July 27, 2022 10:37
@gw-bot

This comment was marked as outdated.

Sign in to view
@gw-bot
Copy link

gw-bot bot commented Jul 27, 2022 •
edited
Loading

Running integration test

Workflow Run Id: 2746091149

Components:

Manually running integration test

Post a comment contains

/itest
[prebuilds: tag]
[godwoken: branch/ref]
[scripts: branch/ref]
[polyjuice: branch/ref]
[web3: branch/ref]
[kicker: branch/ref]
[tests: branch/ref]

Note: [] means optional, for example

/itest
prebuilds: dev-202203280240
godwoken: develop
scripts: 81676d9d53ffdf5bbaa60483928d07da16eb4a88
polyjuice: e37553b9

Run Result

success

@jjyr jjyr merged commit 970fdae into godwokenrises:develop Jul 28, 2022
This was referenced Aug 22, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjyr jjyr jjyr approved these changes

@blckngm blckngm blckngm approved these changes

@magicalne magicalne Awaiting requested review from magicalne

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zeroqn @jjyr @blckngm