[Wallet] Improve ReorderTransactions(..) #4697

ReorderTransactions is never called, unless someone would load a very old wallet. I couldnt even trigger ReorderTransactions by experimenting with zapwallettxs. And if it gets triggered somehow, it causes problems, see #3894. The function doesnt even write back txs to disk, if they have nOrderPos == -1, looks like a bug to me.

The worst thing that could happen, is that some txs/entries have nOrderPos = -1. But I dont see any problems with this, even if happen. At least those old transactions would always appear behind new ones.

Not sure about this. For wallets it's extremely important to keep compatibility, even with the oldest possible ones. But if you say that multiple transactions having the same orderpos (or an orderpos<0) doesn't give problems then it may not be so important to keep ReorderTransactions around, especially if the current implementation makes it even more broken.

This worked when I wrote it. Maybe there's a bug introduced by the zap wallet stuff, but that shouldn't be "fixed" by breaking compatibility with old wallets.

I will do some more testing and report back.

In principle ReorderTransactions is a quite simple function. Not some Satoshi monster code that we have to get rid of asap. So my preference would go to fixing it as well.

Updated the title from "Remove" to "Improve" now.

I think we should write all changes to nOrderPos back to the database. Currently txs and nOrderPosNext are not written back, only in-memory. This means that ReorderTransactions(..) is run everytime you start the wallet and have -1 ids.

If you only had AccEntrys with -1, this could result in a bug, because they are written back with higher ids, but nOrderPosNext is not written back. If you restart now, you would start with the lower nOrderPosNext from before. Though this bug is not really a real world scenario, because someone would have txs with -1 as well. @luke-jr Could you take a look at the code changes?

I think that looks safe, but it's been a while... Writing nOrderPosNext should be sufficient to fix it, no?

Yes,

but if we dont write the txs back, they have nOrderPos=-1 again, next time you start the wallet. Unless you trigger a wtx.WriteToDisk() somehow else, through rescan for example.

This means that ReorderTransactions(..) is called everytime you load the wallet. But it should be a one time thing.

I am fine with closing this pull request too, if you prefer that, as this code is 2 years old now.

@cozz It's okay to redo it every time: the results should be the same. :)

But writing it back should be harmless too.

Closing in that case, thanks for helping here anyway.

I don't think it makes sense to do it every time. Fixing an old wallet should be a one-time thing - after that it should behave like a new wallet.

Reopening in that case.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4697_da2ede2aa68ba14e1228b61e41a5840669560eee/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

ACK