Page MenuHomePhabricator

Prepare functional tests for deprecation of signrawtransaction rpc
AbandonedPublic

Authored by Fabien on Dec 9 2018, 20:29.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Summary

The changes involved for splitting signrawtransaction into
signrawtransactionwithkey and signrawtransactionwithwallet will be split
to allow for easier review, and the deprecation of signrawtransaction
will lead to multiple test failures. This diff will avoid breaking the
tests, and will be reverted when these tests are converted to the new
signrawtransactionwith* function.

Test Plan
./test/functional/test_runner.py --extended

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR10579_fix_broken_tests
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4292
Build 6649: Bitcoin ABC Teamcity Staging
Build 6648: arc lint + arc unit

Event Timeline

Fabien created this revision.Dec 9 2018, 20:29
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 9 2018, 20:29
Herald added a subscriber: schancel. · View Herald Transcript
Fabien updated this revision to Diff 6313.Dec 9 2018, 20:55

Add missing deprecated arg on node restart

jasonbcox accepted this revision.Dec 20 2018, 20:16
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
test/functional/mempool_reorg.py
21

This syntax is very strange and I can't find a reference to it. I'm assuming this duplicates the inner list?

This revision is now accepted and ready to land.Dec 20 2018, 20:16
deadalnix requested changes to this revision.Thu, Jan 3, 22:14

I'm not a big fan of this approach because it is completely unclear why these test require this change and not others or why this change is required at all. This change is not self contained.

Is there a reason these test cannot be changed alongside the code that require them to be changed ?

This revision now requires changes to proceed.Thu, Jan 3, 22:14
Fabien added a comment.Fri, Jan 4, 18:14

signrawtransaction is being deprecated in D2007, and will break all these tests at once.

In order to avoid breaking the tests, here is the strategy:

  • This diff allow the use of the deprecated rpc
  • Then the backport (D2007) deprecates the signrawtransaction rpc, and set the ones. Tests are not broken thanks to the -deprecatedrpc option
  • The tests are fixed with the new RPC in D2008
  • This diff is reverted to remove the -deprecatedrpc option (actually part of D2008, will split pending)
  • A new test is added to check that the old api is still available through the -deprecatedrpc option, in D2009

Doing so avoid breaking the tests along the process.

Fabien requested review of this revision.Fri, Jan 4, 18:15
deadalnix requested changes to this revision.Sat, Jan 5, 17:09

You need to proceed as follow:

  1. Build the alternative to signrawtransaction
  2. Convert the tests to use the alternative
  3. Deprecate signrawtransaction, and add the flag to the tests that need it.
This revision now requires changes to proceed.Sat, Jan 5, 17:09
Fabien abandoned this revision.Sat, Jan 5, 17:20

Will update the workflow according to the review, this diff is no longer needed