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 **/
Could add the commit or wallet version where this was changed?
I've expanded the comment here
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;
Would this write uninitialized memory to the wallet file?
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.
Yeah, I am interested in seeing that commit. That probably means you can get rid of the 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?
I like the assert
But the following removes the serialization support so maybe not worth it?
I agree (if we decide to keep the second commit)
Just to clarify: When I open+close an "old" wallet file, this will overwrite all merkle txs with zeros? If yes, would it be safe to open such a file with the older version of Bitcoin Core?
Concept ACK
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.
Concept ACK. Light code review, build, all tests pass locally.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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;
You're invoking the serialization operators inside an Unserialize function. I suspect you mean to call the >> ones instead.
gah. Thanks. Fixed.
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
: tx(std::move(arg))
, hashBlock(uint256())
, nIndex(-1)
That is neither in the style-guide nor in clang-format
6779b7cbdeec091bf2c5a704c07a018641625d5f
None is AFAICT.
This code is fine. Please do not give formatting comments about preferences, that are not in the code style or developer notes,
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.
Correct. Empty 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.
See #16451 (comment). I'll remove everything after the first commit if others agree.
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:
<details>
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -365,26 +365,6 @@ struct COutputEntry
int vout;
};
-/** Legacy class used for deserializing vtxPrev for backwards compatibility.
- * vtxPrev was removed in commit 93a18a3650292afbb441a47d1fa1b94aeb0164e3,
- * but old wallet.dat files may still contain vtxPrev vectors of CMerkleTxs.
- * These need to get deserialized for field alignment when deserializing
- * a CWalletTx, but the deserialized values are discarded.**/
-class CMerkleTx
-{
-public:
- template<typename Stream>
- void Unserialize(Stream& s)
- {
- CTransactionRef tx;
- uint256 hashBlock;
- std::vector<uint256> vMerkleBranch;
- int nIndex;
-
- s >> tx >> hashBlock >> vMerkleBranch >> nIndex;
- }
-};
-
//Get the marginal bytes of spending the specified output
int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* pwallet, bool use_max_sig = false);
@@ -512,9 +492,17 @@ public:
Init(nullptr);
std::vector<uint256> dummy_vector1; //!< Used to be vMerkleBranch
- std::vector<CMerkleTx> dummy_vector2; //!< Used to be vtxPrev
char dummy_char; //! Used to be fSpent
- s >> tx >> hashBlock >> dummy_vector1 >> nIndex >> dummy_vector2 >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> dummy_char;
+ s >> tx >> hashBlock >> dummy_vector1 >> nIndex;
+ {
+ CTransactionRef tx;
+ uint256 hashBlock;
+ std::vector<uint256> vMerkleBranch;
+ int nIndex;
+
+ s >> tx >> hashBlock >> vMerkleBranch >> nIndex; //!< Used to be vtxPrev
+ }
+ s >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> dummy_char;
ReadOrderPos(nOrderPos, mapValue);
nTimeSmart = mapValue.count("timesmart") ? (unsigned int)atoi64(mapValue["timesmart"]) : 0;
</details>
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.
Ah sorry.. You could read compact size, then loop to read dummy values but really not worth it. You can resolve this.
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.
See #16451 (comment). I'll remove everything after the first commit if others agree.
ACK fad5dec
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.
ACK 458e43ed7cc3f4ddc196fc4e6e369ef8eba34aef, refactor reviewed, confirmed moved lines. I think 2nd and 3rd commits are fine.
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.
Light ACK 458e43ed7cc3f4ddc196fc4e6e369ef8eba34aef, code review of the 3 new commits, build with no warnings, all tests pass.
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.
rebased
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>>>.