core/rawdb: Enforce readonly in freezer instantiation

[s1na]

When opening the freezer in readonly mode (e.g. db inspect command) we still perform repair, possibly modifying the underlying files. This PR changes that and only raises an error instead when freezer is in an invalid state.

[holiman]

I think you also should take care of the case where someone tries to actually write to the freezer. This can be done in several ways -- all write happens via the batch, afaik, so you could centralize the check there. Perhaps make the singular f.writeBatch be nil if readonly, or set the batch to non-nil but "refusing to write".

[holiman]

Hm, my comment was a bit misplaced. So the freezer already checks the readonly, and the individual table doesn't have any public method to append items (any more).
However, we have the freezerTableBatch, which has an Append method where we could have a corresponding check.

[holiman]

However, we have the freezerTableBatch, which has an Append method where we could have a corresponding check.

This is not implemented, and I added a test to check how it would behave. What happens is that the AppendRaw goes ahead, but at some point, when the commi is called (either explicitly or due to the append causing a new table to be opened), it will try to append to the index file, and find that the file descriptor is bad (probably wrong mode).

I think that's an ok scenario, The caller can write to a batch, but it will eventually fail, and there should be no side-effects, afaict. So I'm good.

[holiman]

LGTM

[rjl493456442]

I think we can merge these two loops into single one, to have less code.

[karalabe]

Other nit, please rename k to something a bit better, lest go with kind without a better idea.

[s1na]

I think we can merge these two loops into single one, to have less code.

I understand that would be shorted in terms of lines, but I find it more readable to have them logically separate. If you don't have a strong feeling about it I'd like to keep it as is.

[s1na]

So I found out db inspect still truncates an inconsistent db. That's because the freezer is opened twice, and the first time it's opened in write-mode via makeConfigNode->setEthConfig->MakeChainDatabase.

[s1na]

After fixing that issue the logs for db inspect on an inconsistent db look like this:

❯ ./build/bin/geth --dev --datadir dev-datadir-legacy db inspect
INFO [01-11|19:57:55.086] Maximum peer count                       ETH=50 LES=0 total=50
INFO [01-11|19:57:55.087] Set global gas cap                       cap=50,000,000
INFO [01-11|19:57:55.151] Using developer account                  address=0xe0AD84EE1bC06DCb400235A3006997e262A3106F
INFO [01-11|19:57:55.151] Allocated cache and file handles         database=~/data/geth/chaindata cache=512.00MiB handles=5120 readonly=true
WARN [01-11|19:57:55.158] Truncating dangling indexes              database=~/data/geth/chaindata/ancient table=receipts indexed=1.19MiB stored=1.19MiB
Fatal: Could not open database: truncate ~/data/geth/chaindata/ancient/receipts.cidx: invalid argument

Not super clean IMHO. I'm tilting slightly towards adding back the extra checks.

[karalabe]

SGTM