rpc: Remove dependency on interfaces::Chain in SignTransaction #15784

pull ariard wants to merge 1 commits into bitcoin:master from ariard:2019-04-remove-chain-sign-tx-utility changing 4 files +34 −19

ariard commented at 1:51 PM on April 10, 2019: member

Assuming wallet RPCs and node RPCs will go into different processes, signrawtransactionwithkey doesn't need to access Coins via interfaces::Chain, it may use directly utility in node/coins.cpp

Obviously will need rebase after #15638
fanquake added the label Refactoring on Apr 10, 2019
fanquake added the label RPC/REST/ZMQ on Apr 10, 2019
fanquake added the label Needs rebase on Apr 10, 2019
jnewbery commented at 2:02 PM on April 10, 2019: member

Concept ACK. Thanks @ariard!
ariard force-pushed on Apr 11, 2019
DrahtBot removed the label Needs rebase on Apr 11, 2019
DrahtBot commented at 6:01 PM on April 11, 2019: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #15810 ([WIP] Remove nAbsurdFee fee from AcceptToMemoryPool by jnewbery)
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/rpc/rawtransaction_util.h:8 in 442a65b0bb outdated
```
   4 | @@ -5,6 +5,11 @@
   5 |  #ifndef BITCOIN_RPC_RAWTRANSACTION_UTIL_H
   6 |  #define BITCOIN_RPC_RAWTRANSACTION_UTIL_H
   7 |  
   8 | +#include <coins.h>
```
jnewbery commented at 6:25 PM on April 11, 2019:

You should just be able to forward declare the Coin object and leave this include in the .cpp file if you change the coins argument into a reference.

(thanks to @ryanofsky for answering my question about this)

ariard commented at 12:41 PM on April 12, 2019:

Just forward declaration seems to do the job without gcc complaints

jnewbery commented at 9:05 PM on April 12, 2019:

Thanks. You should pass by reference anyway, otherwise this will make a copy of the entire map.
ariard force-pushed on Apr 12, 2019

in src/rpc/rawtransaction_util.cpp:152 in b772cce787 outdated

 148 | @@ -149,18 +149,8 @@ static void TxInErrorToJSON(const CTxIn& txin, UniValue& vErrorsRet, const std::
 149 |      vErrorsRet.push_back(entry);
 150 |  }
 151 |  
 152 | -// TODO(https://github.com/bitcoin/bitcoin/pull/10973#discussion_r267084237):
 153 | -// The dependency on interfaces::Chain should be removed, so
 154 | -// signrawtransactionwithkey doesn't need access to a Chain instance.
 155 | -UniValue SignTransaction(interfaces::Chain& chain, CMutableTransaction& mtx, const UniValue& prevTxsUnival, CBasicKeyStore *keystore, bool is_temp_keystore, const UniValue& hashType)
 156 | +UniValue SignTransaction(CMutableTransaction& mtx, const UniValue& prevTxsUnival, CBasicKeyStore* keystore, std::map<COutPoint, Coin> coins, bool is_temp_keystore, const UniValue& hashType)

promag commented at 10:12 PM on April 12, 2019:

Like @jnewbery said, could be const std::map<COutPoint, Coin>& coins).

ariard commented at 1:18 PM on April 13, 2019:

Updated to pass-by-reference but I think can't be const due to coins[out] = std::move(newcoin) L198 ?

jnewbery commented at 4:50 PM on April 13, 2019:

I think you're right that it can't be const

promag commented at 10:22 AM on April 14, 2019:

Then I think it should not pass by reference or you could comment that coins is modified after the call.

ariard commented at 12:40 PM on April 16, 2019:

@promag followed your suggestion and also took the opportunity to add comments for all SignTransaction params

ariard force-pushed on Apr 13, 2019
meshcollider commented at 4:14 AM on April 14, 2019: contributor

Thanks, utACK https://github.com/bitcoin/bitcoin/pull/15784/commits/0f4b200443092422e6a924a76c848f92df1db534
ariard force-pushed on Apr 16, 2019
ariard commented at 1:34 PM on April 16, 2019: member

Got this on travis failure : "Please manually re-run this job by using the travis restart button or asking a bitcoin maintainer to restart. The next run should not time out because the build cache has been saved."

in src/rpc/rawtransaction.cpp:20 in 70da7418e4 outdated

  16 | @@ -17,6 +17,7 @@
  17 |  #include <merkleblock.h>
  18 |  #include <node/psbt.h>
  19 |  #include <node/transaction.h>
  20 | +#include <node/coin.h>

promag commented at 1:38 PM on April 16, 2019:

nit, could sorted.

in src/rpc/rawtransaction_util.h:29 in 70da7418e4 outdated

  26 | + * @param  prevTxs       Array of previous txns outputs that tx depends on but may not yet be in the block chain
  27 | + * @param  keystore      Temporary keystore containing signing keys
  28 | + * @param  coins         Map of unspent outputs - coins in mempool and current chain UTXO set, may be extended by previous txns outputs after call
  29 | + * @param  tempKeystore  Whether to use temporary keystore
  30 | + * @param  hashType      The signature hash type
  31 | + * return signed tx

promag commented at 1:38 PM on April 16, 2019:

@returns JSON object with details of the signed transaction or something along that.

in src/rpc/rawtransaction_util.h:16 in 70da7418e4 outdated
```
  11 |  class UniValue;
  12 |  struct CMutableTransaction;
  13 | +class Coin;
  14 | +class COutPoint;
  15 |  
  16 |  namespace interfaces {
```
promag commented at 1:42 PM on April 16, 2019:

Remove.

ariard commented at 3:14 PM on April 16, 2019:

Seems I need to forward declare both Coin and COutPoint objects there ?

promag commented at 3:23 PM on April 16, 2019:

I mean you can remove the interfaces namespace.

jnewbery commented at 3:36 PM on April 16, 2019:

promag is saying you can remove the forward declaration of the interfaces::Chain class.
promag commented at 1:43 PM on April 16, 2019: member

@ariard restarted but also left some comments.
ariard force-pushed on Apr 16, 2019
jnewbery commented at 3:39 PM on April 16, 2019: member

utACK c98aeb00c9eaeb12623dc8f6a644ee5f0044f3fa one you've removed that unnecessary interfaces::Chain forward declaration.
rpc: Remove dependency on interfaces::Chain in SignTransaction
Comment SignTransaction utility
99e88a3726
ariard force-pushed on Apr 17, 2019
ariard commented at 12:23 PM on April 17, 2019: member

99e88a3 removed unnecessary interfaces::Chain forward declaration
jnewbery commented at 12:52 PM on April 17, 2019: member

utACK 99e88a3726c2325e3a3a35c0a750bde25bd58ad0
promag commented at 1:11 PM on April 17, 2019: member

utACK 99e88a3.

in src/rpc/rawtransaction_util.h:13 in 99e88a3726

   4 | @@ -5,16 +5,26 @@
   5 |  #ifndef BITCOIN_RPC_RAWTRANSACTION_UTIL_H
   6 |  #define BITCOIN_RPC_RAWTRANSACTION_UTIL_H
   7 |  
   8 | +#include <map>
   9 | +
  10 |  class CBasicKeyStore;
  11 |  class UniValue;
  12 |  struct CMutableTransaction;
  13 | +class Coin;

ryanofsky commented at 4:55 PM on April 17, 2019:

Could sort these between CBasicKeyStore and UniValue above (I normally just pipe these lines to !sort in vim)

ryanofsky approved
ryanofsky commented at 5:00 PM on April 17, 2019: member

utACK 99e88a3726c2325e3a3a35c0a750bde25bd58ad0
meshcollider merged this on Apr 27, 2019
meshcollider closed this on Apr 27, 2019
meshcollider referenced this in commit 703414994a on Apr 27, 2019
sidhujag referenced this in commit cd95cb552d on Apr 27, 2019
deadalnix referenced this in commit 6a41e8229c on May 27, 2020
vijaydasmp referenced this in commit 868efabd18 on Dec 7, 2021
vijaydasmp referenced this in commit e17c086876 on Dec 11, 2021
vijaydasmp referenced this in commit 6c650b3e48 on Dec 13, 2021
vijaydasmp referenced this in commit 71edeac9d3 on Dec 14, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Refactoring RPC/REST/ZMQ

Linked (view graph)

#15638 Move-only: Pull wallet code out of libbitcoin_server #15810 [WIP] mempool: Remove nAbsurdFee fee from AcceptToMemoryPool

rpc: Remove dependency on interfaces::Chain in SignTransaction #15784

Conflicts