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

Add stateful checks for chain ID and expiry height #4315

Merged
merged 3 commits into from
May 6, 2024

Conversation

plaidfinch
Copy link
Collaborator

Describe your changes

Resolves #4308.

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

This is a consensus-breaking change.

@plaidfinch plaidfinch added the consensus-breaking breaking change to execution of on-chain data label May 3, 2024
@plaidfinch plaidfinch requested review from hdevalence and erwanor May 3, 2024 21:48
@plaidfinch
Copy link
Collaborator Author

Looks like there are tests which are not setting the chain ID. Is this because the chain ID is set correctly in client tooling but not in tests, or is it not set in both?

@erwanor
Copy link
Member

erwanor commented May 4, 2024

Thanks a lot for noticing this, and taking the time to make a PR to fix it, I am very appreciative that this was caught. ACK, I'm going to push this across the finish line by fixing the test suite and destructuring TransactionParameters so that something breaks loudly if an addition is made.

@erwanor erwanor added security Issues or work related to security. _P-critical Priority: critical labels May 4, 2024
@erwanor erwanor added this to the Sprint 6 milestone May 4, 2024
@plaidfinch
Copy link
Collaborator Author

Should this be merged now? Happy to push the button myself but wanted to double-check since it contains consensus-breaking changes.

Comment on lines +38 to +41
let app_state = AppState::Content(
genesis::Content::default()
.with_epoch_duration(EPOCH_DURATION)
.with_chain_id(TestNode::<()>::CHAIN_ID.to_string()),
Copy link
Member

Choose a reason for hiding this comment

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

Just for clarification: was this necessary to make the change? It seems desirable to allow the chain ID to be "" for testing, and it seems like there's no downside since any real network will have a chain ID (which wherefore won't match).

Copy link
Member

Choose a reason for hiding this comment

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

It's not necessary, and an empty chain id would work!

@hdevalence
Copy link
Member

Let's wait to merge this until after resolving our current planner issues.

@erwanor
Copy link
Member

erwanor commented May 6, 2024

I'm going to rebase against main and merge on green ci.

@erwanor erwanor force-pushed the check-chain-id-and-expiry branch from 9a944a7 to 1e64696 Compare May 6, 2024 18:14
@erwanor erwanor merged commit 840f145 into main May 6, 2024
13 checks passed
@erwanor erwanor deleted the check-chain-id-and-expiry branch May 6, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-breaking breaking change to execution of on-chain data _P-critical Priority: critical security Issues or work related to security.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Consensus does not check transaction chain ID or expiry
4 participants