Skip to content

Spec for staking module differences #1220

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
Closed
S1nus wants to merge 7 commits into from
Closed

Spec for staking module differences #1220

S1nus wants to merge 7 commits into from

Conversation

@S1nus
Copy link
Member

@S1nus S1nus commented Oct 2, 2023

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023 •
edited
Loading

PR Preview Action v1.4.4
🚀 Deployed preview to https://rollkit.github.io/rollkit/pr-preview/pr-1220/
on branch gh-pages at 2023-10-02 21:51 UTC

@codecov
Copy link

codecov bot commented Oct 2, 2023 •
edited
Loading

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (eb7bdc0) 56.08% compared to head (513f44d) 56.07%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1220      +/-   ##
==========================================
- Coverage   56.08%   56.07%   -0.02%     
==========================================
  Files          63       63              
  Lines        6820     6820              
==========================================
- Hits         3825     3824       -1     
- Misses       2606     2607       +1     
  Partials      389      389

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@S1nus S1nus marked this pull request as ready for review October 2, 2023 22:31
MSevey
MSevey reviewed Oct 3, 2023
View reviewed changes
specs/src/specs/staking_module.md

Rollkit will support customizable functionality for verifying the correctness of a block proposer. However, it currently works by delegating this functionality to the Application, by checking that the `AggregatorsHash` of a header corresponds to the `NextAggregatorsHash` of the previous, where setting the `NextAggregtorsHash` is the responsibility of the ABCI Application.

## Assumptions and Considerations
Copy link
Contributor

@MSevey MSevey Oct 3, 2023

Choose a reason for hiding this comment

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

We should include here our assumptions around how tendermint staking works.
This would allow auditors to better verify our assumptions.

Copy link
Member Author

@S1nus S1nus Oct 3, 2023

Choose a reason for hiding this comment

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

"tendermint staking" does not exist. the ABCI App simply sets the validator set

Copy link
Member

@nashqueue nashqueue Oct 4, 2023

Choose a reason for hiding this comment

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

Yes, we use the staking module for that.

nashqueue
nashqueue requested changes Oct 4, 2023
View reviewed changes
Copy link
Member

@nashqueue nashqueue left a comment

Choose a reason for hiding this comment

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

This spec aims to check how we use the staking module vs. how it is used in tendermint. Please elaborate on that.

specs/src/specs/staking_module.md

Rollkit will support customizable functionality for verifying the correctness of a block proposer. However, it currently works by delegating this functionality to the Application, by checking that the `AggregatorsHash` of a header corresponds to the `NextAggregatorsHash` of the previous, where setting the `NextAggregtorsHash` is the responsibility of the ABCI Application.

## Assumptions and Considerations
Copy link
Member

@nashqueue nashqueue Oct 4, 2023

Choose a reason for hiding this comment

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

Yes, we use the staking module for that.

@S1nus
Copy link
Member Author

S1nus commented Oct 4, 2023 •
edited
Loading

This spec aims to check how we use the staking module vs. how it is used in tendermint. Please elaborate on that.

don't we use it in exactly the same way?

@nashqueue
Copy link
Member

nashqueue commented Oct 5, 2023

This spec aims to check how we use the staking module vs. how it is used in tendermint. Please elaborate on that.

don't we use it in exactly the same way?

The point of this spec is to exactly figure this out and to make sure to capture the nuances if there are differences.

@Manav-Aggarwal Manav-Aggarwal marked this pull request as draft November 8, 2023 14:38
@S1nus
Copy link
Member Author

S1nus commented Nov 14, 2023 •
edited
Loading

As of #1291, we decided that Rollkit vA will control the proposer entirely, ignoring all validator set updates from the ABCI app.

This is being implemented in the Header.Verify(...) method, where it enforces that the ProposerAddress must be equal to the ProposerAddress of the previous header. If implemented properly, this ensures the entire chain is signed by the same person (the centralized sequencer).

@S1nus S1nus closed this Nov 14, 2023
@S1nus S1nus mentioned this pull request Nov 14, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 20, 2023
@S1nus
add staking module differences to block_spec (#1335)
9434283 
closes #1220

Instead of #1220, we'll just fix
and update the block_spec.

Also adds the termination condition, and processing out-of-order rollup
blocks from DA.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Introduced a requirement for blocks to have a valid signature from the
centralized sequencer.
- Implemented support for out-of-order rollup blocks on the Data
Availability network.
- Added a termination condition for sequencer double-signing scenarios.

- **Documentation**
- Updated block specification documentation to reflect new validation
rules.
- Added diagrams to illustrate out-of-order block handling and
termination conditions.

- **Refactor**
- Removed `AggregatorsHash` and `NextAggregatorsHash` fields from block
headers to streamline the validation process.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Connor O'Hara <connor@switchboard.xyz>
gupadhyaya pushed a commit that referenced this pull request Nov 24, 2023
@S1nus
add staking module differences to block_spec (#1335)
aa3dd3f 
closes #1220

Instead of #1220, we'll just fix
and update the block_spec.

Also adds the termination condition, and processing out-of-order rollup
blocks from DA.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Introduced a requirement for blocks to have a valid signature from the
centralized sequencer.
- Implemented support for out-of-order rollup blocks on the Data
Availability network.
- Added a termination condition for sequencer double-signing scenarios.

- **Documentation**
- Updated block specification documentation to reflect new validation
rules.
- Added diagrams to illustrate out-of-order block handling and
termination conditions.

- **Refactor**
- Removed `AggregatorsHash` and `NextAggregatorsHash` fields from block
headers to streamline the validation process.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Connor O'Hara <connor@switchboard.xyz>
@tac0turtle tac0turtle deleted the connor/staking-module-spec branch March 20, 2025 11:57
@claude claude bot mentioned this pull request Jan 20, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nashqueue nashqueue nashqueue requested changes

@Manav-Aggarwal Manav-Aggarwal Awaiting requested review from Manav-Aggarwal

@tzdybal tzdybal Awaiting requested review from tzdybal

+1 more reviewer

@MSevey MSevey MSevey left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@S1nus S1nus

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@S1nus @nashqueue @MSevey