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

Drop and re-create genesis_info_view #689

Merged
merged 1 commit into from
Nov 27, 2023
Merged

Drop and re-create genesis_info_view #689

merged 1 commit into from
Nov 27, 2023

Conversation

GeorgeTsagk
Copy link
Member

Closes #685

@GeorgeTsagk GeorgeTsagk self-assigned this Nov 21, 2023
ffranr
ffranr reviewed Nov 21, 2023
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

I think we should add a word or two in the commit body to explain why this change is necessary.

tapdb/sqlc/migrations/000002_assets.up.sql Show resolved Hide resolved
@GeorgeTsagk
Copy link
Member Author

The above seems to fail the postgres unit tests with

ERROR: cannot drop view genesis_info_view because other objects depend on it (SQLSTATE 2BP01)

Is the approach correct? Seems like dropping the view before re-creating it invalidates other stuff

@jharveyb
Copy link
Contributor

From offline discussion:

The key_group_info_view depends on this view and therefore also needs to be dropped and re-created in this migration.

@GeorgeTsagk GeorgeTsagk force-pushed the anchor-txid-patch branch 2 times, most recently from 2c151be to ccc5309 Compare November 21, 2023 20:19
@jharveyb
Copy link
Contributor

Ok, I reproduced the original error from #685 with approx. these steps:

  • Spin up two tapds at version v0.3.0 with separate backing lnds.
  • Mint an asset on tapd 1 and tapd 2.
  • Add tapd 1 as a federation member for tapd 2.
  • Restart tapd 2 at version v0.3.1.
  • Make an address on tapd 2 for the asset minted on tapd 1. This should succeed.
  • Restart tapd 2.
  • Trying to create any address on tapd 2 should now fail.

Restarting tapd 2 after building this branch worked and resolved the issue, but from manual inspection the DB seems to be in a weird state. The schema migrations table shows version 11 not 12 for example, and I don't see the new row in the genesis_info_view.

Not sure if 000012_anchor_txid_down.sql should be non-empty since we have the DROP commands in the up migration.

@GeorgeTsagk
Copy link
Member Author

Not sure if 000012_anchor_txid_down.sql should be non-empty since we have the DROP commands in the up migration.

Isn't the down migration supposed to revert the effects of the up? If running up leaves you with 2 views then why would down not try to delete those?

@jharveyb
Copy link
Contributor

jharveyb commented Nov 22, 2023 •
edited
Loading

Double-checked, I think the only migration missing a down is 000010, which also recreates a view. I'm not sure exactly why not having a down migration there works.

@ffranr thoughts?

@GeorgeTsagk GeorgeTsagk force-pushed the anchor-txid-patch branch 2 times, most recently from 83494cd to 719687f Compare November 22, 2023 18:35
@dstadulis dstadulis added this to the v0.3.2 milestone Nov 23, 2023
ffranr
ffranr reviewed Nov 27, 2023
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

I don't think we should worry about rolling back migrations. I think it's ok to leave the .down.sql file empty.

Is it possible to add an itest that would break without this new view? I can't see why our itests didn't catch this to begin with.

tapdb/sqlc/migrations/000002_assets.up.sql Outdated
@@ -315,7 +315,7 @@ CREATE TABLE IF NOT EXISTS asset_seedlings (
CREATE VIEW genesis_info_view AS
SELECT
gen_asset_id, asset_id, asset_tag, assets_meta.meta_data_hash meta_hash,
output_index, asset_type, genesis_points.prev_out prev_out,
output_index, asset_type, genesis_points.prev_out prev_out,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should modify an old migration file. Might make it more complicated to debug later.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR only has a trailing space as diff. Are you suggesting we revert the contents of this migration to the ones present prior to #667 ? (remove anchor_txid from this PR?)

@GeorgeTsagk
tapdb: drop and re-create genesis_info_view
ddff1af 
This change patches a previous commit which erroneously editted the
000002 migration to include an extra field in a created view. We now
make a new migration which drops the old view and creates a new one
in its place with the extra field present.
@GeorgeTsagk GeorgeTsagk requested a review from guggero November 27, 2023 16:47
jharveyb
jharveyb approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@ffranr ffranr self-requested a review November 27, 2023 20:51
ffranr
ffranr approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

@ffranr ffranr added this pull request to the merge queue Nov 27, 2023
Merged via the queue into lightninglabs:main with commit 80e7c85 Nov 27, 2023
14 checks passed
guggero
guggero reviewed Nov 27, 2023
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Post merge ACK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guggero guggero guggero left review comments

@jharveyb jharveyb jharveyb approved these changes

@ffranr ffranr ffranr approved these changes

Assignees

@GeorgeTsagk GeorgeTsagk

Labels
None yet
Projects
Taproot-Assets Project Board
Status: ✅ Done
Milestone
v0.3.2
Development

Successfully merging this pull request may close these issues.

[bug]: Can't transfer assets: unknown sqlite error: SQL logic error: no such column: anchor_txid (1)
5 participants
@GeorgeTsagk @jharveyb @guggero @ffranr @dstadulis