Directly operate with CMutableTransaction #13359

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1806-avoidTxConstuctor changing 9 files +33 −16

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)
MarcoFalke added the label Refactoring on May 31, 2018
MarcoFalke added the label RPC/REST/ZMQ on May 31, 2018
MarcoFalke added the label Wallet on May 31, 2018
DocOBITCOIN approved
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

promag commented at 10:45 PM on June 1, 2018: member

Could remove the following in a separate commit?

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

ghost commented at 12:58 PM on June 3, 2018: none

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

@pawan17kumar yes we are hard at work swinging pick-axes into keyboards.
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...
DrahtBot added the label Needs rebase on Jul 18, 2018
MarcoFalke force-pushed on Sep 6, 2018
MarcoFalke renamed this:
~~wallet: Directly operate with CMutableTransaction~~
Directly operate with CMutableTransaction
on Sep 6, 2018
MarcoFalke removed the label Wallet on Sep 6, 2018
MarcoFalke force-pushed on Sep 6, 2018
DrahtBot removed the label Needs rebase on Sep 6, 2018
wallet: Directly operate with CMutableTransaction fa25e2ed94
MarcoFalke force-pushed on Sep 6, 2018
DrahtBot commented at 6:51 PM on September 7, 2018: member
Note to reviewers: This pull request conflicts with the following ones:
- #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.
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.

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

DrahtBot added the label Needs rebase on Sep 11, 2018
DrahtBot commented at 8:38 AM on September 11, 2018: member

Needs rebase
ryanofsky commented at 8:55 PM on September 12, 2018: member

utACK fa25e2ed9426994191c7a960f7d6aa3d4a9bf844
MarcoFalke closed this on Oct 1, 2018
MarcoFalke deleted the branch on Oct 1, 2018
ryanofsky commented at 3:20 PM on October 19, 2018: member

Why was this closed?
MarcoFalke added the label Up for grabs on Oct 20, 2018
laanwj removed the label Needs rebase on Oct 24, 2019
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)
MarcoFalke removed the label Up for grabs on Nov 11, 2019
MarcoFalke locked this on Dec 16, 2021

Contributors

Labels

Refactoring RPC/REST/ZMQ

Linked (view graph)

#13309 Directly operate with CMutableTransaction in SignSignature #15525 Brainstorming: CTransaction vs CMutableTransaction