Skip to content

Data column custody info #7648

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

Open
wants to merge 23 commits into
base: unstable
Choose a base branch
from

Conversation

eserilev
Copy link
Member

@eserilev eserilev commented Jun 25, 2025

Issue Addressed

#7647

Proposed Changes

Introduces a new record in the blobs db DataColumnCustodyInfo

When DataColumnCustodyInfo exists in the db this indicates that a recent cgc change has occurred and/or that a custody backfill sync is currently in progress (custody backfill will be added as a separate PR). When a cgc change has occurred earliest_available_slot will be equal to the slot at which the cgc change occured. During custody backfill syncearliest_available_slot should be updated incrementally as it progresses.

Note that if advertise_false_custody_group_count is enabled we do not add a DataColumnCustodyInfo record in the db as that would affect the status v2 response.
(See comment #7648 (comment))

If DataColumnCustodyInfo doesn't exist in the db this indicates that we have fulfilled our custody requirements up to the DA window.
(It now always exist, and the slot will be set to None once backfill is complete)

StatusV2 now uses DataColumnCustodyInfo to calculate the earliest_available_slot if a DataColumnCustodyInfo record exists in the db, if it's None, then we return the oldest_block_slot.

@eserilev eserilev requested a review from jxs as a code owner June 25, 2025 10:18
@eserilev eserilev added the das Data Availability Sampling label Jun 25, 2025
@eserilev eserilev changed the base branch from stable to unstable June 25, 2025 10:19
@eserilev eserilev added the ready-for-review The code is ready for review label Jun 25, 2025
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

@eserilev Nice, this looks great overall!
The main comment is that we should probably store the effective epoch/slot instead of the current slot - this is when the new CGC is applied in DA check.

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. database and removed ready-for-review The code is ready for review labels Jun 27, 2025
@michaelsproul michaelsproul added the backwards-incompat Backwards-incompatible API change label Jun 30, 2025
@eserilev eserilev requested a review from michaelsproul as a code owner July 2, 2025 11:43
@eserilev
Copy link
Member Author

eserilev commented Jul 2, 2025

@michaelsproul have a question re: schema version increment

This PR introduces a new column, but the database will only contain a value for this column if a recent cgc change has occurred and custody backfill hasn't finished. In that case, is a version increment necessary? An upgrade should be a no-op. A downgrade might also be a no-op because we should not delete this column from the db if a record exists as that would break our ability to track the recent cgc change

@michaelsproul
Copy link
Member

@eserilev I think a schema update is cleanest, especially if we go with my idea from here to make the DataColumnCustodyInfo stored always (when Fulu is enabled): #7648 (comment)

As Jimmy pointed out via DM, a DB downgrade that deletes the value would also guard against starting up with a stale value in the scenario where we downgrade and then re-upgrade. I.e. run the new version, downgrade, run the old version for a bit, upgrade again --> end up using the stale value from the first time we ran. However this is probably not super relevant because by the time we are running Fulu on real networks, downgrading to a prior schema should not be allowed (we could even add a check in the downgrade that only allows downgrading if fulu_fork_epoch.is_none()).

@michaelsproul
Copy link
Member

The schema_stability test will also need an upgrade for the new column. And we could add an assert about the DataColumnCustodyInfo so that future changes to it (which are probably kinda likely) are caught.

@eserilev eserilev added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jul 7, 2025
Copy link

mergify bot commented Jul 7, 2025

Some required checks have failed. Could you please take a look @eserilev? 🙏

@mergify mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jul 7, 2025
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Hey @eserilev
Looking good, i think we can merge this soon - I've left a few comments, the main one is the effective_epoch comment that needs fix I think.

@jimmygchen jimmygchen force-pushed the data-column-custody-info branch from a5a39b0 to a13bb65 Compare July 17, 2025 06:41
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

LGTM!

@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jul 17, 2025
@jimmygchen jimmygchen requested a review from michaelsproul July 18, 2025 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-incompat Backwards-incompatible API change das Data Availability Sampling database ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants