consensus lib work: Reduce boost dependencies #5082

theuni commented at 7:34 PM on October 13, 2014: member

These are the functional changes needed to remove boost as a dependency from the upcoming consensus lib.

After these changes, a good bit of code movement is still needed to actually remove the dependencies, but it's little more than copy/paste. Those will come as a 2nd PR after this one to ease review.

Please pay special attention to the new serializers in walletdb.cpp. I believe I've ported that correctly, but I'd like to have lots of eyes on it.

in src/walletdb.cpp:None in 222ef26d26 outdated

  34 | +
  35 | +    ADD_SERIALIZE_METHODS;
  36 | +
  37 | +    template <typename Stream, typename Operation>
  38 | +    inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
  39 | +        READWRITE(std::string("acentry"));

sipa commented at 7:49 PM on October 13, 2014:

I don't really like data structures that know their own key in a database, as it breaks composability. Can you leave the "acentry" and "destdata" out, and leave those in the actual db write methods using a make_pair("acentry", blah)?

in src/core.cpp:None in 222ef26d26 outdated

 259 | @@ -263,8 +260,8 @@ uint256 CBlock::BuildMerkleTree(bool* fMutated) const
 260 |      */
 261 |      vMerkleTree.clear();
 262 |      vMerkleTree.reserve(vtx.size() * 2 + 16); // Safe upper bound for the number of total nodes.
 263 | -    BOOST_FOREACH(const CTransaction& tx, vtx)
 264 | -        vMerkleTree.push_back(tx.GetHash());
 265 | +    for(std::vector<CTransaction>::const_iterator it(vtx.begin()); it != vtx.end(); ++it)

sipa commented at 7:49 PM on October 13, 2014:

Coding style nit: can you put a space after the 'for'?

in src/key.cpp:None in 222ef26d26 outdated

 433 | @@ -436,16 +434,21 @@ bool CKey::SetPrivKey(const CPrivKey &privkey, bool fCompressedIn) {
 434 |  CPrivKey CKey::GetPrivKey() const {
 435 |      assert(fValid);
 436 |      CPrivKey privkey;
 437 | +    int privkeylen,ret;

sipa commented at 7:50 PM on October 13, 2014:

Coding style nit: can you put a space after the comma?

in src/walletdb.cpp:None in 222ef26d26 outdated

 255 | @@ -218,7 +256,7 @@ void CWalletDB::ListAccountCreditDebit(const string& strAccount, list<CAccountin
 256 |          // Read next record
 257 |          CDataStream ssKey(SER_DISK, CLIENT_VERSION);
 258 |          if (fFlags == DB_SET_RANGE)
 259 | -            ssKey << boost::make_tuple(string("acentry"), (fAllAccounts? string("") : strAccount), uint64_t(0));
 260 | +            ssKey << CAccEntryMeta((fAllAccounts? string("") : strAccount), uint64_t(0));

sipa commented at 7:51 PM on October 13, 2014:

Coding style nit: can you put a space before the question mark?

sipa commented at 7:51 PM on October 13, 2014: member

Concept ACK, and most code changes look good, apart from the nits above.

theuni commented at 3:40 AM on October 14, 2014: member

Nits addressed.

in src/walletdb.cpp:None in 293dfb8e28 outdated

  22 | @@ -23,6 +23,46 @@ using namespace std;
  23 |  
  24 |  static uint64_t nAccountingEntryNumber = 0;
  25 |  
  26 | +namespace {
  27 | +
  28 | +class CAccEntryMeta
  29 | +{
  30 | +    const std::string m_str;

TheBlueMatt commented at 6:24 AM on October 14, 2014:

Is this our coding style? m_?

theuni commented at 6:28 AM on October 14, 2014:

Old habit for member vars (a good habit for once, imo). I haven't seen any real continuity in the codebase. What would you prefer?

TheBlueMatt commented at 7:30 AM on October 14, 2014:

A long, long time ago In a galaxy far, far away... ^H^H^H^H^H^H^Hit was always typeVarName, but anything that isnt introducing another conflicting style is good with me (generally matching what is in surrounding code is best).

laanwj commented at 11:57 AM on October 14, 2014:

I agree that m_ for members is a fairly good practice, but this (some obscure wallet structure) isn't the place to get it started.

Edit: also we always use explicit public: / private:, and usually start the class with public: members/methods.

laanwj commented at 2:28 PM on October 14, 2014:

But for something slightly more interesting than style: CAccEntryMeta and CDestDataMeta are created, serialized, then deleted again. They are never deserialized, and the fields are never read. Do you intend to use these on the read-side of things as well, or are you saving that for a later pull?

sipa commented at 6:23 PM on October 14, 2014:

They're being deserialized field by field in the ReadKeyValue function.

TheBlueMatt commented at 7:34 AM on October 14, 2014: member

Mostly looks good...while you're changing serialize.h, can you add comments, as the changes now make it even less clear what is going on in that mess.

sipa commented at 6:25 PM on October 14, 2014: member

@theuni passing the name of the field to the classes is not what I meant; this still means they're not composable.

What I meant is: Write(make_pair(std::string("destdata"), CDestData(address, key)), value);

However, if you see it like that, there is not even a need for a separate class (though it may help for readability) - you could just as wel use:

Write(make_pair(std::string("destdata"), make_pair(address, key)), value);

theuni commented at 8:03 PM on October 14, 2014: member

nits addressed, I believe. I used @sipa's idea to use a pair of pairs for the serialization rather than the custom classes. That seems more clear, and matches one of the uses that was already there as well.

I'll squash if all looks good now.

sipa commented at 10:29 PM on October 14, 2014: member

ut ACK

TheBlueMatt commented at 5:15 AM on October 15, 2014: member

utACK

laanwj commented at 7:19 AM on October 15, 2014: member

Yes, not introducing those classes at all is better. utACK

theuni force-pushed on Oct 15, 2014

theuni commented at 7:13 PM on October 15, 2014: member

squashed and rebased.

boost: drop dependency on is_fundamental in serialization

There's only one case where a vector containing a fundamental type is
serialized all-at-once, unsigned char. Anything else would lead to
strange results.

Use a dummy argument to overload in that case.

1d9b86d584

boost: drop dependency on tuple in serialization

There's only one user of this form of serialization, so it can be easily
dropped. It could be re-added if desired when we switch to c++11.

52955068b7

boost: remove CPrivKey dependency from CECKey

This allows CECKey to be used without directly depending on the secure
allocators

e405aa48c7

boost: drop boost dependency in core.cpp e1c9467766

boost: drop boost dependency in utilstrencodings.cpp 352058e8b0

boost: drop boost dependency in version.cpp.

Also add a test to verify.

5f4bcf6b14

sipa merged this on Oct 15, 2014

sipa closed this on Oct 15, 2014

sipa referenced this in commit e8f6d54f1f on Oct 15, 2014

DrahtBot locked this on Sep 8, 2021