Make CTransaction actually immutable #8580

pull sipa wants to merge 4 commits into bitcoin:master from sipa:pain changing 28 files +287 −292
  1. 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.

  2. sipa force-pushed on Aug 24, 2016
  3. jonasschnelli added the label Refactoring on Aug 25, 2016
  4. laanwj commented at 8:20 AM on August 25, 2016: member

    Concept ACK, thanks for doing this!

  5. 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).

  6. sipa force-pushed on Aug 25, 2016
  7. sipa force-pushed on Aug 25, 2016
  8. in src/primitives/transaction.h:None 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?

  9. 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).

  10. dcousens commented at 12: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++)

  11. sipa commented at 7:32 AM on August 29, 2016: member

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

  12. sipa force-pushed on Aug 29, 2016
  13. sipa force-pushed on Sep 5, 2016
  14. paveljanik commented at 8:00 AM on September 15, 2016: contributor

    S&M request: please rebase pain ;-)

  15. sipa force-pushed on Sep 19, 2016
  16. sipa commented at 11:15 AM on September 19, 2016: member

    Rebased.

  17. 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?

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

  19. gmaxwell commented at 7:29 PM on September 23, 2016: contributor

    Concept ACK.

  20. gmaxwell commented at 12:39 PM on September 24, 2016: contributor

    Needs rebase.

  21. sipa force-pushed on Oct 2, 2016
  22. sipa commented at 11:46 PM on October 2, 2016: member

    Rebased.

  23. sipa force-pushed on Oct 21, 2016
  24. sipa commented at 7:40 PM on October 21, 2016: member

    Rebased.

  25. in src/primitives/transaction.h:None 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).

  26. in src/blockencodings.h:None 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?

  27. in src/qt/walletmodeltransaction.cpp:None 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

  28. in src/primitives/block.h:None 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?

  29. in src/main.cpp:None 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>>.

  30. in src/main.cpp:None 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.

  31. in src/miner.cpp:None 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.

  32. in src/script/bitcoinconsensus.cpp:None 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.

  33. in src/wallet/wallet.cpp:None 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

  34. kazcw changes_requested
  35. kazcw commented at 8:54 PM on October 28, 2016: contributor

    utACK except CreateTransaction bug

  36. in src/serialize.h:None 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.

  37. in src/wallet/rpcwallet.cpp:None 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)

  38. in src/wallet/wallet.h:None 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)

  39. 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).

  40. 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).

  41. 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)

  42. sipa force-pushed on Nov 13, 2016
  43. 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.

  44. sipa force-pushed on Nov 13, 2016
  45. sipa force-pushed on Nov 15, 2016
  46. in src/bitcoin-tx.cpp:None 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.

  47. in src/wallet/wallet.h:None 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 12: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 12: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.

  48. in src/primitives/transaction.h:None 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.

  49. jtimon commented at 1:50 AM on November 18, 2016: contributor

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

  50. sipa force-pushed on Nov 20, 2016
  51. sipa force-pushed on Nov 22, 2016
  52. sipa commented at 7:08 PM on November 22, 2016: member

    Rebased.

  53. 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)

  54. in src/primitives/transaction.cpp:None 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.

  55. in src/primitives/transaction.h:None 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.

  56. TheBlueMatt commented at 3:26 AM on November 29, 2016: member

    utACK db4783e1774fd64c09e826b7bf4e851fa0195f8e

  57. in src/core_read.cpp:None 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.

  58. in src/wallet/wallet.h:None 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:

    void SetTx(CTransactionRef ref)
{
    tx = std::move(ref);
}

    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.

  59. ryanofsky approved
  60. sipa force-pushed on Nov 30, 2016
  61. sipa commented at 11:19 PM on November 30, 2016: member

    Rebased, and addressed @ryanofsky's comments.

  62. sipa force-pushed on Nov 30, 2016
  63. sipa force-pushed on Dec 1, 2016
  64. in src/rpc/rawtransaction.cpp:None 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.

  65. in src/rpc/rawtransaction.cpp:None 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.

  66. in src/wallet/wallet.cpp:None 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.

  67. in src/wallet/wallet.h:None 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.

  68. in src/primitives/transaction.h:None 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.

  69. in src/primitives/transaction.h:None 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:

        s >> tx.vin;
    if (tx.vin.size() == 0 && fAllowWitness) {
        /* We read a dummy or an empty vin. */
        s >> flags;
        if (flags == 0)
            throw std::ios_base::failure("Invalid transaction flags");
        s >> tx.vin;
    }
    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.

  70. sipa force-pushed on Dec 1, 2016
  71. in src/wallet/wallet.cpp:None 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.

  72. ryanofsky approved
  73. ryanofsky commented at 9:50 PM on December 1, 2016: member

    utACK f006fcc1da680540def6c7d7d1f9b3d8aca8a1f8 with GetTransactionWeight fix.

  74. sipa force-pushed on Dec 1, 2016
  75. Switch GetTransaction to returning a CTransactionRef a1883536b4
  76. Make CWalletTx store a CTransactionRef instead of inheriting c3f5673a63
  77. Make DecodeHexTx return a CMutableTransaction 42fd8dee30
  78. sipa force-pushed on Dec 3, 2016
  79. Make CTransaction actually immutable 81e3228fcb
  80. gmaxwell commented at 10:05 PM on December 4, 2016: contributor

    Is there anything else this is waiting on?

  81. laanwj commented at 7:03 AM on December 5, 2016: member

    utACK 81e3228

  82. laanwj merged this on Dec 5, 2016
  83. laanwj closed this on Dec 5, 2016
  84. laanwj referenced this in commit 46904ee5d2 on Dec 5, 2016
  85. in src/primitives/transaction.h:None 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.

    Assertion 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

  86. zkbot referenced this in commit f9e39f293d on Jul 14, 2017
  87. codablock referenced this in commit 857c549931 on Jan 16, 2018
  88. codablock referenced this in commit 318215ac2b on Jan 16, 2018
  89. codablock referenced this in commit 6eed004da5 on Jan 16, 2018
  90. codablock referenced this in commit a260907693 on Jan 16, 2018
  91. codablock referenced this in commit 1e62969fa6 on Jan 17, 2018
  92. codablock referenced this in commit 525c049316 on Jan 17, 2018
  93. andvgal referenced this in commit 5521e80f37 on Jan 6, 2019
  94. andvgal referenced this in commit 2d2de12bad on Jan 6, 2019
  95. CryptoCentric referenced this in commit 4dfa918ff6 on Feb 25, 2019
  96. CryptoCentric referenced this in commit 4838d42ffe on Feb 25, 2019
  97. CryptoCentric referenced this in commit 8eba182f8d on Feb 25, 2019
  98. CryptoCentric referenced this in commit 93ec6e1a4c on Feb 26, 2019
  99. CryptoCentric referenced this in commit 989963b41f on Feb 26, 2019
  100. CryptoCentric referenced this in commit a2a6a6d54a on Feb 26, 2019
  101. CryptoCentric referenced this in commit f1b08b4910 on Feb 26, 2019
  102. CryptoCentric referenced this in commit 209db55a21 on Feb 26, 2019
  103. CryptoCentric referenced this in commit d3f4277a6e on Feb 26, 2019
  104. furszy referenced this in commit 8cd9c592a9 on May 29, 2020
  105. furszy referenced this in commit 7b2b9d048e on Sep 24, 2020
  106. furszy referenced this in commit 67b5bb6b8a on Feb 12, 2021
  107. MarcoFalke locked this on Sep 8, 2021
Contributors
sipalaanwjluke-jrdcousenspaveljanikgmaxwelltheuniTheBlueMattkazcwjtimonryanofskyNicolasDoriermorcos
Labels
Refactoring
Linked (view graph)
#8451 Get rid of the const field in CTransaction#8589 Inline CTxInWitness inside CTxIn#9039 Various serialization simplifcations and optimizations#9125 Make CBlock a vector of shared_ptr of CTransactions#9282 CMutableTransaction is defined as struct
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-04 00:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me