Page MenuHomePhabricator

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

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

Details

Reviewers
jasonbcox
deadalnix
Group Reviewers
Restricted Project
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
arcpatch-D2007
Lint
Lint OK
Unit
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
doc/release-notes.md
18 ↗(On Diff #6321)

You need to rebase this.

src/rpc/rawtransaction.cpp
29 ↗(On Diff #6321)

Remove

985 ↗(On Diff #6321)

remove

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)

braces

1119 ↗(On Diff #6321)

There is a parameter missing here. Why the difference ?

1150 ↗(On Diff #6321)

braces

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.

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)

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

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

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

Rebase

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

Rebase

jasonbcox requested changes to this revision.Mon, Feb 11, 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.Mon, Feb 11, 22:03
Fabien planned changes to this revision.Tue, Feb 12, 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

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