Fix duplicate asset creation in itest #965
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gijswijs
force-pushed
the
postgres-clobber
branch
from
651af43 to
1083cf1
Compare
guggero
force-pushed
the
duplicate-asset-insert
branch
from
bceae3b to
dd8ba78
Compare
gijswijs
reviewed
Jun 24, 2024
There was a problem hiding this comment.
This PR touches ApplyPendingOutput which creates new assets. We still need to address Oli's comment about not reusing lock_time and relative_lock_time from the spent asset. See:
taproot-assets/tapdb/assets_store.go
Lines 2781 to 2784 in daf9676
Although not relevant for this specific PR, I wanted to raise it here for future reference.
gijswijs
reviewed
Jun 24, 2024
gijswijs
reviewed
Jun 24, 2024
guggero
force-pushed
the
duplicate-asset-insert
branch
from
dd8ba78 to
16ae4dd
Compare
gijswijs
force-pushed
the
postgres-clobber
branch
from
1083cf1 to
84c739d
Compare
guggero
force-pushed
the
duplicate-asset-insert
branch
from
16ae4dd to
42a9d57
Compare
|
lgtm 🎉 |
gijswijs
approved these changes
Jun 24, 2024
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.
To make sure we don't insert duplicate assets, we turn all queries that insert into the assets table into upserts.
The test that discovered the duplicate asset issue in the first place would've shown the issue more clearly if we actually asserted the number of asset UTXOs created. We now add this assertion to make sure we only have the expected number of assets in the asset table.
guggero
force-pushed
the
duplicate-asset-insert
branch
from
42a9d57 to
e6713c5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #691.
Depends on #954.
It turns out that in one of our itests (
multi_address), because we do an address self-transfer, we sometimes created duplicate asset entries, if the timing was unfortunate.Basically both the send logic (freighter) as well as the address receive logic (custodian) inserted the same asset (one materializing it from the transfer output, the other from the proof found in the DB).