Sanitize some wallet serialization#12658
Conversation
eklitzke
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
nit: Instead of move here and erase a few lines below, you can do the search one and localize the logic:
auto it = mapValue.find("fromaccount");
assert(it != mapValue.end()); // optional, if you feel like it
strFromAccount = std::move(it->second);
mapValue.erase(it);One day in the glorious future when we're all using C++17 you can use .extract().
There was a problem hiding this comment.
As this code isn't particularly performance critical, I prefer the current shorter code.
There was a problem hiding this comment.
nit: consider making this:
if (!mapValueCopy.empty() || !_ssExtra.empty()) {6ed3ab8 to
42343c7
Compare
|
utACK 42343c7 LGTM. |
| //! Note: strAccount is serialized as part of the key, not here. | ||
| READWRITE(nCreditDebit); | ||
| READWRITE(nTime); | ||
| READWRITE(LIMITED_STRING(strOtherAccount, 65536)); |
There was a problem hiding this comment.
Not being changed by this PR, but seems bad to me that length is not checked on serialization (only on deserialization).
There was a problem hiding this comment.
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.
|
utACK 42343c7 |
42343c7 Split up and sanitize CAccountingEntry serialization (Pieter Wuille) 029ecac Split up and sanitize CWalletTx serialization (Pieter Wuille) Pull request description: 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`. Tree-SHA512: 487e04996dea6aba5b9b8bdaf2c4e680808f111a15afc557b8d078e14b01e4f40f8ef27588869be62f9a87052117c17e0a0c26c59150f83472a9076936af035e
42343c7 Split up and sanitize CAccountingEntry serialization (Pieter Wuille) 029ecac Split up and sanitize CWalletTx serialization (Pieter Wuille) Pull request description: This is a small subset of changes taken from bitcoin#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`. Tree-SHA512: 487e04996dea6aba5b9b8bdaf2c4e680808f111a15afc557b8d078e14b01e4f40f8ef27588869be62f9a87052117c17e0a0c26c59150f83472a9076936af035e
This is a small subset of changes taken from #10785, fixing a few of the craziest constness violations in the serialization code.
CWalletTxcurrently serializes some of its fields by embedding them in a key-valuemapValue, which is modified (and then fixed up) even from theSerializemethod (for whichmapValueis const).CAccountingEntrygoes even further in that it stores such a map by appending it intostrCommentafter 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.