Page MenuHomePhabricator

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

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 is marked as DEPRECATED
and 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

Depends on D1967 and D2184
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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 4293
Build 6651: Bitcoin ABC Teamcity Staging
Build 6650: arc lint + arc unit

Event Timeline

Fabien created this revision.Nov 6 2018, 11:59
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 6 2018, 11:59
Fabien updated this revision to Diff 5663.Nov 6 2018, 12:10

Fix misplaced comments and add an entry to the release notes

Fabien updated this revision to Diff 5664.Nov 6 2018, 12:28

Fix some more nits

Fabien updated this revision to Diff 5665.Nov 6 2018, 12:32

Fix comment case issues

jasonbcox requested changes to this revision.Nov 9 2018, 19:14
jasonbcox added a subscriber: jasonbcox.

Needs a rebase as well.

doc/release-notes.md
20 ↗(On Diff #5665)

I think you can remove the "This is a backport from bitcoin core." comment, as it's not entirely relevant to the release.

src/rpc/rawtransaction.cpp
1246 ↗(On Diff #5665)

Change to v0.19

1247 ↗(On Diff #5665)

v0.18

1250 ↗(On Diff #5665)

v0.19

src/wallet/rpcwallet.cpp
3625 ↗(On Diff #5665)

Does this work? it looks like it's putting a double lock on cs_main.

This revision now requires changes to proceed.Nov 9 2018, 19:14
Fabien marked 3 inline comments as done.Nov 9 2018, 21:16
Fabien added inline comments.
doc/release-notes.md
20 ↗(On Diff #5665)

I will remove it (and rebase)

src/rpc/rawtransaction.cpp
1246 ↗(On Diff #5665)

I don't know when minor version number increase ? I suspected 0.19 could be the next release, and so removing it in 0.19 would have been too early. I also may introduce micro version (0.18.x) if this does match the version numbering.

src/wallet/rpcwallet.cpp
3625 ↗(On Diff #5665)

Yes, it locks cs_main twice (it's not exactly the same scope however). As long as it is the same thread it should not be a problem. I will take a closer look to check what needs to be protected in SignTransaction and verify that a cs_main lock is not missing.

Fabien updated this revision to Diff 5780.Mon, Nov 12, 22:48

Rebase and fix as per review

schancel edited the summary of this revision. (Show Details)Wed, Nov 28, 20:58
schancel retitled this revision from Split signrawtransaction into wallet and non-wallet to [Part 1 of 3] Split signrawtransaction into wallet and non-wallet.
deadalnix requested changes to this revision.Sun, Dec 2, 23:42
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
doc/release-notes.md
5 ↗(On Diff #5780)

This clearly needs rebasing.

This revision now requires changes to proceed.Sun, Dec 2, 23:42
Fabien updated this revision to Diff 6264.Mon, Dec 3, 17:50

Rebase

deadalnix requested changes to this revision.Sat, Dec 8, 13:10

Back on your queue, this is apparently breaking many tests.

This revision now requires changes to proceed.Sat, Dec 8, 13:10
Fabien updated this revision to Diff 6310.Sun, Dec 9, 20:31

Rebase on top of D2184, avoid breaking tests

Fabien updated this revision to Diff 6314.Sun, Dec 9, 20:56

Rebase