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

  2. fanquake added the label Refactoring on Apr 10, 2019
  3. fanquake added the label RPC/REST/ZMQ on Apr 10, 2019
  4. fanquake added the label Needs rebase on Apr 10, 2019
  5. jnewbery commented at 2:02 pm on April 10, 2019: member
    Concept ACK. Thanks @ariard!
  6. ariard force-pushed on Apr 11, 2019
  7. DrahtBot removed the label Needs rebase on Apr 11, 2019
  8. 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.

  9. 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.
  10. ariard force-pushed on Apr 12, 2019
  11. 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
  12. ariard force-pushed on Apr 13, 2019
  14. ariard force-pushed on Apr 16, 2019
  15. 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.”
  16. 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.
  17. 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.
  18. 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.
  19. promag commented at 1:43 pm on April 16, 2019: member
    @ariard restarted but also left some comments.
  20. ariard force-pushed on Apr 16, 2019
  21. jnewbery commented at 3:39 pm on April 16, 2019: member
    utACK c98aeb00c9eaeb12623dc8f6a644ee5f0044f3fa one you’ve removed that unnecessary interfaces::Chain forward declaration.
  22. rpc: Remove dependency on interfaces::Chain in SignTransaction 
    Comment SignTransaction utility
    99e88a3726
  23. ariard force-pushed on Apr 17, 2019
  24. ariard commented at 12:23 pm on April 17, 2019: member
    99e88a3 removed unnecessary interfaces::Chain forward declaration
  25. jnewbery commented at 12:52 pm on April 17, 2019: member
    utACK 99e88a3726c2325e3a3a35c0a750bde25bd58ad0
  26. promag commented at 1:11 pm on April 17, 2019: member
    utACK 99e88a3.
  27. 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)
  28. ryanofsky approved
  29. ryanofsky commented at 5:00 pm on April 17, 2019: member
    utACK 99e88a3726c2325e3a3a35c0a750bde25bd58ad0
  30. meshcollider merged this on Apr 27, 2019
  31. meshcollider closed this on Apr 27, 2019
  32. meshcollider referenced this in commit 703414994a on Apr 27, 2019
  33. sidhujag referenced this in commit cd95cb552d on Apr 27, 2019
  34. deadalnix referenced this in commit 6a41e8229c on May 27, 2020
  35. vijaydasmp referenced this in commit 868efabd18 on Dec 7, 2021
  36. vijaydasmp referenced this in commit e17c086876 on Dec 11, 2021
  37. vijaydasmp referenced this in commit 6c650b3e48 on Dec 13, 2021
  38. vijaydasmp referenced this in commit 71edeac9d3 on Dec 14, 2021
  39. DrahtBot locked this on Dec 16, 2021


ariard jnewbery DrahtBot promag meshcollider ryanofsky

Labels
Refactoring RPC/REST/ZMQ

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: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me