Report LevelDB estimate for chainstate size in gettxoutsetinfo

sipa commented at 2:56 am on May 14, 2017: member

gmaxwell approved

gmaxwell commented at 3:17 am on May 14, 2017: contributor

utACK

ghost commented at 2:58 pm on May 14, 2017: none

Why remove bytes_serialized?

sipa commented at 6:27 pm on May 14, 2017: member

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.

fanquake added the label RPC/REST/ZMQ on May 15, 2017

fanquake added the label Resource usage on May 15, 2017

laanwj commented at 5:11 am on May 15, 2017: member

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 commented at 5:41 am on May 15, 2017: member

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 force-pushed on May 15, 2017

sipa commented at 7:23 am on May 15, 2017: member

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

paveljanik commented at 9:30 am on May 15, 2017: contributor

Compiling this brings a lot of new warnings:

 0+In file included from test/test_bitcoin_fuzzy.cpp:14:
 1+./coins.h:342:10: warning: 'GetCoins' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
 2+    bool GetCoins(const uint256 &txid, CCoins &coins) const;
 3+         ^
 4+./coins.h:310:18: note: overridden virtual function is here
 5+    virtual bool GetCoins(const uint256 &txid, CCoins &coins) const;
 6+                 ^
 7+./coins.h:343:10: warning: 'HaveCoins' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
 8+    bool HaveCoins(const uint256 &txid) const;
 9+         ^
10+./coins.h:314:18: note: overridden virtual function is here
11+    virtual bool HaveCoins(const uint256 &txid) const;
12+                 ^
13+./coins.h:344:13: warning: 'GetBestBlock' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
14+    uint256 GetBestBlock() const;
15+            ^
16+./coins.h:317:21: note: overridden virtual function is here
17+    virtual uint256 GetBestBlock() const;
18+                    ^
19+./coins.h:346:10: warning: 'BatchWrite' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
20+    bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
21+         ^
22+./coins.h:321:18: note: overridden virtual function is here
23+    virtual bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
24+                 ^
25+./coins.h:347:23: warning: 'Cursor' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
26+    CCoinsViewCursor *Cursor() const;
27+                      ^
28+./coins.h:324:31: note: overridden virtual function is here
29+    virtual CCoinsViewCursor *Cursor() const;
30+                              ^
31+5 warnings generated.

sipa commented at 10:43 pm on May 15, 2017: member

@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 commented at 10:42 am on May 22, 2017: member

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 force-pushed on May 30, 2017

sipa commented at 7:56 pm on May 30, 2017: member

Rebased, and removed the override for now.

in src/coins.h:188 in 7086ee08ce outdated

344@@ -342,6 +345,7 @@ class CCoinsViewBacked : public CCoinsView
345     void SetBackend(CCoinsView &viewIn);
346     bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
347     CCoinsViewCursor *Cursor() const;
348+    size_t EstimateSize() const override;

laanwj commented at 5:30 pm on June 1, 2017:

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

sipa commented at 10:03 pm on June 1, 2017:

Fixed.

sipa force-pushed on Jun 1, 2017

sipa closed this on Jun 1, 2017

sipa force-pushed on Jun 1, 2017

sipa commented at 11:33 pm on June 1, 2017: member

Closing because merged as part of #10195.

DrahtBot locked this on Sep 8, 2021

Report LevelDB estimate for chainstate size in gettxoutsetinfo #10396