wallet_db: put 'genesis_blockhash' in DB, detect mainnet/testnet mixup. (db upgrade)#10592
Conversation
If the user tries to open a wallet for a different chain (mainnet vs testnet), try to show a reasonable error message. See previous attempt at this: spesmilo@c13e057, which added `wallet.test_addresses_sanity()`. However there are many codepaths where "random" exceptions might get raised before the Wallet object is even instantiated. See [discussion there](spesmilo@c13e057#commitcomment-28017341): > should we actually fix that? > if yes, it would be better to write the network type in storage Indeed now I think we should do that. At the time I was concerned it would not help against altcoin forks if we put "mainnet" or "testnet" in the DB. Now I realise we should just put the genesis block hash in the DB instead. Many of the reports in spesmilo#6526 are likely due to users trying to open a mainnet wallet in testnet mode or vice-versa. fixes spesmilo#9134 same issue in wizard 2fa two-step wallet-creation flow
| self.up_to_date_changed_event = asyncio.Event() | ||
|
|
||
| self.test_addresses_sanity() | ||
| assert self.db.get('genesis_blockhash') == constants.net.GENESIS, self.db.get('genesis_blockhash') |
There was a problem hiding this comment.
I noticed this AssertionError gets caught in ElectrumGui.start_new_window() and shown as warning dialog instead of being forwarded to the crash reporter.
We should probably modify start_new_window() for the regular wallet loading case similar to the change done here for wizard exceptions: 457a092
Edit: opened a PR for this: #10605
There was a problem hiding this comment.
Hmm, I did not think it possible to trigger this assert -- in theory the same check in WalletDB.upgrade_wallet_db should raise before we get here.
Is that not the case?
There was a problem hiding this comment.
Yeah i was just manually modifying the db for testing purposes. I just think that in general it would make sense to send unexpected exceptions coming from load_wallet to the crash reporter (though this is not directly related to this PR)
|
Tested/reviewed 68e6995, works as intended. |
2 participants
If the user tries to open a wallet for a different chain (mainnet vs testnet), try to show a reasonable error message.
See previous attempt at this: c13e057, which added
wallet.test_addresses_sanity(). However there are many codepaths where "random" exceptions might get raised before the Wallet object is even instantiated.See discussion there:
Indeed now I think we should do that. At the time I was concerned it would not help against altcoin forks if we put "mainnet" or "testnet" in the DB. Now I realise we should just put the genesis block hash in the DB instead.