Page MenuHomePhabricator

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

Authored by Fabien on Nov 6 2018, 11:59.


Group Reviewers
Restricted Project

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/ signrawtransactions txn_clone abc-high_prioriy_transaction

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage
Build Status
Buildable 4964
Build 7991: Bitcoin ABC Teamcity Staging
Build 7990: 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
18 ↗(On Diff #6321)

You need to rebase this.

29 ↗(On Diff #6321)


985 ↗(On Diff #6321)


1011 ↗(On Diff #6321)

This is not part of the original diff. Finding the origin of that code backporting it apropriately would be better than sticking it into this backport.

1030 ↗(On Diff #6321)


1119 ↗(On Diff #6321)

There is a parameter missing here. Why the difference ?

1150 ↗(On Diff #6321)


1245 ↗(On Diff #6321)

Is that on our roadmap ?

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.

3521 ↗(On Diff #6321)


3789 ↗(On Diff #6321)

dito, it seems that there are more spaces than required in there

14 ↗(On Diff #6321)


This revision now requires changes to proceed.Dec 20 2018, 13:02
Fabien added inline comments.Dec 21 2018, 14:39
1119 ↗(On Diff #6321)

This is a segwit related flag which tells whether witness data is serialized in the tx or not

14 ↗(On Diff #6321)

This is needed for signrawtransactionwithwallet (return type)

Fabien updated this revision to Diff 6391.Dec 21 2018, 14:39

Rebase and address comments

Fabien updated this revision to Diff 6394.Dec 21 2018, 16:02

Rebase on top of D2214

deadalnix requested changes to this revision.Dec 23 2018, 15:33
deadalnix added inline comments.
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 updated this revision to Diff 6525.Jan 6 2019, 12:04

Move deprecation and release not to later diff

Fabien edited the summary of this revision. (Show Details)Jan 6 2019, 12:06
deadalnix requested changes to this revision.Jan 7 2019, 00:37

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.

1161 ↗(On Diff #6525)


1372 ↗(On Diff #6525)

The formating is all screwed up.

3791 ↗(On Diff #6525)

There are 2 spaces instead of 1

14 ↗(On Diff #6525)


This revision now requires changes to proceed.Jan 7 2019, 00:37
Fabien added inline comments.Jan 7 2019, 09:43
14 ↗(On Diff #6525)

It won't build without, this is the return type of signrawtransactionwithwallet

Fabien updated this revision to Diff 6534.Jan 7 2019, 09:44

Fix nits

Fabien updated this revision to Diff 6535.Jan 7 2019, 09:48

Fix 1 more nit

deadalnix accepted this revision.Jan 8 2019, 16:43
Fabien updated this revision to Diff 6598.Jan 11 2019, 10:46


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?

Fabien updated this revision to Diff 7294.Mon, Feb 11, 12:02


jasonbcox requested changes to this revision.Mon, Feb 11, 22:03
jasonbcox added inline comments.
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)


3663 ↗(On Diff #7294)


This revision now requires changes to proceed.Mon, Feb 11, 22:03
Fabien planned changes to this revision.Tue, Feb 12, 07:33
Fabien added inline comments.
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

jasonbcox accepted this revision.Tue, Feb 12, 23:46
This revision is now accepted and ready to land.Tue, Feb 12, 23:46