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: add CellsCommitments softfork #4090

Closed
wants to merge 9 commits into from

Conversation

quake
Copy link
Member

@quake quake commented Aug 4, 2023

What problem does this PR solve?

This PR adds a softfork implementation of CellsCommitments that can be used to prove transaction output (cell) existence and status (live / dead).

What is changed and how it works?

The implementation uses the merkle mountian range (MMR) as internal data structure, the hash of cell status is stored as MMR element. This PR is still in draft, and is created only for early code review. Please noted that no additional unit or integration tests have been added to this PR, as the relevant code logic has already been covered by the original tests, e.g. whether the MMR root was updated correctly when the chain was reorging.

There are 4 more TODOs.

  • Because the transactions in the block will affect the MMR root, the original way of adding transactions by blank block template without modifying the extension field is no longer suitable for the integration test, there are about 30+ integration tests that need to be modified, and some of them have been modified so far, and need to be handled in a unified way.

  • lack of 2 RPC to generate and verify MMR proof.

  • DB migration, there may be a need for an efficient way to update MMR for historical data.

  • the corresponding RFC is being prepared, and will be submitted together with the RFC when this PR is formally submitted. RFC: Cells Commitments [WIP - DO NOT MERGE] rfcs#424

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • There may be performance regression, need more test on mainnet and testnet real data.

Release note

Note: Add a note under the PR title in the release note.

@quake quake requested a review from a team as a code owner August 4, 2023 01:09
@quake quake requested review from doitian and removed request for a team August 4, 2023 01:09
@quake quake marked this pull request as draft August 4, 2023 01:46
@quake quake changed the title feat: add TxoCommitments softfork feat: add CellsCommitments softfork Aug 15, 2023
.push(hash)
.map_err(|e| InternalErrorKind::MMR.other(e))?;
let cell_status = CellStatus::new(mmr_position, block_number);
txn.insert_cells_root_mmr_status(&out_point, &cell_status)?;
Copy link
Member

Choose a reason for hiding this comment

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

Ensure to add test cases that cells created in the block are consumed in other transactions in the same block.

Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale To be closed due to a lack of activity label Jul 30, 2024
Copy link

github-actions bot commented Aug 9, 2024

This PR was closed because it has been stalled for 10 days with no activity.

@github-actions github-actions bot closed this Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale To be closed due to a lack of activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants