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: apply tx_pool limit #841

Merged
merged 2 commits into from
May 23, 2019
Merged

feat: apply tx_pool limit #841

merged 2 commits into from
May 23, 2019

Conversation

zhangsoledad
Copy link
Member

@zhangsoledad zhangsoledad commented May 19, 2019 •
edited by doitian
Loading

Features

  1. apply tx_pool limit
  2. tx size verify, enforce tx size below block size limit

BREAKING CHANGES

config ckb.toml

[tx_pool]
- max_pool_size = 10000
- max_orphan_size = 10000
- max_proposal_size = 10000
- max_cache_size = 1000
- max_pending_size = 10000
- txs_verify_cache_size = 100000
+ max_mem_size = 20_000_000 # 20mb
+ max_cycles = 200_000_000_000
+ max_verfify_cache_size = 100_000

rpc tx_pool_info

+ "total_tx_cycles": "2",
+ "total_tx_size": "156",

@nervos-bot nervos-bot bot changed the title Zhangsoledad/tx pool size limit [ᚬrylai] Zhangsoledad/tx pool size limit May 19, 2019
@zhangsoledad zhangsoledad changed the title [ᚬrylai] Zhangsoledad/tx pool size limit feat: apply tx_pool limit May 19, 2019
@nervos-bot nervos-bot bot changed the title feat: apply tx_pool limit [ᚬrylai] feat: apply tx_pool limit May 19, 2019
@zhangsoledad zhangsoledad force-pushed the zhangsoledad/tx_pool_size_limit branch from 358fd28 to 7c475f8 Compare May 19, 2019 08:02
@nervos-bot nervos-bot bot added the breaking change The feature breaks consensus, database, message schema or RPC interface. label May 19, 2019
@zhangsoledad zhangsoledad force-pushed the zhangsoledad/tx_pool_size_limit branch from 7c475f8 to c821626 Compare May 19, 2019 08:19
doitian
doitian requested changes May 19, 2019
View reviewed changes
rpc/json/rpc.json Outdated Show resolved Hide resolved
@doitian doitian added b:cli Break cli options and config file b:rpc Break RPC interface labels May 19, 2019
@zhangsoledad zhangsoledad force-pushed the zhangsoledad/tx_pool_size_limit branch from c821626 to bfdeca3 Compare May 19, 2019 11:07
jjyr
jjyr suggested changes May 20, 2019
View reviewed changes
shared/src/tx_pool/pool.rs Outdated Show resolved Hide resolved
doitian
doitian previously requested changes May 20, 2019
View reviewed changes
Copy link
Member

@doitian doitian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase this PR to develop.

@doitian doitian changed the base branch from rylai to develop May 20, 2019 03:33
@doitian doitian changed the title [ᚬrylai] feat: apply tx_pool limit feat: apply tx_pool limit May 20, 2019
@zhangsoledad zhangsoledad force-pushed the zhangsoledad/tx_pool_size_limit branch from bfdeca3 to 7d807d9 Compare May 20, 2019 03:38
@zhangsoledad zhangsoledad force-pushed the zhangsoledad/tx_pool_size_limit branch from 7d807d9 to b90d92c Compare May 20, 2019 06:06
@doitian doitian dismissed their stale review May 20, 2019 06:22

Rebased

@zhangsoledad zhangsoledad requested review from jjyr and a team and removed request for jjyr May 20, 2019 06:43
jjyr
jjyr suggested changes May 22, 2019
View reviewed changes
Copy link
Contributor

@jjyr jjyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it is too easy to miss size calculation in the current implementation, better to move size calculation inside the tx_pool basic interfaces.

shared/src/chain_state.rs Show resolved Hide resolved
@zhangsoledad
Copy link
Member Author

IMO, it is too easy to miss size calculation in the current implementation, better to move size calculation inside the tx_pool basic interfaces.

It's not quite easy, It's associated with error handle on different levels.
It's not considered at the beginning.
We have the plan to deal with this. :shipit: Don't waste time on this.

@doitian doitian requested a review from jjyr May 23, 2019 01:52
quake
quake reviewed May 23, 2019
View reviewed changes
verification/src/transaction_verifier.rs
pub fn new(transaction: &'a Transaction, block_bytes_limit: u64) -> Self {
SizeVerifier {
transaction,
block_bytes_limit,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use consensus max_block_bytes here, the transaction which size approaching bytes limit will be accepted and broadcasted to peers, but cannot be assembled in BlockTemplate because of the extra cellbase size.

How about adding cellbase size limit to consensus and minus this value here as gap?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's address this in #885

@doitian doitian mentioned this pull request May 23, 2019
Closed
@doitian doitian merged commit bba944e into nervosnetwork:develop May 23, 2019
This was referenced May 24, 2019
Closed
Merged
@zhangsoledad zhangsoledad deleted the zhangsoledad/tx_pool_size_limit branch August 16, 2019 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quake quake quake left review comments

@doitian doitian doitian approved these changes

@jjyr jjyr jjyr approved these changes

Assignees
No one assigned
Labels
b:cli Break cli options and config file b:rpc Break RPC interface breaking change The feature breaks consensus, database, message schema or RPC interface.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zhangsoledad @quake @doitian @jjyr