-
Notifications
You must be signed in to change notification settings - Fork 236
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
Don't account for cell dep for MAX_ANCESTORS_COUNT #4315
Don't account for cell dep for MAX_ANCESTORS_COUNT #4315
Conversation
ff5782c
to
08b362c
Compare
08b362c
to
a0290f5
Compare
fb784c0
to
4f661a7
Compare
@@ -184,7 +184,8 @@ fn test_send_transaction_exceeded_maximum_ancestors_count() { | |||
}); | |||
} | |||
|
|||
// the default value of pool config `max_ancestors_count` is 125, only 125 txs will be added to committed list of the block template | |||
// the default value of pool config `max_ancestors_count` is 125, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I found:
https://github.com/nervosnetwork/ckb/blob/4851b307b018b6bc86d2f79632223c97364f53c6/resource/ckb.toml#L140-L139
the max_ancestors_count
in default config is 25, but DEFAULT_MAX_ANCESTORS_COUNT
is 125.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can change the parameter in ckb.toml
to 125 later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a comment error, we should change the default const value to 25 also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we change const DEFAULT_MAX_ANCESTORS_COUNT: usize
to 25 too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be 125?
ckb/util/app-config/src/legacy/tx_pool.rs
Line 123 in b8ccf92
max_ancestors_count: cmp::max(DEFAULT_MAX_ANCESTORS_COUNT, max_ancestors_count), |
25 is originally referred from bitcoin, we use 125.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be min
instead of max
?
max_ancestors_count: cmp::min(DEFAULT_MAX_ANCESTORS_COUNT, max_ancestors_count),
Maybe we should only set the max_ancestors_count
to DEFAULT_MAX_ANCESTORS_COUNT
when the user doesn't specify it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it's max
.
I think this commit is not complete, since we change from 25 to 125, we'd better also change the value in ckb.toml
?
07d4db9
LGMT |
What problem does this PR solve?
We met a scenario that a
cell dep
may have many transactions as descendants, and we can not submit a new transaction to consume the dep cell.Problem Summary:
What is changed and how it works?
cell dep
should be handled specially when calculating the entry'sancestors_count
.In this PR, we add a
direct_ancestors_count
to store the directed parent-child relationship between transactions.This is a short-term solution.
What's Changed:
Related changes
owner/repo
:Check List
Tests
Side effects
Release note