Skip to content

Remove code related to reorganization #106

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
Merged
merged 7 commits into from
Jan 22, 2020
Merged

Remove code related to reorganization #106

merged 7 commits into from
Jan 22, 2020

Conversation

@HoOngEe
Copy link
Contributor

@HoOngEe HoOngEe commented Jan 16, 2020

It closes #31

@HoOngEe HoOngEe force-pushed the refactor/remove-reorg branch 4 times, most recently from a41f868 to d322548 Compare January 17, 2020 07:29
@HoOngEe HoOngEe changed the title [WIP] Remove code related to reorganization Remove code related to reorganization Jan 17, 2020
@HoOngEe HoOngEe requested a review from sgkim126 January 17, 2020 07:30
@HoOngEe HoOngEe force-pushed the refactor/remove-reorg branch from d322548 to e60e4d3 Compare January 17, 2020 08:19
@sgkim126 sgkim126 added the refactoring Behavior does not change. Make code simple label Jan 17, 2020
sgkim126
sgkim126 requested changes Jan 17, 2020
View reviewed changes
Copy link
Contributor

@sgkim126 sgkim126 left a comment

Choose a reason for hiding this comment

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

CodeChain don't have to handle such cases anymore. -> Foundry doesn't have to handle such cases anymore. in the first commit message

sgkim126
sgkim126 reviewed Jan 17, 2020
View reviewed changes
core/src/miner/mem_pool_types.rs
/// External transaction received from network
External,
/// Transaction from retracted blocks
RetractedBlock,
Copy link
Contributor

@sgkim126 sgkim126 Jan 17, 2020

Choose a reason for hiding this comment

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

I'm not sure about it.
What happened for the transactions in the rejected proposal block?

Copy link
Contributor Author

@HoOngEe HoOngEe Jan 20, 2020

Choose a reason for hiding this comment

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

Thank you, this change is not desirable. Retracted transactions by one-block retraction due to consensus failure (or by randomized-leader-election) should be considered.

Copy link
Contributor

@majecty majecty Jan 21, 2020

Choose a reason for hiding this comment

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

@sgkim126 and @HoOngEe What do you think about removing transactions from Memory pool only when the transactions are included in a finalized block?

Then we don't need to add transactions that was removed from the Memory pool to the Memory pool again.

Copy link
Contributor Author

@HoOngEe HoOngEe Jan 21, 2020

Choose a reason for hiding this comment

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

There are two methods removing transactions from the mem pool. One is remove and the other is remove_old. There is only one explicit call remove which removes invalid transactions(txs from malicious users or verification failed txs). Also there is one explicit call remove_old in chain_new_blocks which removes old (the norm of old here is the accounts' sequences in txs are old) transactions.
This old transactions are determined by chain's accounts' sequences in the latest block. The latest block fetches best block so the transactions in blocks rejected by consensus will not be removed from the mem pool.

sgkim126
sgkim126 reviewed Jan 17, 2020
View reviewed changes
@HoOngEe
Remove test code related to reorganization
43a0d96
@HoOngEe
Remove the enum variant BranchBecomingCanonChain
72a8961 
In tendermint consensus algorithm branch is not allowed so that
Foundry don't have to handle such cases anymore.
@HoOngEe HoOngEe force-pushed the refactor/remove-reorg branch from e60e4d3 to e39dc6f Compare January 20, 2020 13:22
@sgkim126 sgkim126 requested a review from majecty January 20, 2020 13:35
@sgkim126
Copy link
Contributor

sgkim126 commented Jan 20, 2020

@majecty Would you review tendermint related parts?

sgkim126
sgkim126 requested changes Jan 20, 2020
View reviewed changes
majecty
majecty requested changes Jan 21, 2020
View reviewed changes
Copy link
Contributor

@majecty majecty left a comment

Choose a reason for hiding this comment

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

Please check how transactions are managed by the Memory pool when creating a proposal block and making a proposal block as the best block.

majecty
majecty previously approved these changes Jan 21, 2020
View reviewed changes
@HoOngEe
Remove retracted from ImportRoute
01d5dc1 
ImportRoute no longer allows retracted blocks or headers
@HoOngEe
Remove TxOrigin::RetractedBlock from mem_pool
612c5c6
@HoOngEe
No longer consider retracted txs in db
b3d80fd 
Previously, optional types are needed to handle retracted txs by
distinguishing them from enacted txs. Now, it is removed.
@HoOngEe HoOngEe force-pushed the refactor/remove-reorg branch from e39dc6f to b3d80fd Compare January 21, 2020 06:18
@sgkim126
Copy link
Contributor

sgkim126 commented Jan 21, 2020

I have a question about the commit titled "No longer consider retracted txs in db".
Doesn't we store the proposal block to prepare to reboot the node?
It seems that we should keep this code to handle the case that a proposal block is rejected, but I'm not sure. @majecty would you help me?

@majecty
Copy link
Contributor

majecty commented Jan 21, 2020

@sgkim126 We don't need to consider the proposal block.
CodeChain does not remove transactions from the Memory pool when receiving a proposal block or creating a proposal block.
Transactions in the Memory pool removed only when the best block is changed.

sgkim126
sgkim126 approved these changes Jan 21, 2020
View reviewed changes
Copy link
Contributor

@sgkim126 sgkim126 left a comment

Choose a reason for hiding this comment

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

@majecty Thank you.
@HoOngEe LGTM.

@majecty majecty merged commit c9f2afc into CodeChain-io:master Jan 22, 2020
@HoOngEe HoOngEe deleted the refactor/remove-reorg branch January 22, 2020 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@majecty majecty majecty approved these changes

@sgkim126 sgkim126 sgkim126 approved these changes

Assignees

No one assigned

Labels

refactoring Behavior does not change. Make code simple

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove code related to reorganization

3 participants

@HoOngEe @sgkim126 @majecty