Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
meshcollider merged 1 commit into from
Apr 27, 2019
Merged

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

meshcollider merged 1 commit into from
Apr 27, 2019

Conversation

@ariard
Copy link

@ariard ariard commented Apr 10, 2019

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

@jnewbery
Copy link
Contributor

jnewbery commented Apr 10, 2019

Concept ACK. Thanks @ariard!

@ariard ariard force-pushed the 2019-04-remove-chain-sign-tx-utility branch from a6d7634 to 442a65b Compare April 11, 2019 13:26
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 11, 2019 •
edited
Loading

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.

jnewbery
jnewbery reviewed Apr 11, 2019
View reviewed changes
src/rpc/rawtransaction_util.h Outdated
Copy link
Contributor

@jnewbery jnewbery Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

@ariard ariard Apr 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just forward declaration seems to do the job without gcc complaints

Copy link
Contributor

@jnewbery jnewbery Apr 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. You should pass by reference anyway, otherwise this will make a copy of the entire map.

@ariard ariard force-pushed the 2019-04-remove-chain-sign-tx-utility branch from 442a65b to b772cce Compare April 12, 2019 12:39
@jnewbery jnewbery mentioned this pull request Apr 12, 2019
Closed
promag
promag reviewed Apr 12, 2019
View reviewed changes
@ariard ariard force-pushed the 2019-04-remove-chain-sign-tx-utility branch from b772cce to 0f4b200 Compare April 13, 2019 13:16
@meshcollider
Copy link
Contributor

meshcollider commented Apr 14, 2019

Thanks, utACK 0f4b200

@ariard ariard force-pushed the 2019-04-remove-chain-sign-tx-utility branch from 0f4b200 to 70da741 Compare April 16, 2019 12:39
@ariard
Copy link
Author

ariard commented Apr 16, 2019

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

promag
promag reviewed Apr 16, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ariard restarted but also left some comments.

@ariard ariard force-pushed the 2019-04-remove-chain-sign-tx-utility branch from 70da741 to c98aeb0 Compare April 16, 2019 15:13
@jnewbery
Copy link
Contributor

jnewbery commented Apr 16, 2019

utACK c98aeb00c9eaeb12623dc8f6a644ee5f0044f3fa one you've removed that unnecessary interfaces::Chain forward declaration.

@ariard ariard force-pushed the 2019-04-remove-chain-sign-tx-utility branch from c98aeb0 to 99e88a3 Compare April 17, 2019 12:20
@ariard
Copy link
Author

ariard commented Apr 17, 2019

99e88a3 removed unnecessary interfaces::Chain forward declaration

@jnewbery
Copy link
Contributor

jnewbery commented Apr 17, 2019

utACK 99e88a3

@promag
Copy link
Contributor

promag commented Apr 17, 2019

utACK 99e88a3.

ryanofsky
ryanofsky approved these changes Apr 17, 2019
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 99e88a3

src/rpc/rawtransaction_util.h
class CBasicKeyStore;
class UniValue;
struct CMutableTransaction;
class Coin;
Copy link
Contributor

@ryanofsky ryanofsky Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@meshcollider meshcollider merged commit 99e88a3 into bitcoin:master Apr 27, 2019
meshcollider added a commit that referenced this pull request Apr 27, 2019
@meshcollider
Merge #15784: rpc: Remove dependency on interfaces::Chain in SignTran…
7034149 
…saction

99e88a3 rpc: Remove dependency on interfaces::Chain in SignTransaction (Antoine Riard)

Pull request description:

  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

Tree-SHA512: 42ee8fcbcd38643bbd82210db6f68249bed5ee036a4c930a1db534d0469a133e287b8869c977bf0cc79a7296dde04f72adb74d24e1cd20f4a280f4c2b7fceb74
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 27, 2019
@meshcollider @sidhujag
Merge bitcoin#15784: rpc: Remove dependency on interfaces::Chain in S…
cd95cb5 
…ignTransaction

99e88a3 rpc: Remove dependency on interfaces::Chain in SignTransaction (Antoine Riard)

Pull request description:

  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 bitcoin#15638

Tree-SHA512: 42ee8fcbcd38643bbd82210db6f68249bed5ee036a4c930a1db534d0469a133e287b8869c977bf0cc79a7296dde04f72adb74d24e1cd20f4a280f4c2b7fceb74
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 27, 2020
@majcosta
[backport#15784] rpc: Remove dependency on interfaces::Chain in SignT…
6a41e82 
…ransaction

Summary:
Comment SignTransaction utility

bitcoin/bitcoin@99e88a3

---

Depends on D6257

Backport of Core [[bitcoin/bitcoin#15784 | PR15784]]

Test Plan:
  ninja check check-functional

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6258
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+2 more reviewers

@jnewbery jnewbery jnewbery left review comments

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ariard @jnewbery @DrahtBot @meshcollider @promag @ryanofsky @fanquake