Skip to content

feat: add CellsCommitments softfork#4090

Closed
quake wants to merge 9 commits intonervosnetwork:developfrom
quake:quake/live-cells
Closed

feat: add CellsCommitments softfork#4090
quake wants to merge 9 commits intonervosnetwork:developfrom
quake:quake/live-cells

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
@quake quake force-pushed the quake/live-cells branch from 842d5dc to 75d10f7 Compare August 15, 2023 01:30
.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.

@github-actions
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
@github-actions
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

Comments