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

feat: implement non-interactive default rules for reduced padding #1152

Merged
merged 1 commit into from
Jan 3, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Dec 22, 2022

Closes #1149

@rootulp rootulp self-assigned this Dec 22, 2022
@rootulp
Copy link
Collaborator Author

rootulp commented Dec 22, 2022

💀 https://github.com/celestiaorg/celestia-app/actions/runs/3761251730/attempts/1

@codecov-commenter
Copy link

Codecov Report

Merging #1152 (505d00d) into main (8226479) will increase coverage by 0.20%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1152      +/-   ##
==========================================
+ Coverage   48.26%   48.47%   +0.20%     
==========================================
  Files          71       71              
  Lines        3999     4021      +22     
==========================================
+ Hits         1930     1949      +19     
- Misses       1899     1901       +2     
- Partials      170      171       +1     
Impacted Files Coverage Δ
app/write_square.go 92.98% <100.00%> (ø)
pkg/inclusion/paths.go 100.00% <100.00%> (ø)
pkg/shares/non_interactive_defaults.go 100.00% <100.00%> (ø)
pkg/shares/share_splitting.go 73.56% <100.00%> (ø)
pkg/shares/share_merging.go 81.64% <0.00%> (+0.21%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@evan-forbes
Copy link
Member

yeah, we should run the fuzzers locally for a while on main and then on #1147 and see if we get failures on each. naively it makes sense that we would get failures without #1147 after #1137

@rootulp rootulp marked this pull request as ready for review December 22, 2022 21:53
@rootulp
Copy link
Collaborator Author

rootulp commented Dec 22, 2022

naively it makes sense that we would get failures without #1147 after #1137

+1, hoping it gets resolved via #1137

@evan-forbes
Copy link
Member

I ran tests for a over an hour with no failures locally in #1154 after merging #1147, so I think we're back to no flakes 🤞

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

will take a second look tmrw, but LGTM so far

pkg/shares/non_interactive_defaults.go Show resolved Hide resolved
@rootulp rootulp added the consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules label Dec 23, 2022
@evan-forbes evan-forbes added this to the Incentivized Testnet milestone Jan 2, 2023
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

this LGTM, but might want to have @nashqueue take a look just to have a second pair of eyes

I'm also okay with merging this now, and @nashqueue can inspect after, as it would be nice to know if it breaks #1154

@rootulp
Copy link
Collaborator Author

rootulp commented Jan 3, 2023

sending it 🚀 can address any other PR feedback in a follow-up PR.

@rootulp rootulp merged commit 650819b into celestiaorg:main Jan 3, 2023
@rootulp rootulp deleted the rp/reduced-padding branch January 3, 2023 21:51
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.

LGTM

app/write_square_test.go Show resolved Hide resolved
rootulp added a commit that referenced this pull request Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement non-interactive default rules for reduced padding
4 participants