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

fix(state-transition): Correctly enforce deposits length to avoid panics #2369

Merged
merged 7 commits into from
Jan 18, 2025

Conversation

calbera
Copy link
Contributor

@calbera calbera commented Jan 18, 2025

We didn't have a length check before. If a verifying node did not have up-to-date the latest deposits that are included in block, we would panic in their process proposal (OR finalize block). This PR fixes by erroring to REJECT the proposal in this case.

@calbera calbera requested a review from a team as a code owner January 18, 2025 17:24
Copy link

codecov bot commented Jan 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 31.21%. Comparing base (ce54f45) to head (21c8617).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2369       +/-   ##
===========================================
+ Coverage   21.17%   31.21%   +10.03%     
===========================================
  Files           3      346      +343     
  Lines          85    15426    +15341     
  Branches       20       20               
===========================================
+ Hits           18     4815     +4797     
- Misses         66    10273    +10207     
- Partials        1      338      +337     
Files with missing lines Coverage Δ
state-transition/core/state_processor_staking.go 54.26% <100.00%> (ø)
state-transition/core/validation_deposits.go 48.33% <100.00%> (ø)

... and 341 files with indirect coverage changes

@@ -107,3 +105,76 @@ func TestInvalidDeposits(t *testing.T) {
require.Error(t, err)
require.ErrorContains(t, err, "deposit mismatched")
}

func TestInvalidDepositsCount(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

previously this test would panic

@abi87 abi87 enabled auto-merge (squash) January 18, 2025 18:44
@abi87 abi87 merged commit 7b02411 into main Jan 18, 2025
19 checks passed
@abi87 abi87 deleted the fix-deps-count branch January 18, 2025 18:44
shotes pushed a commit that referenced this pull request Feb 3, 2025
shotes pushed a commit that referenced this pull request Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants