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.

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

fix(modda): correct slice appends #3267

Merged
merged 2 commits into from
Mar 25, 2024
Merged

fix(modda): correct slice appends #3267

merged 2 commits into from
Mar 25, 2024

Conversation

tsite
Copy link
Contributor

@tsite tsite commented Mar 24, 2024

No description provided.

@github-actions github-actions bot added the external Issues created by non node team members label Mar 24, 2024
walldiss
walldiss approved these changes Mar 25, 2024
View reviewed changes
Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this one!

Wondertan
Wondertan reviewed Mar 25, 2024
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Might need a review from @tzdybal as an author of the code

@walldiss
Copy link
Member

The auther per github is @distractedm1nd See:
#3146

@Wondertan
Copy link
Member

Ryan just copy-pasted code from go-da instead of merging histort

@Wondertan
Copy link
Member

This is the original code https://github.com/rollkit/celestia-da/blob/main/celestia/celestia.go#L158-L178.
Would be great to submit this fix there @tsite 🙏🏻

@tsite tsite mentioned this pull request Mar 25, 2024
Closed
@tsite
Copy link
Contributor Author

tsite commented Mar 25, 2024

created rollkit/celestia-da#90

@tsite
Copy link
Contributor Author

tsite commented Mar 25, 2024

I think the rollkit version is okay actually and the bug only exists in celestia-node

@Wondertan
Copy link
Member

Wondertan commented Mar 25, 2024 •
edited
Loading

That's right, @tsite. Appends on empty slice work correctly there, but less optimized because it does not preallocate. Seems like the code was slightly bugified during the copy process 😄. Thanks for catching this again!

@Wondertan Wondertan enabled auto-merge (squash) March 25, 2024 20:13
@Wondertan Wondertan added the kind:fix Attached to bug-fixing PRs label Mar 25, 2024
@Wondertan Wondertan changed the title fix bug in da.Validate api fix(modda): correct slice appends Mar 25, 2024
@Wondertan Wondertan merged commit c1b41b0 into celestiaorg:main Mar 25, 2024
24 checks passed
renaynay pushed a commit to renaynay/celestia-node that referenced this pull request Apr 3, 2024
@tsite @renaynay
fix bug in da.Validate api (celestiaorg#3267)
d8677ce
renaynay pushed a commit to renaynay/celestia-node that referenced this pull request Apr 3, 2024
@tsite @renaynay
fix bug in da.Validate api (celestiaorg#3267)
0b4aba9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wondertan Wondertan Wondertan approved these changes

@walldiss walldiss walldiss approved these changes

@renaynay renaynay Awaiting requested review from renaynay renaynay is a code owner

@vgonkivs vgonkivs Awaiting requested review from vgonkivs vgonkivs is a code owner

@distractedm1nd distractedm1nd Awaiting requested review from distractedm1nd

@ramin ramin Awaiting requested review from ramin

Assignees

@tsite tsite

Labels
external Issues created by non node team members kind:fix Attached to bug-fixing PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tsite @walldiss @Wondertan