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

Remove chain_indices table #107

Merged
merged 2 commits into from
Apr 24, 2024
Merged

Remove chain_indices table #107

merged 2 commits into from
Apr 24, 2024

Conversation

peterjan
Copy link
Member

@peterjan peterjan commented Apr 23, 2024

This PR removes the chain_indices table. I'm not entirely sure that's what we want but since we were discussing getting rid of the CASCADE deletes I don't see a good reason to keep the table. Creating it and linking it is more of a nuisance than anything else imo, if you don't get the upside of the cascaded deletes.

I hit an interesting test failure in the process, an underflow is triggered in TestEphemeralBalance which is interesting because on master there are no orphaned indices, but now there are orphaned siacoin_elements... so I'm a little bit puzzled still on why that is.

I think there might also be a bug in RevertOrphans because we loop over the orphaned indices and it seems we only deal with the balances of the last orphan. (consensus.go:751) Since I'm not sure whether we want to get rid of the chain_indices entirely I'm going to keep it in DRAFT until tomorrow when Nate is back from being OOO.

@peterjan peterjan requested a review from n8maninger April 23, 2024 11:13
@n8maninger n8maninger marked this pull request as ready for review April 23, 2024 14:17
@n8maninger
Copy link
Member

n8maninger commented Apr 23, 2024

The failure in TestEphemeralBalance is valid because we're associating the unspent siacoin element by reverting the block with the block ID we're reverting -- creating an orphan. We need to remove the chain index association from elements altogether, but then we run into problems removing orphans.

The easiest way to solve this problem is to add a spent BOOLEAN to the Siacoin and Siafund elements tables instead of deleting them. Another option may be to remove the lock while rescanning so that reorgs are processed as they happen. We'd also have to track the rescan progress separately instead of repeatedly overwriting the last scan. Unfortunately, I'm not sure if that completely solves the problem or introduces a giant potential race condition.

Regardless, I'm all for removing the chain_indices table

persist/sqlite/consensus.go Show resolved Hide resolved
persist/sqlite/consensus.go Show resolved Hide resolved
persist/sqlite/consensus.go Outdated Show resolved Hide resolved
persist/sqlite/consensus.go Outdated Show resolved Hide resolved
persist/sqlite/consensus.go Outdated Show resolved Hide resolved
persist/sqlite/consensus.go Outdated Show resolved Hide resolved
persist/sqlite/consensus.go Outdated Show resolved Hide resolved
persist/sqlite/init.sql Outdated Show resolved Hide resolved
persist/sqlite/init.sql Outdated Show resolved Hide resolved
persist/sqlite/init.sql Outdated Show resolved Hide resolved
@peterjan peterjan force-pushed the pj/remove-chain-index branch from 28957fc to 80f1337 Compare April 24, 2024 10:30
@peterjan peterjan self-assigned this Apr 24, 2024
@peterjan peterjan requested a review from n8maninger April 24, 2024 10:32
@peterjan
Copy link
Member Author

@n8maninger this PR now removes the chain_indices table and I believe that the behaviour before and after merging is the same. I think I found a bug though in how we handle reverts and orphans, will open another PR to address that issue, still have one broken test.

@n8maninger n8maninger merged commit beb3587 into master Apr 24, 2024
8 checks passed
@n8maninger n8maninger deleted the pj/remove-chain-index branch April 24, 2024 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants