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(schema): Rollback v15 ProveCommitAggregate param schema to match #907

Merged
merged 1 commit into from
Mar 10, 2022

Conversation

placer14
Copy link
Contributor

@placer14 placer14 commented Mar 9, 2022

fixes #904

@frrist frrist self-requested a review March 9, 2022 19:57

-- network v15 network upgrade epoch
WHERE height > 1594679
AND method = 'ProveCommitAggregate';
Copy link
Contributor Author

@placer14 placer14 Mar 9, 2022

Choose a reason for hiding this comment

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

This limits the update to only the messages in parsed_messages table (no other) since the upgrade epoch and for this specifically affected message type. The cost will grow the longer that v0.8.6-v0.8.7 is run before updating to v0.8.8.

Copy link
Member

Choose a reason for hiding this comment

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

This is slick!
One concern I have: we will need to apply this migration to all messages with params containing bitfields, I think this include more than just the ProveCommitAggregate params. Perhaps there is a query that could find all these types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can definitely do a manual search to see which types need adjusting and write a migration for each. 👍

@frrist frrist linked an issue Mar 9, 2022 that may be closed by this pull request
'{"rle":', params->'SectorNumbers'->>'RLE',
',"elemcount":', params->'SectorNumbers'->>'Count',
',"_type":"bitfield"}'
)::jsonb,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This creates a JSON literal which is cast to a jsonb. It interpolates the existing values into the schema with the original keys.

@placer14
Copy link
Contributor Author

placer14 commented Mar 9, 2022

After discussion aside w @frrist we aren't sure whether we want to include a migration with the fix as it would allow uncaught invariants to continue existing when we can be certain of accuracy by deleting and re-walking over the affected epochs (which would be communicated via docs/notices).

So for live node operators, the application of this fix looks something like:

  1. Upgrade to lily v0.8.8
  2. Manually execute SQL to remove incorrect data
  3. Run lily walk over affected epoch ranges on affected tasks.

This is being parked until our in-sync discussion tomorrow. Comments welcome.

@hsanjuan
Copy link
Contributor

hsanjuan commented Mar 9, 2022

This is being parked until our in-sync discussion tomorrow. Comments welcome.

I too think we can live without a migration and fix manually.

The most important thing are the dumps, which cannot be fixed by migration anyways.

@iand
Copy link
Contributor

iand commented Mar 10, 2022

Can you create an issue with the backfill label noting what needs to be backfilled, including height ranges

@placer14 placer14 force-pushed the mg/fix/v15-agg-provecommit-params branch from e37da77 to 1fcc11a Compare March 10, 2022 18:09
Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

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

LGTM, please merge after a 🟢 CI

@placer14
Copy link
Contributor Author

We agreed to remove the migration so it could be applied as required by the use case along w documentation describing the bug (which will be limited to live data). Our data archive will correct this bug with an appropriate announcement of it's completion.

@placer14 placer14 merged commit 5307f29 into master Mar 10, 2022
@placer14 placer14 deleted the mg/fix/v15-agg-provecommit-params branch March 10, 2022 18:16
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.

Aggregate Proof params schema changed in v0.8.6
4 participants