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: add cellbase maturity verification #367

Closed
wants to merge 1 commit into from
Closed

🐞✋ feat: add cellbase maturity verification #367

wants to merge 1 commit into from

Conversation

u2
Copy link
Contributor

@u2 u2 commented Mar 28, 2019 •
edited by zhangsoledad

@quake
Copy link
Member

quake commented Mar 28, 2019

@u2 I proposal a refactoring: move max_cycles to ChainState struct fields, which will simplify add_tx_to_pool / verify_rtx / etc functions' parameters list. I think that cellbase_maturity can be handled same way, what's your thoughts on this?

@u2
Copy link
Contributor Author

u2 commented Mar 28, 2019

@u2 I proposal a refactoring: move max_cycles to ChainState struct fields, which will simplify add_tx_to_pool / verify_rtx / etc functions' parameters list. I think that cellbase_maturity can be handled same way, what's your thoughts on this?

Agree.

@u2 u2 changed the title [WIP] feature: cellbase maturity verify feature: cellbase maturity verify Apr 1, 2019
@u2
Copy link
Contributor Author

u2 commented Apr 1, 2019

@quake updated

@doitian doitian requested a review from quake April 2, 2019 01:26
@doitian doitian added the s:waiting-on-reviewers Status: Waiting for Review label Apr 2, 2019
@quake
Copy link
Member

quake commented Apr 2, 2019

Please hold until this #372 is merged, after that, we can refactor verify_rtx fn to:

chain_state.verify_rtx(&rtx);

quake
quake reviewed Apr 2, 2019
View reviewed changes
chain/src/tests/basic.rs Outdated Show resolved Hide resolved
jjyr
jjyr reviewed Apr 3, 2019
View reviewed changes
verification/src/transaction_verifier.rs
let immature_spend = self.transaction.inputs().iter().any(|input| {
match cell_set.get(&input.previous_output.hash) {
Some(ref meta)
if meta.is_cellbase()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not every cell outpoint in cellbase need to be maturity, I think we need more discussion on this?

Consider the UDT tx fee design, maybe we should leave more rooms for upper UDT contracts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not every cell outpoint in cellbase need to be maturity, I think we need more discussion on this?

Consider the UDT tx fee design, maybe we should leave more rooms for upper UDT contracts?

I think it should be because of the block confirmation. The UDT tx fee would be included in the cellbase?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not decide yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seem your implement is correct due to the explanation of maturity in this link https://bitcoin.stackexchange.com/questions/1991/what-is-the-block-maturation-time

Anyway please hold this PR until #372 merged, 372 introduce few refactoring about tx verification.

@jjyr jjyr changed the title feature: cellbase maturity verify [HOLD]feature: cellbase maturity verify Apr 3, 2019
@jjyr jjyr added the s:discussion-needed Status: Need to Discuss label Apr 3, 2019
doitian
doitian requested changes Apr 11, 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 latest develop and ensure:

  • Make this a configurable option in chain spec.
  • Only enable it in testnet chain spec. Disable it in dev and integration.

@doitian
Copy link
Member

doitian commented Apr 12, 2019

@jjyr is going to take a look at this PR.

@quake Do you prefer to merge it after your store refactoring, or it doesn't matter.

@quake
Copy link
Member

quake commented Apr 12, 2019

it doesn't matter.

@u2
Copy link
Contributor Author

u2 commented Apr 12, 2019

Updated,
https://github.com/nervosnetwork/ckb/pull/367/files#diff-b2197becbc6452c42618e284e3dbf52eR226
But I am not sure whether the cellbase_maturity and cell_set are correct, because the max_cycles coming from the function params, and the cellbase_maturity coming from the self.consensus.

@doitian doitian changed the title [HOLD]feature: cellbase maturity verify ✋feature: cellbase maturity verify Apr 13, 2019
@doitian doitian changed the title ✋feature: cellbase maturity verify ✋ feature: cellbase maturity verify Apr 13, 2019
@doitian doitian changed the title ✋ feature: cellbase maturity verify ✋ feat: add cellbase maturity verification Apr 14, 2019
@doitian doitian assigned doitian and jjyr and unassigned doitian Apr 14, 2019
@doitian doitian changed the title ✋ feat: add cellbase maturity verification 🐞✋ feat: add cellbase maturity verification Apr 14, 2019
@doitian doitian added the 500 label Apr 14, 2019
@doitian
Copy link
Member

doitian commented Apr 14, 2019

I'm sorry but it seems that @github is not going to fix the 500 issue. The team could not open this PR in desktop browser, it is inconvenient to discuss in this PR. Could you please close this and create a new PR instead?

@u2 u2 closed this Apr 15, 2019
@u2 u2 mentioned this pull request Apr 15, 2019
Merged
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

@jjyr jjyr jjyr left review comments

@doitian doitian doitian requested changes

Assignees

@jjyr jjyr

Labels
s:discussion-needed Status: Need to Discuss s:waiting-on-reviewers Status: Waiting for Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@u2 @quake @doitian @jjyr