CMerkleTx is only used as a base class for CWalletTx. It was previously also used for vtxPrev which was removed in 93a18a3650292afbb441a47d1fa1b94aeb0164e3.
This PR moves all of the CMerkleTx members and logic into CWalletTx. The CMerkleTx class is kept for deserialization and serialization of old wallet files.
This makes the refactor in #15931 cleaner.
364@@ -365,82 +365,24 @@ struct COutputEntry
365 int vout;
366 };
367
368-/** A transaction with a merkle branch linking it to the block chain. */
369+/** Legacy class used for deserializing vtxPrev for backwards compatibility **/
411
412 template <typename Stream, typename Operation>
413 inline void SerializationOp(Stream& s, Operation ser_action) {
414+ CTransactionRef tx;
415+ uint256 hashBlock;
416+ int nIndex;
No CMerkleTx object is ever serialized.
I could go even further in this PR and remove the CMerkleTx object entirely. Let me add a commit to see what that looks like.
static_cast<const CMerkleTx&>(*this) that happens in the serialization of CWalletTx
static_cast<const CMerkleTx&>(*this)
Oh, that probably means that serialization is called, so I’d like to re-raise my previous question of uninitialized memory.
That probably means you can get rid of the static_cast<const CMerkleTx&>(*this) that happens in the serialization of CWalletTx
Huh? This PR already does get rid of the static_cast<const CMerkleTx&>(*this)
fd2773f2eba659d78fae9d74461d9dd952609aa9
No CMerkleTx object is ever serialized.
Right, the vector is always empty. You could add assert(ser_action.ForRead()). But the following removes the serialization support so maybe not worth it?
But the following removes the serialization support so maybe not worth it?
I agree (if we decide to keep the second commit)
When I open+close an “old” wallet file, this will overwrite all merkle txs with zeros?
No. When opening the old file, the CWalletTx deserialization logic will deserialize the merkle txs into a locally scoped vUnused which is thrown away. When closing the wallet, CWalletTx::Serialize() serializes an empty vector vUnused into the file. From my reading of serialize.h, that just writes a single zero byte (a CompactSize of 0).
That behaviour should be unchanged by this PR.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
424- std::vector<uint256> vMerkleBranch; // For compatibility with older versions.
425- READWRITE(tx);
426- READWRITE(hashBlock);
427- READWRITE(vMerkleBranch);
428- READWRITE(nIndex);
429+ s << tx << hashBlock << vMerkleBranch << nIndex;
>> ones instead.
Yeah, I am interested in seeing that commit.
I’ve added a second commit that removes the CMerkleTx serialization code (since we never actually have to use it). I couldn’t figure out how to replicate the deserialization code for CMerkleTx in CWalletTx::deserialize() and be sure that I’d replicated it byte-for-byte, so I left that in.
Let me know if you think that second commit is useful. I’m happy to keep it or leave it.
IMO, messing with this serialization code is not an improvement. It makes the PR more longer and more confusing than it needs to be and now repeats the READWRITE(tx, hashBlock, vMerkleBranch, nIndex) logic across 3 different functions instead of just leaving it in one place.
I’d suggest leave the inheritance untouched, leave the serialization untouched, getting rid of the comment explaining serialization workarounds, and just move all the CMerkleTx methods to CWalletTx, leaving CMerkleTx as a simple struct with a serialization operator.
IMO, messing with this serialization code is not an improvement. It makes the PR more longer and more confusing than it needs to be and now repeats the READWRITE(tx, hashBlock, vMerkleBranch, nIndex) logic across 3 different functions instead of just leaving it in one place.
I’d suggest leave the inheritance untouched, leave the serialization untouched, getting rid of the comment explaining serialization workarounds, and just move all the CMerkleTx methods to CWalletTx, leaving CMerkleTx as a simple struct with a serialization operator.
Thanks for reviewing @ryanofsky . I prefer having all the CWalletTx logic in a single place, but I can understand your point about making this PR smaller.
I’m happy to keep this PR as it is with two commits, reduce to just the first commit, or reduce further as you’ve suggested (put I’d probably add some comments to explain why we have a base class for CWalletTx that is just used for serialization/deserialization). I’ll wait for others to give their opinions before changing this.
but I can understand your point about making this PR smaller
PR size isn’t the main issue. The main thing I don’t like is that READWRITE(tx, hashBlock, vMerkleBranch, nIndex) logic is now repeated in 3 places, rewritten 3 slightly different ways, when it only needs to live in one place. The current serialization seems fine and little seems to be gained by blowing it up, since either way you have to keep the CMerkleTx class. The difference is that with my suggestion, CMerkleTx is something normal: a straightforward struct with a serialization operator. While with the current commits, CMerkleTx turns into this weird duplicative dangling thing requiring explanations from ancient history.
I also don’t understand the division between the two commits. First a “Remove CMerkleTx” commit that doesn’t actually remove it, then a “tidy up” commit with no description that changes more random things?
So I don’t get why a simple MOVEONLY commit that just moves all CMerkleTx methods except SerializationOp to CWalletTx isn’t the obvious choice here. But I’m mostly just perplexed by the choice. If you actually think the changes here are great and make sense then please keep them!
460@@ -512,7 +461,10 @@ class CWalletTx : public CMerkleTx
461 mutable bool fInMempool;
462 mutable CAmount nChangeCached;
463
464- CWalletTx(const CWallet* pwalletIn, CTransactionRef arg) : CMerkleTx(std::move(arg))
465+ CWalletTx(const CWallet* pwalletIn, CTransactionRef arg)
466+ : tx(std::move(arg)),
467+ hashBlock(uint256()),
fd2773f2eba659d78fae9d74461d9dd952609aa9
nit, personally I prefer
0: tx(std::move(arg))
1, hashBlock(uint256())
2, nIndex(-1)
clang-format
6779b7cbdeec091bf2c5a704c07a018641625d5f
None is AFAICT.
499@@ -505,22 +500,21 @@ class CWalletTx
500 mapValueCopy["timesmart"] = strprintf("%u", nTimeSmart);
501 }
502
503- std::vector<uint256> dummy_vector; //!< Used to be vMerkleBranch
504- s << tx << hashBlock << dummy_vector << nIndex;
505- std::vector<CMerkleTx> vUnused; //!< Used to be vtxPrev
506- s << vUnused << mapValueCopy << vOrderForm << fTimeReceivedIsTxTime << nTimeReceived << fFromMe << fSpent;
507+ std::vector<char> dummy_vector1; //!< Used to be vMerkleBranch
508+ std::vector<char> dummy_vector2; //!< Used to be vtxPrev
fad5decb1fdab249f626c6468174c50be15f14cf
Just to be sure, this means that an empty vector<char> serialized is compatible with the unserialization of vector<CMerkleTx>?
Edit: nervermind, went and see Unserialize_impl for vectors.
vector<>s get serialized to a single 0 byte (the compactSize of an empty vector)
in commit fad5dec:
I think you can keep everything in this function as is (except changing the type of the vector). This should simplify review.
519- std::vector<uint256> dummy_vector; //!< Used to be vMerkleBranch
520- s >> tx >> hashBlock >> dummy_vector >> nIndex;
521- std::vector<CMerkleTx> vUnused; //!< Used to be vtxPrev
522- s >> vUnused >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> fSpent;
523+ std::vector<uint256> dummy_vector1; //!< Used to be vMerkleBranch
524+ std::vector<CMerkleTx> dummy_vector2; //!< Used to be vtxPrev
fad5decb1fdab249f626c6468174c50be15f14cf
Could remove this and inline CMerkleTx::Unserialize, see:
0--- a/src/wallet/wallet.h
1+++ b/src/wallet/wallet.h
2@@ -365,26 +365,6 @@ struct COutputEntry
3 int vout;
4 };
5
6-/** Legacy class used for deserializing vtxPrev for backwards compatibility.
7- * vtxPrev was removed in commit 93a18a3650292afbb441a47d1fa1b94aeb0164e3,
8- * but old wallet.dat files may still contain vtxPrev vectors of CMerkleTxs.
9- * These need to get deserialized for field alignment when deserializing
10- * a CWalletTx, but the deserialized values are discarded.**/
11-class CMerkleTx
12-{
13-public:
14- template<typename Stream>
15- void Unserialize(Stream& s)
16- {
17- CTransactionRef tx;
18- uint256 hashBlock;
19- std::vector<uint256> vMerkleBranch;
20- int nIndex;
21-
22- s >> tx >> hashBlock >> vMerkleBranch >> nIndex;
23- }
24-};
25-
26 //Get the marginal bytes of spending the specified output
27 int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* pwallet, bool use_max_sig = false);
28
29@@ -512,9 +492,17 @@ public:
30 Init(nullptr);
31
32 std::vector<uint256> dummy_vector1; //!< Used to be vMerkleBranch
33- std::vector<CMerkleTx> dummy_vector2; //!< Used to be vtxPrev
34 char dummy_char; //! Used to be fSpent
35- s >> tx >> hashBlock >> dummy_vector1 >> nIndex >> dummy_vector2 >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> dummy_char;
36+ s >> tx >> hashBlock >> dummy_vector1 >> nIndex;
37+ {
38+ CTransactionRef tx;
39+ uint256 hashBlock;
40+ std::vector<uint256> vMerkleBranch;
41+ int nIndex;
42+
43+ s >> tx >> hashBlock >> vMerkleBranch >> nIndex; //!< Used to be vtxPrev
44+ }
45+ s >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> dummy_char;
46
47 ReadOrderPos(nOrderPos, mapValue);
48 nTimeSmart = mapValue.count("timesmart") ? (unsigned int)atoi64(mapValue["timesmart"]) : 0;
And if you do so then I’d squash now that CMerkleTx is actually removed.
This isn’t equivalent. vtxPrev was a vector, so:
CMerkleTx objects serializedI would like to remove CMerkleTx entirely, but implementing the low-lever deserialization logic for a vector of CMerkleTxs here seems like it could be easy to introduce an inconsistency (potentially causing wallets to get corrupted) and it seems wrong to be adding low-level deserialization code to this file.
You could read compact size, then loop to read dummy values but really not worth it.
Exactly. Seems error-prone and redundant.
Concept ACK.
First a “Remove CMerkleTx” commit that doesn’t actually remove it
I was going to say the same. BTW nit, add wallet prefix to commit message.
@ryanofsky suggestion to keep CMerkleTx just for serialization/unserialization make the code more straightforward and easy to understand than the last commit, IMO.
Otherwise my comment below has a suggestion to actually drop CMerkleTx, but I think with Russ suggestion for now.
The main thing I don’t like is that READWRITE(tx, hashBlock, vMerkleBranch, nIndex) logic is now repeated in 3 places, rewritten 3 slightly different ways, when it only needs to live in one place.
CWalletTx already has separate functions for serialize and deserialize, and adding the logic to both places seems to fit with that pattern.
I much prefer that after this PR I can look at a single function to see how a wallet tx will be serialized or deserialized, rather than seeing the object being static_casted to a CMerkleTx, then (de)serialized, then a vector of CMerkleTxs (de)serialized.
Longer term, I think it might make sense to move all of the serialization logic for the wallet objects to walletdb.{cpp,h}. There are already some objects that are defined in that header file with (de)serialization functions and deserialization fixups in ReadKeyValue().
I also don’t understand the division between the two commits. First a “Remove CMerkleTx” commit that doesn’t actually remove it, then a “tidy up” commit with no description that changes more random things?
I’ll try to improve the commit structure/messages.
So I don’t get why a simple MOVEONLY commit that just moves all CMerkleTx methods except SerializationOp to CWalletTx isn’t the obvious choice here.
I can certainly do that. The main aim of this PR is to move the CMerkleTx members into CWalletTx. Flattening the class hierarchy seemed like a good next step, but I can remove it from this PR if that’s what others want.
512
513 template<typename Stream>
514 void Unserialize(Stream& s)
515 {
516 Init(nullptr);
517- char fSpent;
in commit fad5decb1fdab249f626c6468174c:
I think this function does not need to be changed and can be kept as is.
I also don’t understand the division between the two commits.
I’ve resplit the changes into three commits:
CMerkleTx methods except initialization and serialization into CWalletTxCMerkleTx data members into CWalletTx and moves the serialization logic into CWalletTx.CMerkleTx serialization, just leaving CMerkleTx deserialization.Only (1) is needed. I think (2) and (3) are nice-to-haves, but can split them into a separate PR if others disagree.
408-
409 ADD_SERIALIZE_METHODS;
410
411 template <typename Stream, typename Operation>
412 inline void SerializationOp(Stream& s, Operation ser_action) {
413+ CTransactionRef tx;
6779b7cbdeec091bf2c5a704c07a018641625d5f
Just noting that it’s not really necessary to make these local variables since that unserialized CMerkleTx are discarded anyway. I guess you are doing this to underline the idea, so maybe also add a comment here?
Yeah, it’s equivalent, but I think the intent is clearer this way. Why have data members in a class where any instantiated objects are just going to be thrown away?
I don’t think an additional comment is necessary here - the code along with the comment on the CMerkleTx class should be clear enough.
I couldn’t figure out how to replicate the deserialization code for CMerkleTx in CWalletTx::deserialize() and be sure that I’d replicated it byte-for-byte, so I left that in.
FWIW, it’s possible to do something like std::vector<std::pair<CTransactionRef, std::pair<uint256, std::pair<...>>>> tmp; ss >> tmp; (with a nesting of pairs correspond to all the fields in the original CMerkleTx).
Not sure that’s an improvement though.
FWIW, it’s possible to do something like std::vector<std::pair<CTransactionRef, std::pair<uint256, std::pair<…»» tmp; ss » tmp; (with a nesting of pairs correspond to all the fields in the original CMerkleTx).
Thanks! I’ve implemented that here: https://github.com/jnewbery/bitcoin/commit/efcdb2e02c2c2b1726fc0c973869bb325685a893, which removes CMerkleTx entirely. I don’t feel confident reviewing that it will always deserialize the bytes identically, and I’d want to test with an old wallet file before proposing that we make that change.
I’ve reviewed the code and it looks good 458e43ed7cc3f4ddc196fc4e6e369ef8eba34aef
I have no strong opinion on whether the second and third commits are kept or not, I think the PR is good as-is
CMerkleTx only exists as a base class for CWalletTx and for wallet file
serialization/deserialization. Move CMerkleTx methods into CWalletTx,
but leave class hierarchy and serialization logic in place.
Removes CMerkleTx as a base class for CWalletTx. Serialization logic is
moved from CMerkleTx to CWalletTx.
CMerkleTx is only used for deserialization of old wallet files. Remove
the serialization logic, and tidy up CWalletTx serialization logic.
ACK 05b56d1c937b7667ad51400d2f9fb674af72953f. Looks good to me.
I can certainly do that. The main aim of this PR is to move the CMerkleTx members into CWalletTx. Flattening the class hierarchy seemed like a good next step
Having an intermediate base class just for serialization/deserialization is just silly and I’m all for getting rid of that in this PR.
And I think flattening the class hierarchy when possible is great in general, this simplifies understanding of the code.
Changing it to a separate “legacy class” solely for de-serializing makes a lot of sense to me, it’s easier to review for correctness/compatibility and also self-documenting, more so than std::pair<CTransactionRef, std::pair<uint256, std::pair<std::vector<uint256>, int>>>.