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 assets SQL table unique constrinat #691

Closed
ffranr opened this issue Nov 21, 2023 · 3 comments · Fixed by #965
Closed

Fix assets SQL table unique constrinat #691

ffranr opened this issue Nov 21, 2023 · 3 comments · Fixed by #965
Assignees
@guggero
Labels
enhancement New feature or request needs triage refactoring sqlc sqlite

Comments

@ffranr
Copy link
Contributor

ffranr commented Nov 21, 2023 •
edited
Loading

The assets SQL table's unique constraint is ineffective (it's a no-op). It doesn't do what we intend it to do.

The current unique constraint is on the fields:

asset_id, genesis_id, script_key_id

but asset_id is the table's primary key integer and not the asset ID byte array. This means that each row is unique and that duplicate assets can be inserted into the table.

We add a unique constraint index with the following fields to the table:

anchor_utxo_id, genesis_id, script_key_id

With this new constraint we should not be able to add duplicate assets into the table.

Adding the new constraint will mean deleting rows from the assets table which violate this new constraint.

Another change that follows naturally from this modification is to change the InsertNewAsset SQL statement into an upsert.

This issue has caused this bug #665 but another solution has now addressed that bug.

PR #684 is an attempt at fixing this issue.
@ffranr ffranr linked a pull request Nov 21, 2023 that will close this issue
Change SQL statement InsertNewAsset into an upsert. #684
Closed
@dstadulis
Copy link
Collaborator

I had started writing an issue for these needs. Feel free to edit or lift any relevant elements.

Issue title:

Test duplicate address receive flow + DB uniqueness

Description:

This test will ensure that if a client tries to send to two duplicate addresses (say in the same transfer), that the daemon throw an error, we'll want to note if it gets clobbered in an internal map. It's important to make sure this is tested to avoid any potential issues with transactions being processed incorrectly. The unit tests should cover the scenarios:

  1. Global/DB uniqueness: Test the outpoint + assetID + scriptKey are indeed globally unique and sufficient to ensure unique DB location and compatible with address reuse.

  2. Batch collision: internalKey + assetID + scriptKey don't collide within a batch.

  3. Outpoint collision: assetID + scriptKey don't collide within an outpoint.

  4. Duplicate Sends: Test should also include a scenario where a client sends to multiple different addresses, but one of them is duplicated - to test if only the duplicates get rejected or if all transactions fail due to an error.

@ffranr
Copy link
Contributor Author

ffranr commented Nov 21, 2023 •
edited by dstadulis
Loading

We should complete issue #694 before addressing this issue.
#691 is ready

@ffranr ffranr mentioned this issue Nov 27, 2023
Closed
@guggero guggero mentioned this issue Mar 5, 2024
Closed
guggero added a commit that referenced this issue Jun 24, 2024
@guggero
sqlc/migrations: add new unique key to assets table
1453a71 
Fixes #691 by adding a unique constraint that will actually work. The
assumption of the constraint is that there will never be an asset of the
same asset ID with the same script key in the same anchor transaction
output. This would be impossible anyway because the asset leaves would
collide within the asset-level MS-SMT tree.
So this unique key is safe to use as an upsert detection mechanism.
@guggero guggero mentioned this issue Jun 24, 2024
Merged
guggero added a commit that referenced this issue Jun 24, 2024
@guggero
sqlc/migrations: add new unique key to assets table
aa8b251 
Fixes #691 by adding a unique constraint that will actually work. The
assumption of the constraint is that there will never be an asset of the
same asset ID with the same script key in the same anchor transaction
output. This would be impossible anyway because the asset leaves would
collide within the asset-level MS-SMT tree.
So this unique key is safe to use as an upsert detection mechanism.
guggero added a commit that referenced this issue Jun 24, 2024
@guggero
sqlc/migrations: add new unique key to assets table
1b21bdf 
Fixes #691 by adding a unique constraint that will actually work. The
assumption of the constraint is that there will never be an asset of the
same asset ID with the same script key in the same anchor transaction
output. This would be impossible anyway because the asset leaves would
collide within the asset-level MS-SMT tree.
So this unique key is safe to use as an upsert detection mechanism.
@guggero guggero removed a link to a pull request Jun 24, 2024
Change SQL statement InsertNewAsset into an upsert. #684
Closed
@guggero guggero linked a pull request Jun 24, 2024 that will close this issue
Fix duplicate asset creation in itest #965
Merged
@dstadulis dstadulis moved this from 🆕 New to 👀 In review in Taproot-Assets Project Board Jun 24, 2024
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Taproot-Assets Project Board Jun 24, 2024
@gijswijs
Copy link
Contributor

Removing the old constraint is still an open issue #967

@gijswijs gijswijs mentioned this issue Jun 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@guggero guggero

Labels
enhancement New feature or request needs triage refactoring sqlc sqlite
Projects
Taproot-Assets Project Board
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix duplicate asset creation in itest lightninglabs/taproot-assets
4 participants
@guggero @ffranr @gijswijs @dstadulis