Report LevelDB estimate for chainstate size in gettxoutsetinfo

[sipa]

No description provided.

[gmaxwell]

utACK

[ghost]

Why remove bytes_serialized?

[sipa]

Why remove bytes_serialized?

It has no real meaning. It wasn't an accurate representation of how much space on disk was used, and its definition is very hard to maintain after #10195.

[laanwj]

Nice idea to add the leveldb size estimate - I had no idea such a thing existed.

I'm not that happy that bytes_serialized is a going away.

It provided a fairly neutral way of measuring the UTXO set size (It is not dependent on a specific database implementation and its overhead), I understand leveldb's estimate is better in estimating the actual disk usage, but bytes_serialized is useful for comparison against older versions of the same value for statistics and such.

It has no real meaning. It wasn't an accurate representation of how much space on disk was used, and its definition is very hard to maintain after #10195.

Isn't it a necessary by-product of computing hash_serialized?
Edit: ah so #10195 changes the meaning of that and renames it, I guess we could do bytes_serialized_2 instead, to signify that there is a break with the past format?

[sipa]

The problem is mostly that after #10195, we start relying heavily on LevelDB's prefix compression (whenever two subsequent keys share a prefix, that repeated part is not encoded). This is needed because multiple outputs from the same txid otherwise will require an extra 32 bytes on disk for every output after the first.

It is possible to implement a compatibility layer, which processes the outputs in pertxout, re-assembles them into per-tx things, and uses that to compute bytes_serialized and hash_serialized (that's what #10195 currently does, however, it cannot remain exactly the same as now, as the txversion information is dropped from the db).

It would be much simpler if no such compatibility hack was needed, and bytes_serialized is either dropped entirely (this PR), or replaced with something that has hardly any relation with the disk size anymore. Especially if we ever want to be able to have gettxoutsetinfo not iterate through the whole set anymore...

[sipa]

Updated to no longer remove hash_serialized. We can discuss its removal or semantics change independently.

[paveljanik]

Compiling this brings a lot of new warnings:

+In file included from test/test_bitcoin_fuzzy.cpp:14:
+./coins.h:342:10: warning: 'GetCoins' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
+    bool GetCoins(const uint256 &txid, CCoins &coins) const;
+         ^
+./coins.h:310:18: note: overridden virtual function is here
+    virtual bool GetCoins(const uint256 &txid, CCoins &coins) const;
+                 ^
+./coins.h:343:10: warning: 'HaveCoins' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
+    bool HaveCoins(const uint256 &txid) const;
+         ^
+./coins.h:314:18: note: overridden virtual function is here
+    virtual bool HaveCoins(const uint256 &txid) const;
+                 ^
+./coins.h:344:13: warning: 'GetBestBlock' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
+    uint256 GetBestBlock() const;
+            ^
+./coins.h:317:21: note: overridden virtual function is here
+    virtual uint256 GetBestBlock() const;
+                    ^
+./coins.h:346:10: warning: 'BatchWrite' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
+    bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
+         ^
+./coins.h:321:18: note: overridden virtual function is here
+    virtual bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
+                 ^
+./coins.h:347:23: warning: 'Cursor' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
+    CCoinsViewCursor *Cursor() const;
+                      ^
+./coins.h:324:31: note: overridden virtual function is here
+    virtual CCoinsViewCursor *Cursor() const;
+                              ^
+5 warnings generated.

[sipa]

@paveljanik Those warnings seem unrelated. #10195 adds the necessary overrides as most of the relevant function signatures and changed there anyway, but I don't think that should happen in this PR.

[laanwj]

The problem is that if you use override in one place in a class, the compiler (at least gcc?) wants you to use it everywhere where it's appropriate. Might be better to leave it out then add it for the whole class later. Warnings in .h files are kind of annoying as they get emitted for every single compilation unit that includes them.

utACK otherwise

[sipa]

Rebased, and removed the override for now.

[laanwj]

Seems you forgot one, I still get warnings for override in this class

[sipa]

Fixed.

[sipa]

Closing because merged as part of #10195.