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: deepcopy filliing nil data with zeroes #310

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

walldiss
Copy link
Member

@walldiss walldiss commented Apr 9, 2024

Corrects the bug introduced by #100. Currently, if deepcopy() encounters nil inner slice values, it creates a slice of 0 length, which is incorrect. This PR addresses the issue. The problem becomes evident when eds is partially computed, resulting in Row() or Col() methods returning invalid copies.

On the side note, it is technically breaking public API change. But the nature of the bug suggests noone relied on existing misbehaviour before.

Copy link

codecov bot commented Apr 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.30%. Comparing base (d6c118c) to head (e560ef8).
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #310      +/-   ##
==========================================
+ Coverage   80.89%   82.30%   +1.40%     
==========================================
  Files           8        7       -1     
  Lines         869      616     -253     
==========================================
- Hits          703      507     -196     
+ Misses        119       66      -53     
+ Partials       47       43       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Wondertan
Copy link
Member

The rust mind cannot comprehend this 😂

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.

Minor correction: The bug was there before the linked PR. The PR extracted existing code as is.

@walldiss
Copy link
Member Author

walldiss commented Apr 9, 2024

I wish golang had immutables too 🤝
Updated desc, bug was introduced in previous PR #100

@walldiss walldiss merged commit 18f79c2 into celestiaorg:main Apr 15, 2024
5 checks passed
Wondertan pushed a commit to celestiaorg/celestia-node that referenced this pull request Apr 23, 2024
…struction (#3306)

We should not restrict the BEFP constructor to collect proof only for orthogonal axes to ErrByzantine. This PR enables the constructor to attempt building both Row and Col proofs, irrespective of the ErrByzantine axis type. Additionally, it prevents the BEFP constructor from requesting proofs pr shars from the network by granting access exclusively to the local blockstore. There should be sufficient proofs and shares in the local blockstore at the time BEFP is detected.

Breaks befp message, by introducing proofAxis field, which is not supported by older befp subscribers.

Depends on 
- #3305 to have enough data in blockstore.
- celestiaorg/rsmt2d#310 to provide proper coordinates of verified shares
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.

4 participants