Details

Reviewers

jasonbcox
deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING5180ae42d752: [Part 1 of 3] Split signrawtransaction into wallet and non-wallet
rABC5180ae42d752: [Part 1 of 3] Split signrawtransaction into wallet and non-wallet

Summary

Splits signrwatransaction into a wallet version (signrawtransactionwithwallet) and
non-wallet version (signrawtransactionwithkey). signrawtransaction will call the right signrawtransaction* command as per the parameters in order to
maintain compatibility.

Updated signrawtransactions test to use new RPCs

Backport 1/3 of core PR10579 (partial backport of commit 1e79c05)

Note: in order to not break the tests, the signrawtransaction deprecation will be backported in a later diff, once all the tests are updated to the new RPC.

Part of T438

Test Plan

make check
./test/functional/test_runner.py signrawtransactions txn_clone abc-high_prioriy_transaction

Diff Detail

Repository

rABC Bitcoin ABC

Branch

PR10579_base

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4289
Build 6643: Bitcoin ABC Buildbot (legacy)
Build 6642: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

deadalnix added inline comments.Dec 20 2018, 13:02

src/rpc/rawtransaction.cpp
1389 ↗	(On Diff #6321)	Cnsidering there are several spaces here, I fail to see why everythibg has to be shifted right by so many spaces.
src/wallet/rpcwallet.cpp
3521 ↗	(On Diff #6321)	braces
3789 ↗	(On Diff #6321)	dito, it seems that there are more spaces than required in there
src/wallet/rpcwallet.h
14 ↗	(On Diff #6321)	Remove

This revision now requires changes to proceed.Dec 20 2018, 13:02

Fabien added inline comments.Dec 21 2018, 14:39

src/rpc/rawtransaction.cpp
1119 ↗	(On Diff #6321)	This is a segwit related flag which tells whether witness data is serialized in the tx or not
src/wallet/rpcwallet.h
14 ↗	(On Diff #6321)	This is needed for `signrawtransactionwithwallet` (return type)

Rebase and address comments

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4341: Diff 6391.Dec 21 2018, 14:47

Rebase on top of D2214

Fabien edited the summary of this revision. (Show Details)Dec 21 2018, 16:04

Fabien added a parent revision: D2214: More user-friendly error message when partially signing.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4344: Diff 6394.Dec 21 2018, 16:12

deadalnix requested changes to this revision.Dec 23 2018, 15:33

deadalnix added inline comments.

src/rpc/rawtransaction.cpp
1248 ↗	(On Diff #6394)	Same as on the other diff, what's the roadmap for that ? Is someone making sure this will be removed in 0.19 ?

This revision now requires changes to proceed.Dec 23 2018, 15:33

Fabien mentioned this in D2184: Prepare functional tests for deprecation of signrawtransaction rpc.Jan 4 2019, 18:14

Move deprecation and release not to later diff

Fabien edited the summary of this revision. (Show Details)Jan 6 2019, 12:06

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4424: Diff 6525.Jan 6 2019, 13:41

Reviewing these long diff is a very long and tedious process and it's been no several round where the hange are requested due to lack of attention to detail rather than any actual issue with the code. Please have a pass over it, get it all sorted out so we can land this and move on.

src/rpc/rawtransaction.cpp
1161 ↗	(On Diff #6525)	revert
1372 ↗	(On Diff #6525)	The formating is all screwed up.
src/wallet/rpcwallet.cpp
3791 ↗	(On Diff #6525)	There are 2 spaces instead of 1
src/wallet/rpcwallet.h
14 ↗	(On Diff #6525)	Remove

This revision now requires changes to proceed.Jan 7 2019, 00:37

Fabien added inline comments.Jan 7 2019, 09:43

src/wallet/rpcwallet.h
14 ↗	(On Diff #6525)	It won't build without, this is the return type of `signrawtransactionwithwallet`

Fix nits

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Fix 1 more nit

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B4432: Diff 6534.Jan 7 2019, 09:55

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4433: Diff 6535.Jan 7 2019, 09:59

deadalnix accepted this revision.Jan 8 2019, 16:43

Rebase

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4476: Diff 6598.Jan 11 2019, 10:57

Please rebase and then bug me if I don't get to reviewing this again in the next few days.

@Fabien are these still on the table for 0.19?

Rebase

@markblundeberg Yes they are

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4954: Diff 7294.Feb 11 2019, 12:17

Fabien removed parent revisions: D2214: More user-friendly error message when partially signing, D2184: Prepare functional tests for deprecation of signrawtransaction rpc, D1967: Move transaction combining from signrawtransaction to new RPC.Feb 11 2019, 12:23

jasonbcox requested changes to this revision.Feb 11 2019, 22:03

jasonbcox added inline comments.

src/rpc/rawtransaction.cpp
1000 ↗	(On Diff #7294)	The above transactions won't be valid on the BCH chain. Is there any reason to include these in the help message? I don't think we want to encourage people to do this. If there is some reasoning for allowing non-FORKID txs, there needs to be tests for this.
1080 ↗	(On Diff #7294)	Missing `DEPRECATED` note in the message
1128 ↗	(On Diff #7294)	ditto
src/wallet/rpcwallet.cpp
3663 ↗	(On Diff #7294)	ditto

This revision now requires changes to proceed.Feb 11 2019, 22:03

Fabien planned changes to this revision.Feb 12 2019, 07:33

Fabien added inline comments.

src/rpc/rawtransaction.cpp
1000 ↗	(On Diff #7294)	Good point. The RPC will return an error if the FORKID flag is not set (see line 882). But this raises the problem of the test coverage, so far there seems to be no test for this behavior => added on my todo list. Note: this is already the case in the current codebase, and not introduced in this diff. Regarding the help, intended that there is this error, I think its OK to just remove the invalid combinations from the help. I will update the comments. If FORKID is made non mandatory in the future they can be re-added.
1080 ↗	(On Diff #7294)	Deprecation is done in D2009