Remove CMerkleTx #16451

jnewbery · 2019-07-24T14:50:48Z

CMerkleTx is only used as a base class for
CWalletTx. It was previously also used for vtxPrev which
was removed in 93a18a3.

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.

maflcko

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?

maflcko · 2019-07-24T15:29:27Z

src/wallet/wallet.h

@@ -365,82 +365,24 @@ struct COutputEntry
    int vout;
 };

-/** A transaction with a merkle branch linking it to the block chain. */
+/** 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

maflcko · 2019-07-24T15:34:56Z

src/wallet/wallet.h

    ADD_SERIALIZE_METHODS;

    template <typename Stream, typename Operation>
    inline void SerializationOp(Stream& s, Operation ser_action) {
+        CTransactionRef tx;
+        uint256 hashBlock;
+        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)

fd2773f

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)

maflcko · 2019-07-24T15:36:59Z

Concept ACK

jnewbery · 2019-07-24T15:49:56Z

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.

jonatack

Concept ACK. Light code review, build, all tests pass locally.

ariard · 2019-07-24T17:54:24Z

Concept ACK, should go before #15931 as it lets me remove one commit

DrahtBot · 2019-07-24T17:59:58Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16341 (Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)

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.

sipa · 2019-07-24T18:11:47Z

src/wallet/wallet.h

-        READWRITE(hashBlock);
-        READWRITE(vMerkleBranch);
-        READWRITE(nIndex);
+        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.

jnewbery · 2019-07-24T18:13:36Z

@MarcoFalke

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.

ryanofsky · 2019-07-24T20:32:56Z

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.

jnewbery · 2019-07-24T21:47:50Z

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.

ryanofsky · 2019-07-24T22:19:14Z

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!

promag

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.

promag · 2019-07-24T22:43:40Z

src/wallet/wallet.h

    ADD_SERIALIZE_METHODS;

    template <typename Stream, typename Operation>
    inline void SerializationOp(Stream& s, Operation ser_action) {
+        CTransactionRef tx;
+        uint256 hashBlock;
+        int nIndex;


fd2773f

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?

promag · 2019-07-24T22:45:32Z

src/wallet/wallet.h

-    CWalletTx(const CWallet* pwalletIn, CTransactionRef arg) : CMerkleTx(std::move(arg))
+    CWalletTx(const CWallet* pwalletIn, CTransactionRef arg)
+        : tx(std::move(arg)),
+          hashBlock(uint256()),


fd2773f

nit, personally I prefer

: tx(std::move(arg)) , hashBlock(uint256()) , nIndex(-1)

That is neither in the style-guide nor in clang-format

6779b7c

None is AFAICT.

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#variable-names ;)

I actually prefer your style @promag , but I'll keep this as is for now to not invalidate your review. If I need to update the PR again I'll swap to your style.

This code is fine. Please do not give formatting comments about preferences, that are not in the code style or developer notes,

promag · 2019-07-24T23:56:24Z

src/wallet/wallet.h

-        std::vector<CMerkleTx> vUnused; //!< Used to be vtxPrev
-        s << vUnused << mapValueCopy << vOrderForm << fTimeReceivedIsTxTime << nTimeReceived << fFromMe << fSpent;
+        std::vector<char> dummy_vector1; //!< Used to be vMerkleBranch
+        std::vector<char> dummy_vector2; //!< Used to be vtxPrev


fad5dec

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)

promag · 2019-07-25T00:00:23Z

src/wallet/wallet.h

-        std::vector<CMerkleTx> vUnused; //!< Used to be vtxPrev
-        s >> vUnused >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> fSpent;
+        std::vector<uint256> dummy_vector1; //!< Used to be vMerkleBranch
+        std::vector<CMerkleTx> dummy_vector2; //!< Used to be vtxPrev


fad5dec

Could remove this and inline CMerkleTx::Unserialize, see:

--- 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;

And if you do so then I'd squash now that CMerkleTx is actually removed.

This isn't equivalent. vtxPrev was a vector, so:

there can be multiple CMerkleTx objects serialized

the serialization is preceded with a compactSize

I 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.

jnewbery · 2019-07-25T14:26:35Z

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.

maflcko

ACK fad5dec

maflcko · 2019-07-25T14:31:49Z

src/wallet/wallet.h

    }

    template<typename Stream>
    void Unserialize(Stream& s)
    {
        Init(nullptr);
-        char fSpent;


in commit fad5dec:

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.

maflcko · 2019-07-25T14:32:58Z

src/wallet/wallet.h

-        std::vector<CMerkleTx> vUnused; //!< Used to be vtxPrev
-        s << vUnused << mapValueCopy << vOrderForm << fTimeReceivedIsTxTime << nTimeReceived << fFromMe << fSpent;
+        std::vector<char> dummy_vector1; //!< Used to be vMerkleBranch
+        std::vector<char> dummy_vector2; //!< Used to be vtxPrev


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.

jnewbery · 2019-07-25T15:00:41Z

@ryanofsky

I also don't understand the division between the two commits.

I've resplit the changes into three commits:

Moves the all CMerkleTx methods except initialization and serialization into CWalletTx
Moves CMerkleTx data members into CWalletTx and moves the serialization logic into CWalletTx.
Removes 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.

promag

ACK 458e43e, refactor reviewed, confirmed moved lines. I think 2nd and 3rd commits are fine.

promag · 2019-07-25T15:11:52Z

src/wallet/wallet.h

-        std::vector<CMerkleTx> vUnused; //!< Used to be vtxPrev
-        s >> vUnused >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> fSpent;
+        std::vector<uint256> dummy_vector1; //!< Used to be vMerkleBranch
+        std::vector<CMerkleTx> dummy_vector2; //!< Used to be vtxPrev


Ah sorry.. You could read compact size, then loop to read dummy values but really not worth it. You can resolve this.

promag · 2019-07-25T15:13:55Z

src/wallet/wallet.h

-    CWalletTx(const CWallet* pwalletIn, CTransactionRef arg) : CMerkleTx(std::move(arg))
+    CWalletTx(const CWallet* pwalletIn, CTransactionRef arg)
+        : tx(std::move(arg)),
+          hashBlock(uint256()),


6779b7c

None is AFAICT.

promag · 2019-07-25T15:36:13Z

src/wallet/wallet.h

    ADD_SERIALIZE_METHODS;

    template <typename Stream, typename Operation>
    inline void SerializationOp(Stream& s, Operation ser_action) {
+        CTransactionRef tx;


6779b7c

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.

sipa · 2019-07-25T16:12:25Z

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.

jnewbery · 2019-07-25T16:38:27Z

@sipa

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: jnewbery@efcdb2e, 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.

jonatack

Light ACK 458e43e, code review of the 3 new commits, build with no warnings, all tests pass.

meshcollider

I've reviewed the code and it looks good 458e43e

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.

jnewbery · 2019-07-30T15:57:21Z

rebased

laanwj · 2019-07-31T06:57:54Z

ACK 05b56d1. 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>>>.

05b56d1 [wallet] Remove CMerkleTx serialization logic (John Newbery) 783a76f [wallet] Flatten CWalletTx class hierarchy (John Newbery) b3a9d17 [wallet] Move CMerkleTx functions into CWalletTx (John Newbery) Pull request description: CMerkleTx is only used as a base class for CWalletTx. It was previously also used for vtxPrev which was removed in 93a18a3. 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. ACKs for top commit: laanwj: ACK 05b56d1. Looks good to me. Tree-SHA512: 3d3a0069ebb536b12a328f1261e7dc55158a71088d445ae4b4ace4142c432dc296f58c8183b1922e54a60b8cc77e9d17c3dce7478294cd68693594baacf2bab3

05b56d1 [wallet] Remove CMerkleTx serialization logic (John Newbery) 783a76f [wallet] Flatten CWalletTx class hierarchy (John Newbery) b3a9d17 [wallet] Move CMerkleTx functions into CWalletTx (John Newbery) Pull request description: CMerkleTx is only used as a base class for CWalletTx. It was previously also used for vtxPrev which was removed in 93a18a3. 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 bitcoin#15931 cleaner. ACKs for top commit: laanwj: ACK 05b56d1. Looks good to me. Tree-SHA512: 3d3a0069ebb536b12a328f1261e7dc55158a71088d445ae4b4ace4142c432dc296f58c8183b1922e54a60b8cc77e9d17c3dce7478294cd68693594baacf2bab3

… refactor 7e1aa0d Adapt GUI to the latest CTransactionRef migration changes. (furszy) 362366b BugFix: sapling operation, only set the final transaction at the end of the process. (furszy) 28aec49 [MOVE-ONLY] Wallet, moving CWalletTx and it's related classes above CWallet class definition. (furszy) 19d5338 [wallet] Remove CMerkleTx serialization logic (John Newbery) 77d1ce8 [wallet] Flatten CWalletTx class hierarchy (John Newbery) 9dd431f wallet: remove now unused CWalletTx default constructor (furszy) c08824f Wallet: mapWallet, moving away from map bracket operator usage. (furszy) 3e836f9 Decouple CWalletTx serialization in ser & unser methods. (furszy) 7073a78 [wallet] Move CMerkleTx functions into CWalletTx 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. (John Newbery) 03a4ebe Wallet: Moved CreateTransaction and CommitTransaction to use CTransactionRef (furszy) da8e05c Refactor: Migrated functions using CTransaction to CTransactionRef inside the wallet and validation areas. (furszy) Pull request description: Have grouped a good number of improvements and updates over the wallet area, need all of them for a subsequent, probably larger, PR that i'm cooking: 1) Migrated several functions using `CTransaction` to `CTransactionRef`. 2) Moved `CreateTransaction` and `CommitTransaction` methods signatures from using `CWalletTx` to use `CTransactionRef`. 3) Refactored every `mapWallet` bracket operator usage. 4) Removed `CWalletTx` default constructor (Thanks to (2) and (3)) 5) Decouple `CWalletTx` templated serialization in specific ser & unser functions. 6) Solved a small bug inside the sapling operation process, which was setting the final transaction before finalizing successfully. 7) Move-only, moved the `CWalletTx` and it's related classes definition above the `CWallet` class definition (required groundwork for a future PR). 8) Adapted upstream bitcoin#16451 to our sources. ACKs for top commit: random-zebra: ACK 7e1aa0d Fuzzbawls: ACK 7e1aa0d Tree-SHA512: fd993b2d9e1fc52ba1f11119ed39ed87221eee14fe79396e2392391d6d0a6b775ec4d617d1772f6ff21ddfefc9897521c5b30e8f494b7ce1b1069926d8ea9179

05b56d1 [wallet] Remove CMerkleTx serialization logic (John Newbery) 783a76f [wallet] Flatten CWalletTx class hierarchy (John Newbery) b3a9d17 [wallet] Move CMerkleTx functions into CWalletTx (John Newbery) Pull request description: CMerkleTx is only used as a base class for CWalletTx. It was previously also used for vtxPrev which was removed in 93a18a3. 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 bitcoin#15931 cleaner. ACKs for top commit: laanwj: ACK 05b56d1. Looks good to me. Tree-SHA512: 3d3a0069ebb536b12a328f1261e7dc55158a71088d445ae4b4ace4142c432dc296f58c8183b1922e54a60b8cc77e9d17c3dce7478294cd68693594baacf2bab3

DrahtBot added the Wallet label Jul 24, 2019

maflcko reviewed Jul 24, 2019

View reviewed changes

jonatack reviewed Jul 24, 2019

View reviewed changes

jnewbery force-pushed the 2019-07-remove-CMerkleTx branch from c3a4d16 to f7fe30c Compare July 24, 2019 18:09

sipa reviewed Jul 24, 2019

View reviewed changes

jnewbery force-pushed the 2019-07-remove-CMerkleTx branch from f7fe30c to 82ed04c Compare July 24, 2019 18:11

ariard mentioned this pull request Jul 24, 2019

Remove GetDepthInMainChain dependency on locked chain interface #15931

Merged

jnewbery force-pushed the 2019-07-remove-CMerkleTx branch from 82ed04c to fad5dec Compare July 24, 2019 21:41

promag reviewed Jul 25, 2019

View reviewed changes

maflcko reviewed Jul 25, 2019

View reviewed changes

jnewbery force-pushed the 2019-07-remove-CMerkleTx branch from fad5dec to 458e43e Compare July 25, 2019 14:57

promag reviewed Jul 25, 2019

View reviewed changes

jonatack reviewed Jul 25, 2019

View reviewed changes

meshcollider reviewed Jul 27, 2019

View reviewed changes

DrahtBot added the Needs rebase label Jul 30, 2019

jnewbery added 3 commits July 30, 2019 11:57

[wallet] Move CMerkleTx functions into CWalletTx

b3a9d17

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.

[wallet] Flatten CWalletTx class hierarchy

783a76f

Removes CMerkleTx as a base class for CWalletTx. Serialization logic is moved from CMerkleTx to CWalletTx.

[wallet] Remove CMerkleTx serialization logic

05b56d1

CMerkleTx is only used for deserialization of old wallet files. Remove the serialization logic, and tidy up CWalletTx serialization logic.

jnewbery force-pushed the 2019-07-remove-CMerkleTx branch from 458e43e to 05b56d1 Compare July 30, 2019 15:57

DrahtBot removed the Needs rebase label Jul 30, 2019

laanwj merged commit 05b56d1 into bitcoin:master Jul 31, 2019

furszy mentioned this pull request Feb 9, 2021

[Wallet] CTransactionRef migration + CWalletTx/CMerkleTx refactor PIVX-Project/PIVX#2201

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Remove CMerkleTx #16451

Remove CMerkleTx #16451

Conversation

jnewbery commented Jul 24, 2019

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented Jul 24, 2019

jnewbery commented Jul 24, 2019

jonatack left a comment

Choose a reason for hiding this comment

ariard commented Jul 24, 2019

DrahtBot commented Jul 24, 2019 • edited Loading

Conflicts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Jul 24, 2019

ryanofsky commented Jul 24, 2019

jnewbery commented Jul 24, 2019

ryanofsky commented Jul 24, 2019

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj Jul 31, 2019 • edited Loading

Choose a reason for hiding this comment

promag Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Jul 25, 2019

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Jul 25, 2019

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa commented Jul 25, 2019

jnewbery commented Jul 25, 2019

jonatack left a comment

Choose a reason for hiding this comment

meshcollider left a comment

Choose a reason for hiding this comment

jnewbery commented Jul 30, 2019

laanwj commented Jul 31, 2019

DrahtBot commented Jul 24, 2019 •

edited

Loading

laanwj Jul 31, 2019 •

edited

Loading

promag Jul 24, 2019 •

edited

Loading