Page MenuHomePhabricator

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

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

Details

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 4293
Build 6651: Bitcoin ABC Buildbot (legacy)
Build 6650: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
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

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

Move deprecation and release not to later diff

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
src/wallet/rpcwallet.h
14 ↗(On Diff #6525)

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

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

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

This revision is now accepted and ready to land.Feb 12 2019, 23:46
This revision was automatically updated to reflect the committed changes.