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

feat: limit tx max ancestors count #1788

Merged
merged 1 commit into from Nov 6, 2019

Conversation

@jjyr
Copy link
Member

jjyr commented Nov 1, 2019

Txs with long ancestors chain affect tx pool performance. we limit max ancestors count of a single tx to resolve this issue, tx pool will reject txs which ancestors count large than the limit.

The default max_ancestors_count is 25.

@jjyr jjyr requested a review from nervosnetwork/ckb-code-review as a code owner Nov 1, 2019
@jjyr jjyr force-pushed the jjyr:feat-limit-ancestor-count branch from 524807a to 6f708f3 Nov 1, 2019
@jjyr jjyr requested a review from nervosnetwork/ckb-dev Nov 1, 2019
@jjyr jjyr force-pushed the jjyr:feat-limit-ancestor-count branch from 6f708f3 to 49bb65a Nov 1, 2019

pool.add_entry(TxEntry::new(
tx1.clone(),
MOCK_CYCLES,
MOCK_FEE,
MOCK_SIZE,
get_related_dep_out_points(&tx1, |_| None).unwrap(),
));
))
.unwrap();

This comment has been minimized.

Copy link
@xxuejie

xxuejie Nov 1, 2019

Member

There's a lot of unwrap added here, any chance we can properly deal with the errors? Or are we sure they won't happen?

This comment has been minimized.

Copy link
@jjyr

jjyr Nov 2, 2019

Author Member

This is test code.

@jjyr jjyr force-pushed the jjyr:feat-limit-ancestor-count branch from 49bb65a to c9bcdc8 Nov 4, 2019
@jjyr jjyr requested review from xxuejie, quake and nervosnetwork/ckb-dev Nov 4, 2019
@doitian doitian added this to 👀 Awaiting review in CKB Pull Requests Nov 5, 2019
@quake
quake approved these changes Nov 5, 2019
@jjyr jjyr requested a review from nervosnetwork/ckb-dev Nov 5, 2019
self.remove_entry(&short_id)
} else {
None
};

This comment has been minimized.

Copy link
@u2

u2 Nov 6, 2019

Contributor

Why do you move this part of the code?

This comment has been minimized.

Copy link
@jjyr

jjyr Nov 6, 2019

Author Member

If ancestors reach the limit, we should just return an error and not remove the conflict tx. Other wise we should go to the old logic.

@@ -40,12 +41,16 @@ impl Future for PlugEntryProcess {
match self.target {
PlugTarget::Pending => {
for entry in entries {
tx_pool.add_pending(entry);
if let Err(err) = tx_pool.add_pending(entry) {
error!("plug entry error {}", err);

This comment has been minimized.

Copy link
@u2

u2 Nov 6, 2019

Contributor

plug?

This comment has been minimized.

Copy link
@jjyr

jjyr Nov 6, 2019

Author Member

Yes, see the file name.

CKB Pull Requests automation moved this from 👀 Awaiting review to ✅ Reviewer approved Nov 6, 2019
@xxuejie
xxuejie approved these changes Nov 6, 2019
@jjyr

This comment has been minimized.

Copy link
Member Author

jjyr commented Nov 6, 2019

bors r+

bors bot added a commit that referenced this pull request Nov 6, 2019
Merge #1788
1788: feat: limit tx max ancestors count r=jjyr a=jjyr

Txs with long ancestors chain affect tx pool performance. we limit max ancestors count of a single tx to resolve this issue, tx pool will reject txs which ancestors count large than the limit.

The default `max_ancestors_count` is 25.

Co-authored-by: jjy <jjyruby@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Nov 6, 2019

Build succeeded

  • continuous-integration/travis-ci/push
@bors bors bot merged commit c9bcdc8 into nervosnetwork:develop Nov 6, 2019
5 checks passed
5 checks passed
Dummy CI CI that does nothing
Details
Travis CI - Pull Request Build Passed
Details
bors Build succeeded
Details
nervosnetwork.ckb Build #20191104.3 succeeded
Details
nervosnetwork.ckb (UnitTest) UnitTest succeeded
Details
CKB Pull Requests automation moved this from ✅ Reviewer approved to Done Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.