Sanitize some wallet serialization #12658

1. 
   sipa commented at 1:32 am on March 10, 2018: member

   This is a small subset of changes taken from #10785, fixing a few of the craziest constness violations in the serialization code.

   CWalletTx currently serializes some of its fields by embedding them in a key-value mapValue, which is modified (and then fixed up) even from the Serialize method (for which mapValue is const). CAccountingEntry goes even further in that it stores such a map by appending it into strComment after a null char, which is again later fixed up again.

   Fix this by splitting the serialization and deserialization code, and making the serialization act on a copy of mapValue / strComment.

2. Split up and sanitize CWalletTx serialization 029ecac1bc
3. fanquake added the label Wallet on Mar 10, 2018
4. sipa requested review from ryanofsky on Mar 10, 2018
5. sipa renamed this:
   Sanitize some wallet serialize
   Sanitize some wallet serialization
   on Mar 10, 2018
6. in src/wallet/wallet.h:420 in 6ed3ab8c90 outdated

   444-        }
   445+        s >> *static_cast<CMerkleTx*>(this);
   446+        std::vector<CMerkleTx> vUnused; //!< Used to be vtxPrev
   447+        s >> vUnused >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> fSpent;
   448+
   449+        strFromAccount = std::move(mapValue["fromaccount"]);
   

   ---

   ---

   

   eklitzke commented at 3:57 pm on March 10, 2018:

   nit: Instead of move here and erase a few lines below, you can do the search one and localize the logic:

   0auto it = mapValue.find("fromaccount");
   1assert(it != mapValue.end());  // optional, if you feel like it
   2strFromAccount = std::move(it->second);
   3mapValue.erase(it);
   

   One day in the glorious future when we’re all using C++17 you can use .extract().

   ---

   

   sipa commented at 5:50 pm on March 11, 2018:

   As this code isn’t particularly performance critical, I prefer the current shorter code.

   ---

   

   eklitzke commented at 6:06 am on March 13, 2018:

   Makes sense.
7. in src/wallet/wallet.h:618 in 6ed3ab8c90 outdated

   635+
   636+        mapValue_t mapValueCopy = mapValue;
   637+        WriteOrderPos(nOrderPos, mapValueCopy);
   638+
   639+        std::string strCommentCopy = strComment;
   640+        if (!(mapValueCopy.empty() && _ssExtra.empty())) {
   

   ---

   ---

   

   eklitzke commented at 4:03 pm on March 10, 2018:

   nit: consider making this:

   0if (!mapValueCopy.empty() || !_ssExtra.empty()) {
   

   ---

   

   sipa commented at 5:57 pm on March 11, 2018:

   Done.
8. 
   eklitzke commented at 4:06 pm on March 10, 2018: contributor

   I’m not an expert in this part of the code, but this change looks good, and I’m happy to see so many red lines in the diff.
9. Split up and sanitize CAccountingEntry serialization 42343c748c
10. sipa force-pushed on Mar 11, 2018
11. 
    promag commented at 2:15 pm on March 13, 2018: member

    utACK 42343c7 LGTM.
12. in src/wallet/wallet.h:621 in 42343c748c

    615+            s << nVersion;
    616+        }
    617         //! Note: strAccount is serialized as part of the key, not here.
    618-        READWRITE(nCreditDebit);
    619-        READWRITE(nTime);
    620-        READWRITE(LIMITED_STRING(strOtherAccount, 65536));
    

    ---

    ---

    

    ryanofsky commented at 2:52 pm on March 13, 2018:

    Not being changed by this PR, but seems bad to me that length is not checked on serialization (only on deserialization).

    ---

    

    laanwj commented at 5:38 pm on March 13, 2018:

    Although you could add a sanity check, serialization is probably too late to check it. Ideally anything inputting account/label names should check this to be able to give an useful error and reject setting it before putting it in a data structure. But yes, out of scope here.
13. 
    ryanofsky commented at 3:06 pm on March 13, 2018: member

    utACK 42343c748c2bca8ba9b888d949088dbfd1f142b4. Nice to make #10785 a little simpler.
14. 
    laanwj commented at 5:39 pm on March 13, 2018: member

    utACK 42343c748c2bca8ba9b888d949088dbfd1f142b4
15. laanwj merged this on Mar 13, 2018
16. laanwj closed this on Mar 13, 2018

    ---

17. laanwj referenced this in commit af88094e4f on Mar 13, 2018
18. PastaPastaPasta referenced this in commit 51a0d0bb09 on Jun 9, 2020
19. PastaPastaPasta referenced this in commit b33a9b94e2 on Jun 9, 2020
20. PastaPastaPasta referenced this in commit 85e1502e9d on Jun 10, 2020
21. PastaPastaPasta referenced this in commit 4faeba9963 on Jun 11, 2020
22. PastaPastaPasta referenced this in commit 98ee5d6161 on Jun 11, 2020
23. PastaPastaPasta referenced this in commit 0b5142bef8 on Jun 11, 2020
24. PastaPastaPasta referenced this in commit 1634d58421 on Jun 12, 2020
25. MarcoFalke locked this on Sep 8, 2021

Contributors
sipa eklitzke promag ryanofsky laanwj

Labels
Wallet