Make CTransaction actually immutable

sipa commented at 4:47 pm on August 24, 2016: member

This is an alternative to #8451.

Instead of making CTransaction mutable-but-with-cached-hash, it makes it actually fully immutable and never modifies any of its elements (apart from wit) after construction.

To do so, we heavily rely on C++11. CTransaction gets a (CMutableTransaction&&) constructor that efficiently converts a CMutableTransaction into a CTransaction without copying. In addition, CTransaction gets a deserializing constructors.

One downside is that CWalletTx cannot easily inherent from CTransaction anymore, as CWalletTx needs a way to modify the CTransaction data inside. By turning the superclass into a CTransactionRef field, it can take advantage (not implemented yet) of sharing CTransactions with the mempool.

sipa force-pushed on Aug 24, 2016

jonasschnelli added the label Refactoring on Aug 25, 2016

laanwj commented at 8:20 am on August 25, 2016: member

Concept ACK, thanks for doing this!

sipa commented at 9:14 am on August 25, 2016: member

@daira This is probably interesting to you as well.

The reason for this alternative is that @laanwj pointed out that #8451 is a step in the wrong direction if we want a more encapsulated CTransaction (which perhaps no longer uses the same representation, but uses a single-malloced block of data for example).

sipa force-pushed on Aug 25, 2016

in src/primitives/transaction.h: in 870833d120 outdated

467      */
468     uint256 GetHash() const;
469+
470+    friend bool operator==(const CMutableTransaction& a, const CMutableTransaction& b)
471+    {
472+        return a.GetHash() == b.GetHash();

luke-jr commented at 3:35 am on August 27, 2016:

This ignores witness data. Is that intentional?

sipa commented at 4:45 pm on August 27, 2016: member

@luke-jr: No, but it’s what the old code did as well (it automatically converted the CMutableTransactions to CTransaction, and then invoked its operator== which only compares the hashes).

dcousens commented at 0:59 am on August 29, 2016: contributor

This PR implements the latter - which I believe to be well-defined but very ugly. Suggestions or rants welcome.

My understanding was that even this hack is illegal and UB. Another good explanation of the edge cases and reasons against this. The triviality of causing UB, even with extreme caution is probably not acceptable, especially for miners who could potentially lose out on their reward.

As for an alternative solution, I don’t see one beyond the mass-copy at this point, but I’m having a look around.

The underlying conflict is assignment and const… you usually can’t have both without doing something illegal… (in C++)

sipa commented at 7:32 am on August 29, 2016: member

@dcousens Thanks. I will switch to a less efficient mechanism.

sipa force-pushed on Aug 29, 2016

sipa force-pushed on Sep 5, 2016

paveljanik commented at 8:00 am on September 15, 2016: contributor

S&M request: please rebase pain ;-)

sipa force-pushed on Sep 19, 2016

sipa commented at 11:15 am on September 19, 2016: member

Rebased.

gmaxwell commented at 5:29 pm on September 23, 2016: contributor

Would it be possible to simply change the representation of a block so the first transaction is handled separately? Perhaps even building up the hashtree so updating the root doesn’t require recomputing everything?

sipa commented at 5:32 pm on September 23, 2016: member

As a follow-up I just plan to turn CBlock::vtx into a vector of shared_ptr’s. That way we can more easily construct blocks from the mempool, more easily readd disconnected transactions back to the mempool, and don’t need all this hackery to update the coinbase.

gmaxwell commented at 7:29 pm on September 23, 2016: contributor

Concept ACK.

gmaxwell commented at 12:39 pm on September 24, 2016: contributor

Needs rebase.

sipa force-pushed on Oct 2, 2016

sipa commented at 11:46 pm on October 2, 2016: member

Rebased.

sipa force-pushed on Oct 21, 2016

sipa commented at 7:40 pm on October 21, 2016: member

Rebased.

in src/primitives/transaction.h: in 1c02fde3a6 outdated

393+    template <typename Stream>
394+    CTransaction(Stream& s, int nType, int nVersion) : CTransaction(CMutableTransaction(s, nType, nVersion)) {}
395+
396     template <typename Stream, typename Operation>
397     inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
398+        assert(!ser_action.ForRead()); /* Cannot deserialize CTransaction directly, as it has const fields */

theuni commented at 6:21 pm on October 28, 2016:

make CSerActionSerialize::ForRead() and CSerActionUnserialize::ForRead() constexpr, then you can static_assert at build-time instead.

sipa commented at 11:49 pm on October 29, 2016:

See #9039 (though I think after that, we won’t actually need this unimplemented branch anymore).

in src/blockencodings.h: in 1c02fde3a6 outdated

181@@ -173,16 +182,32 @@ class CBlockHeaderAndShortTxIDs {
182                     static_assert(SHORTTXIDS_LENGTH == 6, "shorttxids serialization assumes 6-byte shorttxids");
183                 }
184             }
185+
186+            uint64_t nptx;

TheBlueMatt commented at 7:17 pm on October 28, 2016:

nit: lol, can we get a real variable name here?

in src/qt/walletmodeltransaction.cpp: in 1c02fde3a6 outdated

26@@ -29,9 +27,15 @@ QList<SendCoinsRecipient> WalletModelTransaction::getRecipients()
27 
28 CWalletTx *WalletModelTransaction::getTransaction()
29 {
30-    return walletTransaction;
31+    return walletTransaction.get();
32 }
33 
34+void WalletModelTransaction::setTransaction(CWalletTx* tx)

kazcw commented at 7:26 pm on October 28, 2016:

Nit: could accept a unique_ptr&& so the API documents the ownership semantics

in src/primitives/block.h: in 1c02fde3a6 outdated

 9@@ -10,6 +10,8 @@
10 #include "serialize.h"
11 #include "uint256.h"
12 
13+#define MAX_BLOCK_TRANSACTIONS 16665

TheBlueMatt commented at 7:30 pm on October 28, 2016:

Can you make this a define based on the other block size limits and min transaction size isntead of making it unexplained magic?

in src/main.cpp: in 1c02fde3a6 outdated

3520+    // We need a full copy of vtx to modify the coinbase.
3521+    std::vector<CTransaction> vtx;
3522+    vtx.reserve(block.vtx.size());
3523+    vtx.emplace_back(std::move(mcoinbase));
3524+    for (size_t i = 1; i < block.vtx.size(); i++) {
3525+        vtx.emplace_back(block.vtx[i]);

TheBlueMatt commented at 7:48 pm on October 28, 2016:

Can’t you std::move here? That object is not used anywhere hereafter, except to be destructed.

sipa commented at 11:47 pm on October 29, 2016:

If CTransaction is entirely immutable, a move constructor would just be identical to a copy constructor, so no need. We could leave a move constructor that only moves the (mutable) witness field, but that would get more complicated with #8589 (which moves the witness data inside CTxIn). I think a better (but follow-up) change would be changing CBlock::vtx to a std::vector<std::shared_ptr<CTransaction>>.

in src/main.cpp: in 1c02fde3a6 outdated

5538@@ -5530,8 +5539,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5539 
5540         deque<COutPoint> vWorkQueue;
5541         vector<uint256> vEraseQueue;
5542-        CTransaction tx;
5543-        vRecv >> tx;
5544+        CTransaction tx = vRecv.Construct<CTransaction>();

TheBlueMatt commented at 7:52 pm on October 28, 2016:

It seems like you’re making a needless copy here? Why not just CTransaction tx(vRecv, vRecv.nType, vRecv.nVersion) (or, better yet, make a new constructor which knows how to pull out nType and nVersion).

sipa commented at 11:47 pm on October 29, 2016:

See #9039.

in src/miner.cpp: in 1c02fde3a6 outdated

606+    // We need a full copy of vtx to modify the coinbase.
607+    std::vector<CTransaction> vtx;
608+    vtx.reserve(pblock->vtx.size());
609+    vtx.emplace_back(std::move(txCoinbase));
610+    for (size_t i = 1; i < pblock->vtx.size(); i++) {
611+        vtx.emplace_back(pblock->vtx[i]);

TheBlueMatt commented at 7:53 pm on October 28, 2016:

I think you can move here, too.

in src/script/bitcoinconsensus.cpp: in 1c02fde3a6 outdated

80@@ -75,8 +81,7 @@ static int verify_script(const unsigned char *scriptPubKey, unsigned int scriptP
81 {
82     try {
83         TxInputStream stream(SER_NETWORK, PROTOCOL_VERSION, txTo, txToLen);
84-        CTransaction tx;
85-        stream >> tx;
86+        CTransaction tx = stream.Construct<CTransaction>();

TheBlueMatt commented at 8:38 pm on October 28, 2016:

Same here: needless copy sucks.

in src/wallet/wallet.cpp: in 1c02fde3a6 outdated

2419@@ -2422,7 +2420,9 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2420                 }
2421 
2422                 // Embed the constructed transaction data in wtxNew.
2423-                *static_cast<CTransaction*>(&wtxNew) = CTransaction(txNew);
2424+                wtxNew.reset(new CWalletTx(this, CTransaction(std::move(txNew))));

kazcw commented at 8:53 pm on October 28, 2016:

moved here, used below

kazcw changes_requested

kazcw commented at 8:54 pm on October 28, 2016: contributor

utACK except CreateTransaction bug

in src/serialize.h: in 1c02fde3a6 outdated

957@@ -958,6 +958,25 @@ class CSizeComputer
958     size_t size() const {
959         return nSize;
960     }
961+
962+    /** Unimplemented, but needed inside unused SerializeOp deserialization branches */

TheBlueMatt commented at 8:55 pm on October 28, 2016:

I really dont like adding methods here that we just assert(false) on…would prefer to just call the constructor directly instead.

in src/wallet/rpcwallet.cpp: in 1c02fde3a6 outdated

412@@ -410,21 +413,23 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
413         throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send");
414 
415     // Wallet comments
416-    CWalletTx wtx;
417-    if (request.params.size() > 2 && !request.params[2].isNull() && !request.params[2].get_str().empty())
418-        wtx.mapValue["comment"] = request.params[2].get_str();
419-    if (request.params.size() > 3 && !request.params[3].isNull() && !request.params[3].get_str().empty())
420-        wtx.mapValue["to"]      = request.params[3].get_str();
421+    std::unique_ptr<CWalletTx> wtx;

TheBlueMatt commented at 8:58 pm on October 28, 2016:

Tiny nit: can you move this down to right above SendMoney, so its clear that its not init’d until then 9and in the next two functions as well)

in src/wallet/wallet.h: in 1c02fde3a6 outdated

348@@ -319,15 +349,10 @@ class CWalletTx : public CMerkleTx
349             Init(NULL);

TheBlueMatt commented at 9:09 pm on October 28, 2016:

Might as well make the line above this an assert as well (and remove this line)

TheBlueMatt commented at 9:11 pm on October 28, 2016: member

Generally LGTM, just one or two comments and some nits (though I didnt look at tests, mostly).

sipa commented at 11:43 pm on October 29, 2016: member

I’d like to first get #9039 reviewed, as that would remove the need for the Construct method in streams (and thus allowing to construct an object directly by passing a stream into its constructor).

TheBlueMatt commented at 11:44 pm on October 29, 2016: member

Agreed, let’s do #9039 first.

On October 29, 2016 7:43:16 PM EDT, Pieter Wuille notifications@github.com wrote:

I’d like to first get #9039 reviewed, as that would remove the need for the Construct method in streams (and thus allowing to construct an object directly by passing a stream into its constructor).

You are receiving this because you commented. Reply to this email directly or view it on GitHub: #8580 (comment)

sipa force-pushed on Nov 13, 2016

sipa commented at 1:33 am on November 13, 2016: member

Rebased on top of #9125, using a more efficent approach that turns more CTransactions into shared_ptrs (see updated PR description).

This invalidates any earlier review, sorry.

sipa force-pushed on Nov 13, 2016

sipa force-pushed on Nov 15, 2016

in src/bitcoin-tx.cpp: in f630f2ee0d outdated

641@@ -642,10 +642,12 @@ static int CommandLineRawTx(int argc, char* argv[])
642                 throw runtime_error("invalid transaction encoding");
643 
644             startArg = 2;
645-        } else
646+        } else {
647+            txDecodeTmp = MakeTransactionRef();

theuni commented at 9:33 pm on November 17, 2016:

unneeded?

sipa commented at 1:05 am on November 18, 2016:

Nope, without it, bitocin-tx -create segfaults.

theuni commented at 10:22 am on November 18, 2016:

Whoops, I see. Initializing txDecodeTmp above (line 628) with a nullptr might make it more obvious to a casual reader that this points to nowhere initially (though you’d think the “Ref” part would’ve given it away…). No big deal though.

in src/wallet/wallet.h: in c1955323ad outdated

188 
189-    CMerkleTx(const CTransaction& txIn) : CTransaction(txIn)
190-    {
191-        Init();
192-    }
193+    operator const CTransaction&() const { return *tx; }

jtimon commented at 0:58 am on November 18, 2016:

Where is this operator used? CTransactionRef tx is public. Maybe a getter and making it protected would be better?

sipa commented at 0:59 am on November 18, 2016:

This is an implicit conversion operator. It allows you to pass a CWalletTx in any place where a ‘const CTransaction&’ is expected. I can avoid it by changing a number of extra lines.

jtimon commented at 1:27 am on November 18, 2016:

oh, I see, it’s just to avoid extra disruption. I would prefer it gone, yes. But it doesn’t need to be in this PR. Thoughts on the getter instead of a tx being public? Changing that shouldn’t extra disruption. I guess this can be done later too, but it’s “free” now disruption-wise.

ryanofsky commented at 9:34 pm on November 30, 2016:

Maybe add a todo comment to remove this operator in the future.

in src/primitives/transaction.h: in 63296e690a outdated

409+    template <typename Stream>
410+    inline void Serialize(Stream& s) const {
411+        SerializeTransaction(*this, s);
412     }
413 
414+    /* No Unserialize method. Use the deserializing constructor below instead. */

jtimon commented at 1:38 am on November 18, 2016:

Perhaps this could be turned into doxygen documentation for the deserializer constructor. ie “This constructor is provided instead of a deserialize method since the latter requires violating the const nature of most fields in CTransaction” or something of the sort.

sipa commented at 8:19 pm on December 1, 2016:

Done.

jtimon commented at 1:50 am on November 18, 2016: contributor

utACK c1955323ad9135d8e845a885063eb608c250beb3 left-UnserializeTransaction-separation-from-SerializeTransaction-for-another-day utACK 63296e690a0805bc6501d69d100add8c0abdeb3c

sipa force-pushed on Nov 20, 2016

sipa force-pushed on Nov 22, 2016

sipa commented at 7:08 pm on November 22, 2016: member

Rebased.

gmaxwell commented at 8:10 pm on November 25, 2016: contributor

tested ACK! (you might want to rebase, it applies with a bit of fuzz)

in src/primitives/transaction.cpp: in db4783e177 outdated

91-    *const_cast<CTxWitness*>(&wit) = tx.wit;
92-    *const_cast<unsigned int*>(&nLockTime) = tx.nLockTime;
93-    *const_cast<uint256*>(&hash) = tx.hash;
94-    return *this;
95-}
96+CTransaction::CTransaction() : nVersion(CTransaction::CURRENT_VERSION), vin(), vout(), nLockTime(0) {}

TheBlueMatt commented at 3:01 am on November 29, 2016:

Hmm…this behavior is very strange. We are leaving hash null, and while this isnt a behavior change, I cant imagine it is good…

sipa commented at 3:57 am on November 29, 2016:

It basically defines the hash of the empty transaction (which by consensus is always invalid) as 0, rather than technically SHA256^2(0x00000000000000000000).

TheBlueMatt commented at 5:43 am on November 29, 2016:

It seems strange that you could construct the same transaction via CMutableTransaction or via CTransaction, and have them have a different hash, and be operator!=. Maybe not something to fix for this PR, though.

sipa commented at 11:18 pm on November 30, 2016:

Agree, let’s fix this, but not in this PR.

ryanofsky commented at 4:52 pm on December 1, 2016:

Maybe add an explicit hash() initializer or comment to make the behavior more obvious & document it.

sipa commented at 8:19 pm on December 1, 2016:

Done, and added a TODO to remove the constructor entirely.

in src/primitives/transaction.h: in db4783e177 outdated

285@@ -286,73 +286,81 @@ struct CMutableTransaction;
286  *   - CTxWitness wit;
287  * - uint32_t nLockTime
288  */
289-template<typename Stream, typename Operation, typename TxType>
290-inline void SerializeTransaction(TxType& tx, Stream& s, Operation ser_action) {
291+template<typename Stream, typename TxType>
292+inline void UnserializeTransaction(TxType& tx, Stream& s) {

TheBlueMatt commented at 3:13 am on November 29, 2016:

nit: I believe the usual term is “Deserialize”

sipa commented at 3:57 am on November 29, 2016:

I agree, but whomever wrote serialize.h originally did not. The implicitly-defined method that classes get for deserialization is called Unserialize.

TheBlueMatt commented at 3:26 am on November 29, 2016: member

utACK db4783e1774fd64c09e826b7bf4e851fa0195f8e

in src/core_read.cpp: in 0482f43c17 outdated

89@@ -90,7 +90,7 @@ CScript ParseScript(const std::string& s)
90     return result;
91 }
92 
93-bool DecodeHexTx(CTransaction& tx, const std::string& strHexTx, bool fTryNoWitness)
94+bool DecodeHexTx(CTransactionRef& tx, const std::string& strHexTx, bool fTryNoWitness)

ryanofsky commented at 9:22 pm on November 30, 2016:

Maybe DecodeHexTx should be changed to return CMutableTransaction instead of CTransaction. It seems awkward to require use of shared_ptr for a low-level decoding function, and CMutableTransaction would also let you get rid of the segfault workaround in bitcoin-tx.cpp.

sipa commented at 11:18 pm on November 30, 2016:

Agree, changed.

in src/wallet/wallet.h: in 6f7b5fa60d outdated

197         hashBlock = uint256();
198         nIndex = -1;
199     }
200 
201+    template<typename... Args>
202+    void SetTx(Args&&... args)

ryanofsky commented at 10:07 pm on November 30, 2016:

This std::forward stuff is interesting because it means that the SetTx method and the CMerkleTx and CWalletTx constructors can all be called with arguments for any of the 3 non-template overloads of MakeTransactionRef, or with arguments for any of the 4 overloaded CTransaction constructors. But I wonder if you would consider defining SetTx more simply as:

0void SetTx(CTransactionRef ref)
1{
2    tx = std::move(ref);
3}

and similarly replacing the template constructors with regular constructors accepting CTransactionRefs.

The advantage of doing this for someone reading the code is that you won’t have to look 3 levels deep to see what arguments a SetTx call or constructor will accept, and of course won’t have to know anything about std::forward either.

sipa commented at 11:17 pm on November 30, 2016:

I’ve changed CMerkleTx and CWalletTx to just take a CTransactionRef argument now as you suggested, effectively moving the MakeTransactionRef call to the constructor callers. By doing so, I discovered a few places where an std::move could be introduced to avoid a copy, so that makes the change worth it already.

ryanofsky approved

sipa force-pushed on Nov 30, 2016

sipa commented at 11:19 pm on November 30, 2016: member

Rebased, and addressed @ryanofsky’s comments.

sipa force-pushed on Nov 30, 2016

sipa force-pushed on Dec 1, 2016

in src/rpc/rawtransaction.cpp: in 8ac50ee4e7 outdated

519@@ -520,13 +520,13 @@ UniValue decoderawtransaction(const JSONRPCRequest& request)
520     LOCK(cs_main);
521     RPCTypeCheck(request.params, boost::assign::list_of(UniValue::VSTR));
522 
523-    CTransaction tx;
524+    CMutableTransaction tx;

ryanofsky commented at 4:16 pm on December 1, 2016:

Maybe rename these tx variables to mtx. It would make the diff a little bigger, but also make the code review easier in confirming that all uses of these variables are still valid (especially with the new std::move calls which trash them).

sipa commented at 8:27 pm on December 1, 2016:

Done.

in src/rpc/rawtransaction.cpp: in 8ac50ee4e7 outdated

900@@ -901,7 +901,7 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
901         // push to local node and sync with wallets
902         CValidationState state;
903         bool fMissingInputs;
904-        if (!AcceptToMemoryPool(mempool, state, tx, fLimitFree, &fMissingInputs, false, nMaxRawTxFee)) {
905+        if (!AcceptToMemoryPool(mempool, state, CTransaction(std::move(tx)), fLimitFree, &fMissingInputs, false, nMaxRawTxFee)) {

ryanofsky commented at 4:26 pm on December 1, 2016:

Maybe revert the change in this line and instead insert a CTransaction tx(std::move(mtx)); declaration at line 889 after the decode. This would make it easier to understand the variable lifetimes in this function and also avoid calculating the tx hash twice.

sipa commented at 8:27 pm on December 1, 2016:

Done.

in src/wallet/wallet.cpp: in 343dfda84a outdated

2493@@ -2494,7 +2494,7 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2494                 }
2495 
2496                 // Embed the constructed transaction data in wtxNew.
2497-                *static_cast<CTransaction*>(&wtxNew) = CTransaction(txNew);
2498+                wtxNew.SetTx(MakeTransactionRef(txNew));

ryanofsky commented at 4:41 pm on December 1, 2016:

If you change txNew to std::move(txNew) here and GetTransactionWeight(txNew) to GetTransactionWeight(*wtxNew->tx) immediately below, you could avoid two transaction copies and hash computation.

sipa commented at 8:30 pm on December 1, 2016:

Done.

in src/wallet/wallet.h: in 343dfda84a outdated

291-    {
292-        Init(pwalletIn);
293-    }
294-
295-    CWalletTx(const CWallet* pwalletIn, const CTransaction& txIn) : CMerkleTx(txIn)
296+    template<typename... Args>

ryanofsky commented at 4:43 pm on December 1, 2016:

Template line not needed.

sipa commented at 8:31 pm on December 1, 2016:

Done.

in src/primitives/transaction.h: in 450ca59e12 outdated

389+        s << flags;
390+    }
391+    s << tx.vin;
392+    s << tx.vout;
393+    if (flags & 1) {
394+        for (size_t i = 0; i < tx.vin.size(); i++) {

ryanofsky commented at 5:12 pm on December 1, 2016:

Isn’t a WriteCompactSize(s, tx.wit.vtxinwit.size()); call needed here? The previous code was calling READWRITE(tx.wit) which serialized the whole vector, and the Unserialize function here also seems to deserialize the entire vector instead of the individual elements.

Edit: Never mind, I confused serializing the witness vector with serializing the CTxWitness class which wraps the vector and assumes a predetermined vector size.

sipa commented at 8:33 pm on December 1, 2016:

Indeed. The code is a bit ugly in that the serializer does the witness serialization inline, while the deserializer uses CTxWitness. In #8589 CTxWitness is removed entirely.

in src/primitives/transaction.h: in 450ca59e12 outdated

324+    tx.vin.clear();
325+    tx.vout.clear();
326+    tx.wit.SetNull();
327+    /* Try to read the vin. In case the dummy is there, this will be read as an empty vector. */
328+    s >> tx.vin;
329+    if (tx.vin.size() == 0 && fAllowWitness) {

ryanofsky commented at 5:28 pm on December 1, 2016:

I realize you didn’t change this code, but should Unserialize throw an exception if vin size is 0 and fAllowWitness is false? In this case the comment below “We read a non-empty vin. Assume a normal vout follows,” would not be accurate and the flags field would seemingly be misinterpreted as the vout vector length.

sipa commented at 8:33 pm on December 1, 2016:

No. When fAllowWitness is false, this code explicitly supports transactions with vin.size()=0. Even those are not valid due to consensus rules, it is necessary to correctly implement the ambiguitity detection in the decoderawtransaction and fundrawtransaction RPCs (which accept incomplete transactions as inputs).

ryanofsky commented at 9:42 pm on December 1, 2016:

I see, and thanks for clarifying even though this code isn’t really changed by your PR. Two other comments I would make (maybe for a separate PR) are:

The “We read a non-empty vin” part of the comment below is misleading and should probably be dropped or reworded.
The if (flags != 0) condition is confusing and should never be false.

Maybe the following would be an improvement:

0    s >> tx.vin;
1    if (tx.vin.size() == 0 && fAllowWitness) {
2        /* We read a dummy or an empty vin. */
3        s >> flags;
4        if (flags == 0)
5            throw std::ios_base::failure("Invalid transaction flags");
6        s >> tx.vin;
7    }
8    s >> tx.vout;

sipa commented at 11:25 pm on December 4, 2016:

Well there is a (perhaps misguided) attempt to parse the 10-byte [nVersion] 0x00 0x00 [nLockTime] as an empty transaction, rather than as an (incorrectly encoded) witness transaction.

sipa force-pushed on Dec 1, 2016

in src/wallet/wallet.cpp: in f006fcc1da outdated

2493@@ -2494,7 +2494,7 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2494                 }
2495 
2496                 // Embed the constructed transaction data in wtxNew.
2497-                *static_cast<CTransaction*>(&wtxNew) = CTransaction(txNew);
2498+                wtxNew.SetTx(MakeTransactionRef(std::move(txNew)));
2499 
2500                 // Limit size
2501                 if (GetTransactionWeight(txNew) >= MAX_STANDARD_TX_WEIGHT)

ryanofsky commented at 9:37 pm on December 1, 2016:

Need to change this to GetTransactionWeight(*wtxNew.tx) since txNew is moved from.

sipa commented at 10:56 pm on December 1, 2016:

Fixed.

ryanofsky approved

ryanofsky commented at 9:50 pm on December 1, 2016: member

utACK f006fcc1da680540def6c7d7d1f9b3d8aca8a1f8 with GetTransactionWeight fix.

sipa force-pushed on Dec 1, 2016

Switch GetTransaction to returning a CTransactionRef a1883536b4

Make CWalletTx store a CTransactionRef instead of inheriting c3f5673a63

Make DecodeHexTx return a CMutableTransaction 42fd8dee30

sipa force-pushed on Dec 3, 2016

Make CTransaction actually immutable 81e3228fcb

gmaxwell commented at 10:05 pm on December 4, 2016: contributor

Is there anything else this is waiting on?

laanwj commented at 7:03 am on December 5, 2016: member

utACK 81e3228

laanwj merged this on Dec 5, 2016

laanwj closed this on Dec 5, 2016

laanwj referenced this in commit 46904ee5d2 on Dec 5, 2016

in src/primitives/transaction.h: in 81e3228fcb

368+    const bool fAllowWitness = !(s.GetVersion() & SERIALIZE_TRANSACTION_NO_WITNESS);
369+
370+    s << tx.nVersion;
371+    unsigned char flags = 0;
372+    // Consistency check
373+    assert(tx.wit.vtxinwit.size() <= tx.vin.size());

paveljanik commented at 1:02 pm on December 6, 2016:

This assert is the cause of SIGSEGV when paying in the UI.

0Assertion failed: (tx.wit.vtxinwit.size() <= tx.vin.size()), function SerializeTransaction, file ./primitives/transaction.h, line 331.

NicolasDorier commented at 1:40 pm on December 6, 2016:

I remember a PR of @sipa where he made witness scripts part of the TxIn class. I wished it was merged so such error would not possibly happen. :(

morcos commented at 2:03 pm on December 6, 2016:

See #9296

zkbot referenced this in commit f9e39f293d on Jul 14, 2017

codablock referenced this in commit 857c549931 on Jan 16, 2018

codablock referenced this in commit 318215ac2b on Jan 16, 2018

codablock referenced this in commit 6eed004da5 on Jan 16, 2018

codablock referenced this in commit a260907693 on Jan 16, 2018

codablock referenced this in commit 1e62969fa6 on Jan 17, 2018

codablock referenced this in commit 525c049316 on Jan 17, 2018

andvgal referenced this in commit 5521e80f37 on Jan 6, 2019

andvgal referenced this in commit 2d2de12bad on Jan 6, 2019

CryptoCentric referenced this in commit 4dfa918ff6 on Feb 25, 2019

CryptoCentric referenced this in commit 4838d42ffe on Feb 25, 2019

CryptoCentric referenced this in commit 8eba182f8d on Feb 25, 2019

CryptoCentric referenced this in commit 93ec6e1a4c on Feb 26, 2019

CryptoCentric referenced this in commit 989963b41f on Feb 26, 2019

CryptoCentric referenced this in commit a2a6a6d54a on Feb 26, 2019

CryptoCentric referenced this in commit f1b08b4910 on Feb 26, 2019

CryptoCentric referenced this in commit 209db55a21 on Feb 26, 2019

CryptoCentric referenced this in commit d3f4277a6e on Feb 26, 2019

furszy referenced this in commit 8cd9c592a9 on May 29, 2020

furszy referenced this in commit 7b2b9d048e on Sep 24, 2020

furszy referenced this in commit 67b5bb6b8a on Feb 12, 2021

MarcoFalke locked this on Sep 8, 2021

Make CTransaction actually immutable #8580