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

blockchain: Remove incorrect upgrade version check. #3010

Merged
merged 1 commit into from
Oct 16, 2022

Conversation

JoeGruffins
Copy link
Member

@JoeGruffins JoeGruffins commented Oct 12, 2022

If utxo version is 2, ignore the utxo db version and upgrade. This
ensures that old databases have the utxo data moved. Also move the utxo
upgrade which uses the new version below so that it will find the new
bucket.

@JoeGruffins
Copy link
Member Author

JoeGruffins commented Oct 12, 2022

I'm not sure if this is correct. If there is a db but no dbinfo it`s correct, but if the db is completely new, then you want the current version.

It should do based on blockDBVersion maybe?

@chappjc
Copy link
Member

chappjc commented Oct 12, 2022

What comes of a DB that has run the upgrades already without this diff? Busted?

@JoeGruffins
Copy link
Member Author

JoeGruffins commented Oct 13, 2022

I think I am wrong about when the version info was added. It's actually 9 99259dc

What comes of a DB that has run the upgrades already without this diff? Busted?

The utxo db would be version three (or already two) without having run the update from 1 -> 2 -> 3.

For some reason, the 1 -> 2 update not happening doesn't seem to break us, but the 2 -> 3 does.

Our blockDBVersion in dcrdex looks to be version 7.

The affected db is broken unless the update is run apparently. If the 1 -> 2 was missed I'm not sure what the implications are. This would be hard to diagnose for the user afaict. It looks like v1.7 has a currentUtxoDatabaseVersion of 2. So, a user with a blockDBVersion earlier than 9 would skip the 1 > 2 as well I guess.

@JoeGruffins JoeGruffins force-pushed the setinitialversions branch 2 times, most recently from 3cfa737 to 218d31f Compare October 13, 2022 03:03
@JoeGruffins
Copy link
Member Author

I think the exact problem is that the old db will have all utxo in []byte("utxosetv2") so migrateUtxoSetVersion2To3 must happen or the utxo are never moved and thus never found. migrateUtxoSetVersion2To3 is inside upgradeUtxoSetToVersion3 which is locked behind the check if utxoDbInfo.version == 2 && utxoDbInfo.utxoVer == 2 { When currentUtxoDatabaseVersion was only 2 this still happened but stopped after d1b381d set it to 3.

@JoeGruffins
Copy link
Member Author

So, I think the answer is simpler than this. We just require utxoDbInfo.utxoVer == 2. Is there a reason utxoDbInfo.version == 2 matters?

@JoeGruffins
Copy link
Member Author

Is it correct that upgradeUtxoSetToVersion3 comes after upgradeUtxoDbToVersion2? It looks like upgradeUtxoDbToVersion2 uses the new bucket []byte("utxosetv3") that is created later.

@JoeGruffins
Copy link
Member Author

Unrelated, but I see this message in logs a lot 2022-10-13 16:43:12.160 [DBG] PEER: Misbehaving whitelisted peer 127.0.0.1:49728 (inbound): getdata

If utxo version is 2, ignore the utxo db version and upgrade. This
ensures that old databases have the utxo data moved. Also move the utxo
upgrade which uses the new version below so that it will find the new
bucket.
@JoeGruffins JoeGruffins changed the title utxocache: Set new backend info with version one. blockchain/upgrade: Remove version check. Oct 13, 2022
@davecgh
Copy link
Member

davecgh commented Oct 13, 2022

I think the exact problem is that the old db will have all utxo in []byte("utxosetv2") so migrateUtxoSetVersion2To3 must happen or the utxo are never moved and thus never found. migrateUtxoSetVersion2To3 is inside upgradeUtxoSetToVersion3 which is locked behind the check if utxoDbInfo.version == 2 && utxoDbInfo.utxoVer == 2 { When currentUtxoDatabaseVersion was only 2 this still happened but stopped after d1b381d set it to 3.

Yep, that is what I found last night as well when I was pointing out the commits in question. Hence why I said the culprit was introduced in 7780ad9 and then was manifest in d1b381d.

So, I think the answer is simpler than this. We just require utxoDbInfo.utxoVer == 2. Is there a reason utxoDbInfo.version == 2 matters?

Is it correct that upgradeUtxoSetToVersion3 comes after upgradeUtxoDbToVersion2? It looks like upgradeUtxoDbToVersion2 uses the new bucket []byte("utxosetv3") that is created later.

Yes, I dug through history and taking everything into consideration, all that needs to be done is move the upgradeUtxoSetToVersion3 code to happen before the version 2 utxo database upgrade and only check if utxoDbInfo.utxoVer == 2. That should resolve all of the cases.

For reference, what happened here is that originally the version 2 utxoset was put into the separate utxo database and upgraded to version 3 there, but in order to resolve some other issues (data that was needed for the spend journal), it was reversed such that the version 2 utxoset is upgraded to version 3 in the main database and then moved to the separate utxo database. In other words, because the version 2 to 3 utxoset upgrade is happening in the main database, the utxo database version is irrelevant and that is why checking it as part of the upgrade results in incorrect logic.

@davecgh
Copy link
Member

davecgh commented Oct 13, 2022

Unrelated, but I see this message in logs a lot 2022-10-13 16:43:12.160 [DBG] PEER: Misbehaving whitelisted peer 127.0.0.1:49728 (inbound): getdata

This is known and isn't really a big issue. It is something that would be nice to address, but it doesn't really affect anything important.

@davecgh davecgh changed the title blockchain/upgrade: Remove version check. blockchain: Remove incorrect upgrade version check. Oct 13, 2022
@davecgh davecgh added this to the 1.8.0 milestone Oct 13, 2022
@davecgh davecgh added the bug label Oct 13, 2022
@davecgh davecgh merged commit 2815615 into decred:master Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants