search

SiaFoundation / walletd

A new Sia wallet
https://sia.tech/software/walletd
MIT License
15 stars 6 forks source link

Remove `chain_indices` table #107

Closed peterjan closed 2 months ago

peterjan commented 2 months ago

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.

n8maninger commented 2 months ago

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

peterjan commented 2 months ago

@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.