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

chore: add unique constraint on vm_messages.params #1107

Closed
wants to merge 1 commit into from

Conversation

kasteph
Copy link
Contributor

@kasteph kasteph commented Jan 3, 2023

Stacks on: #1105 (please review #1105 first before this one)
Resolves: #1106

To prevent duplicate vm_messages, we can add a constraint on the params.

@kasteph kasteph requested a review from frrist January 3, 2023 04:15
@placer14
Copy link
Contributor

placer14 commented Jan 3, 2023

Two thoughts:

Functionally: Could this constraint be reduced to fewer columns? The likelihood of a (state_root, cid) collision is practically non-existent (since the hash for the cid depends on the state_root hash). With this in hand, could we safely remove the source from the constraint to make insertion a little cheaper?

Overall: I'm also not sure how these duplicates are being introduced? Do the duplicate internal messages (with the same params) actually occur at the same (height, state_root, cid, source) combinations? (The issue is vague on the cause of this problem.) Do we know why this is? Are they important to capture?

The best I can think is that the VM runs certain state changes over and over by virtue of the source message being included in multiple blocks within a single tipset. If this is the case, then the param unique constraint doesn't add anything as the CID would match (because params change the message CID, correct?).

I think a clearer explanation of how this fixes the problem would be helpful.

@kasteph
Copy link
Contributor Author

kasteph commented Jan 6, 2023

Functionally: Could this constraint be reduced to fewer columns? The likelihood of a (state_root, cid) collision is practically non-existent (since the hash for the cid depends on the state_root hash). With this in hand, could we safely remove the source from the constraint to make insertion a little cheaper?

That sounds good.

Overall: I'm also not sure how these duplicates are being introduced? Do the duplicate internal messages (with the same params) actually occur at the same (height, state_root, cid, source) combinations? (The issue is vague on the cause of this problem.) Do we know why this is? Are they important to capture?

Forrest commented on Slack:

Duplicates here are again an artifact of the table schema. There is no requirement that these messages are unique since they do not appear on chain. It is likely that case that these parameters are different for each of the duplicate messages.

@frrist
Copy link
Member

frrist commented Jan 9, 2023

We need to hold on this change until more details are added to #1106. I don't have a clear picture of what problem we are trying to fix here - I actually don't see a problem.

@frrist
Copy link
Member

frrist commented Feb 16, 2023

closing in favor of #1127

@frrist frrist closed this Feb 16, 2023
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.

3 participants