Directly operate with CMutableTransaction #13359

1. 
   MarcoFalke commented at 3:08 pm on May 31, 2018: member

   When serializing a transaction for the wallet or rpc, it shouldn’t matter if the type is CTransaction or CMutableTransaction. (See the code for SerializeTransaction, which is a template on TxType)

   Thus, we can avoid a call to a CTransaction constructor by templating various helper methods similar to #13309 (Directly operate with CMutableTransaction in SignSignature)

2. MarcoFalke added the label Refactoring on May 31, 2018
3. MarcoFalke added the label RPC/REST/ZMQ on May 31, 2018
4. MarcoFalke added the label Wallet on May 31, 2018
5. DocOBITCOIN approved
6. 
   Empact commented at 12:28 pm on June 1, 2018: member

   How about using something like typename std::enable_if<std::is_same<T, CTransaction>::value || std::is_same<T, CMutableTransaction>::value>::type in place of static_assert? I haven’t tested this but I think it could be defined as an abstract transaction template type and then used to restrain the type of each such template.

   Example use here: https://github.com/bitcoin/bitcoin/pull/13076/files#diff-4a0cb5ad5e93d187a1b065a99bbae143

7. 
   promag commented at 10:45 pm on June 1, 2018: member

   Could remove the following in a separate commit?

   0int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts)
   

8. 
   ghost commented at 12:58 pm on June 3, 2018: none

   Is this mining bitcoin
9. 
   Empact commented at 10:13 am on June 5, 2018: member

   @pawan17kumar yes we are hard at work swinging pick-axes into keyboards.
10. 
    laanwj commented at 4:25 pm on June 5, 2018: member

    utACK faadf518e0e86a47fdab67884926f2f211f6fd3f

    Is this mining bitcoin

    if only mining was proof-of-open-source-development…

11. DrahtBot added the label Needs rebase on Jul 18, 2018
12. MarcoFalke force-pushed on Sep 6, 2018
13. MarcoFalke renamed this:
    wallet: Directly operate with CMutableTransaction
    Directly operate with CMutableTransaction
    on Sep 6, 2018
14. MarcoFalke removed the label Wallet on Sep 6, 2018
15. MarcoFalke force-pushed on Sep 6, 2018
16. DrahtBot removed the label Needs rebase on Sep 6, 2018
17. wallet: Directly operate with CMutableTransaction fa25e2ed94
18. MarcoFalke force-pushed on Sep 6, 2018
19. 
    DrahtBot commented at 6:51 pm on September 7, 2018: member

    • #14158 (fix various uses of inline by arvidn)
    • #13558 (Drop unused GetType() from CSizeComputer by Empact)
    • #11634 (wallet: Add missing cs_wallet/cs_KeyStore locks to wallet by practicalswift)

    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.

20. in src/consensus/validation.h:99 in fa25e2ed94

     92@@ -93,8 +93,10 @@ class CValidationState {
     93 // using only serialization with and without witness data. As witness_size
     94 // is equal to total_size - stripped_size, this formula is identical to:
     95 // weight = (stripped_size * 3) + total_size.
     96-static inline int64_t GetTransactionWeight(const CTransaction& tx)
     97+template <typename T>
     98+static inline int64_t GetTransactionWeight(const T& tx)
     99 {
    100+    static_assert(std::is_same<T, CTransaction>::value || std::is_same<T, CMutableTransaction>::value, "no need to allow other types");
    

    ---

    ---

    

    arvidn commented at 7:47 pm on September 7, 2018:

    it might be nicer to have this function not participate in overload resolution (i.e. enable_if) when these conditions are not met, rather than being triggering a static assert.

    ---

    

    ryanofsky commented at 8:54 pm on September 12, 2018:

    Downside of using enable_if is that the resulting compiler error message will probably be more ambiguous and harder to understand.

    ---

    

    lucash-dev commented at 10:21 pm on September 15, 2018:

    Either way, I think there should be a standard logic for checking whether a type is a transaction type – provided by transaction.h, thus preserving encapsulation.
21. in src/policy/rbf.cpp:8 in fa25e2ed94

    3@@ -4,15 +4,18 @@
    4 
    5 #include <policy/rbf.h>
    6 
    7-bool SignalsOptInRBF(const CTransaction &tx)
    8+template <typename T>
    9+bool SignalsOptInRBF(const T& tx)
    

    ---

    ---

    

    arvidn commented at 7:48 pm on September 7, 2018:

    this template should probably constrain T as well
22. DrahtBot added the label Needs rebase on Sep 11, 2018
23. 
    DrahtBot commented at 8:38 am on September 11, 2018: member
24. 
    ryanofsky commented at 8:55 pm on September 12, 2018: member

    utACK fa25e2ed9426994191c7a960f7d6aa3d4a9bf844
25. MarcoFalke closed this on Oct 1, 2018

    ---

26. MarcoFalke deleted the branch on Oct 1, 2018
27. 
    ryanofsky commented at 3:20 pm on October 19, 2018: member

    Why was this closed?
28. MarcoFalke added the label Up for grabs on Oct 20, 2018
29. laanwj removed the label Needs rebase on Oct 24, 2019
30. 
    MarcoFalke commented at 2:01 pm on November 11, 2019: member

    Why was this closed?

    It needs benchmarks, to justify the use of C++11, which is less common in this project (enable_if or static_assert)

31. MarcoFalke removed the label Up for grabs on Nov 11, 2019
32. MarcoFalke locked this on Dec 16, 2021

Contributors
MarcoFalke DocOBITCOIN Empact promag ghost laanwj DrahtBot arvidn ryanofsky lucash-dev

Labels
Refactoring RPC/REST/ZMQ