move-only: move coins statistics utils out of RPC #16728

please keep this list sorted, as before

This includes serialize.h, which takes quite a long time to parse. Why not replace those two includes with a simple forward decl?

+#include <amount.h>
+class CBlockIndex;
+class CCoinsView;

what is this?

In file included from rpc/blockchain.cpp:33:
./validation.h:406:14: warning: redundant redeclaration of ‘CBlockIndex* LookupBlockIndex(const uint256&)’ in same scope [-Wredundant-decls]
  406 | CBlockIndex* LookupBlockIndex(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
      |              ^~~~~~~~~~~~~~~~
In file included from rpc/blockchain.cpp:13:
./coinstats.h:18:14: note: previous declaration of ‘CBlockIndex* LookupBlockIndex(const uint256&)’
   18 | CBlockIndex* LookupBlockIndex(const uint256& blockhash);
      |              ^~~~~~~~~~~~~~~~

In commit "move-only: move coins statistics utils out of RPC" (07da7c422d875e54176a81b37de011a4761966cc)

Note: previous line was

stats.nHeight = LookupBlockIndex(stats.hashBlock)->nHeight;

which crashes if LookupBlockIndex returns null. Otherwise there's no change.

This should never crash, unless there is a data corruption. I'd prefer to keep the previous code or throw an exception instead of withholding the fact that data is corrupted from the user.

Alternatively, you could use error(), as done later on in this function. I don't think the current code makes sense, unless you can give an explanation or code comment to motivate it

Ah my mistake, I don't actually remember changing this code (it's been a while since I wrote the original patch). I'll revert to current behavior to keep move-onlyness.

In commit "move-only: move coins statistics utils out of RPC" (07da7c422d875e54176a81b37de011a4761966cc)

Could consider moving moving these files to src/node/. IMO better to make use of node/, wallet/, qt/, util/, etc subdirectories instead of adding things at the top level. Some notes about this in #15732.

Just to be clear on the rationale here: LookupBlockIndex accesses a global that is only properly initialized in the node process. Thus, the coinstats module should be in the src/node folder?

Just to be clear on the rationale here

Calling LookupBlockIndex and accessing globals is a more of a rationale for including this code in the libbitcoin_server.a library instead of libbitcoin_util.a or another library.

There is libbitcoin_server.a code in the top-level directory, but there is also a code from other libraries there. I just think it's nice to use node/, wallet/, qt/, util/, consensus/ subdirectories to organize code so you can know what context the code functions in and what library it will be a part of.

more of a rationale for including this code in the libbitcoin_server.a library

I think we agree then. src/node should eventually be the folder for libbitcoin_server.

@ryanofsky thanks for the pointer, forgot about these. Will move.

Why are you removing the lock annotation? Can't you just include the header?

validation.cpp will eventually include this file (for use validationg snapshots) and I didn't want to create a circular dep. My mistake, forgot this would remove the lock annotation for uses in this file, will add back.

This would be the first time that we solved a circular dependency by copy-pasting parts of a header file.

validation.cpp will eventually include this file

If, and when, this happens, you can either document the circular dependency or remove the include via this hack. No need to do it now.

Could be an opportunity to comment fields structures like bytes unit of nDiskSize and bitcoin unit of nTotalAmount, why hashSerialized is hash_serilalized_2, what's the purpose of nBogoSize

If you are going to push a separate commit then you could also drop the constructor and initialize members in place, like int nHeight{0};.

Good suggestions. Will add if I have to invalidate the move-onlyness for some reason.

This change deserves its own commit? Assertion works for me too as @MarcoFalke suggested: #16728 (review)

Agree with the separate commit.

Could fix format in this new line (space before arg name).

What is this for?

Good point - stale.

Should #include <cstdint>.

nit sort :trollface:

Can be done the next time this file is touched.