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

Consensus does not check transaction chain ID or expiry #4308

Closed
plaidfinch opened this issue May 3, 2024 · 0 comments · Fixed by #4315
Closed

Consensus does not check transaction chain ID or expiry #4308

plaidfinch opened this issue May 3, 2024 · 0 comments · Fixed by #4315
Assignees
Labels
C-bug Category: a bug consensus-breaking breaking change to execution of on-chain data E-easy Effort: Easy _P-critical Priority: critical _P-V1 Priority: slated for V1 release

Comments

@plaidfinch
Copy link
Collaborator

plaidfinch commented May 3, 2024

Describe the bug

A transaction which contains a mismatched chain ID or an expired expiry height is currently accepted by the chain.

To Reproduce

I have not reproduced this on a testnet, but there is currently, to my ability to determine, no code which checks these properties. It should be in the historical check method right next to the FMD parameters check, per @erwanor.

Expected behavior

If a transaction is submitted with exactly the current chain ID, it should be accepted. Otherwise, it should be rejected. Note that the empty chain ID string "" should not be treated specially here. The empty chain ID should always cause a transaction to fail, because there will never be a chain with an empty chain ID. This is distinct from the current RPC behavior of "failing open" and accepting requests with an empty chain ID, so that you can request information without first acquiring the chain ID.

If a transaction is submitted with a zero expiry height or an expiry height greater than the current height, it should be accepted. Otherwise, it should be rejected.

Additional context

Expiry height was at one point going to be converted to time-based, but this was closed out because of proto stabilization. It's my opinion that we should enforce the field, rather than leaving it in a state that is documented as being enforced, but not enforced. The alternative would be to change the documentation to deprecate it.

This is technically a consensus-breaking change because it makes previously-valid transactions invalid, although no client tooling currently produces such transactions.

@plaidfinch plaidfinch added C-bug Category: a bug E-easy Effort: Easy consensus-breaking breaking change to execution of on-chain data labels May 3, 2024
@plaidfinch plaidfinch self-assigned this May 3, 2024
@github-project-automation github-project-automation bot moved this to Backlog in Penumbra May 3, 2024
@github-actions github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label May 3, 2024
@erwanor erwanor added _P-critical Priority: critical _P-V1 Priority: slated for V1 release and removed needs-refinement unclear, incomplete, or stub issue that needs work labels May 4, 2024
erwanor added a commit that referenced this issue May 6, 2024
## Describe your changes

Resolves #4308.

## Checklist before requesting a review

- [X] 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.**

---------

Co-authored-by: Erwan Or <[email protected]>
@github-project-automation github-project-automation bot moved this from Backlog to Done in Penumbra May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: a bug consensus-breaking breaking change to execution of on-chain data E-easy Effort: Easy _P-critical Priority: critical _P-V1 Priority: slated for V1 release
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants