[master] ZIL-5403: check if a LevelDB object is null or not

[yaron-zilliqa]

Description

Backward Compatibility

• This is not a breaking change
• This is a breaking change

Review Suggestion

Status

Implementation

• ready for review

Integration Test (Core Team)

• local machine test

• small-scale cloud test

[rrw-zilliqa]

Check comments, but I don't think this makes things worse .. worth testing quite thoroughly, though, because bugs here might be quite bad.

[rrw-zilliqa]

This has the effect that if the db isn't available, we assume that it's empty. Under what circumstances might it not be available? I'm thinking of the case where the user induces the contract state db not to be available whilst calling a transition that checks a flag - the db is not there, the flag checks out as no, and the user is then allowed to do something they shouldn't ... ?

[yaron-zilliqa]

The design of this class is pretty bad from the start and indeed like you mentioned, it treats certain errors as if something is empty or not found instead of handling things correctly from the outset.
These changes are backward-compatible with this as I'm not convinced we can fix this completely and the change would be much larger.
Some databases are only created/read in lookup mode, for example. Another possibility is that something wasn't downloaded correctly or got corrupted.
The alternative is to let the process crash completely (which it would have done so far).. and also start downloading persistence upon the next restart, and then possibly crashing again, and so on...

[rrw-zilliqa]

You're not wrong .. OK. Let's go with this on the basis that if at least 1/3 of the nodes have the right database, you won't be able to commit your txn? @JamesHinshelwood @DrZoltanFazekas - am I off base?

[JamesHinshelwood]

@yaron-zilliqa Sorry, I'm not understanding this properly, but the new code does jump out to me as a bit scary. I agree that its dangerous to assume 'DB doesn't exist' is the same as 'DB exists, but is empty'.

Can you explain what the initial bug you were fixing was? A description of how you believe this changes the logic would also be useful. Sorry to be a pain, but because the methods in the class have been re-ordered as well as edited, it makes the diff quite difficult to understand.

[yaron-zilliqa]

The methods haven't been reordered. The only change in logic here is checking for a nullptr before accessing it.
The previous methods were really a copy of one another with a different construction of leveldb::Slice, so the not simply invoke XXXXXImpl(...) instead of repeating the code.
The problem it solves is that the process won't crash (and then redownload persistence upon restart) if the required LevelDB object was NULL. I came across this scenario this time round when setting ARCHIVAL_LOOKUP_WITH_TX_TRACES to true and calling ots_getTransactionError.

[JamesHinshelwood]

I'm not sure this makes sense. Would s.IsNotFound() be true if the DB exists, but the key (arg) doesn't exist in it?

[yaron-zilliqa]

It would; this is backward compatible with the current implementation. Haven't changed anything there. This piece of code simply replaced log_error which I've removed and placed here.

Discussed with @yaron-zilliqa - best not to merge this now; requesting changes so it doesn't end up in master before v9.3

[rrw-zilliqa]

Leaving this as request changes to make sure it doesn't get merged prematurely.