Page MenuHomePhabricator

Merge #9894: remove 'label' filter for rpc command help
ClosedPublic

Authored by nakihito on Jun 13 2019, 00:03.

Details

Summary

6665977 remove 'label' filter for rpc command help (Gregory Sanders)

Tree-SHA512: 0676c55b2893a469cd6785963affbb04126b9a32c130f1bb22dfd233ede6998f695187264e897ced4e0dac48451d9ae0311ebb4f7442096cad632fd22f75080e

Backport of Core PR9894
https://github.com/bitcoin/bitcoin/pull/9894/files

Test Plan
make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR9894
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7799
Build 13629: Bitcoin ABC Buildbot (legacy)
Build 13628: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Jun 13 2019, 00:03
This revision is now accepted and ready to land.Jun 13 2019, 01:02

I tried backporting this one and I noticed that it produced duplicate entries for commands with "label" in the name, and I couldn't figure out why.... Is that still the case now? Can "examine bitcoin-cli help" be added to test plan?

nakihito planned changes to this revision.EditedJun 13 2019, 01:20

I tried backporting this one and I noticed that it produced duplicate entries for commands with "label" in the name, and I couldn't figure out why.... Is that still the case now? Can "examine bitcoin-cli help" be added to test plan?

If this was happening to you, then it seems worth checking for. Will update test plan with this after I give it a shot and then re-up for review .

Edit: I checked the output of bitcoin-cli help and did indeed see repeating commands. This looked weird to me, so after looking into the code a little deeper, I think there is a bug in the way the RPC commands are being registered to tableRPC.

The duplicate commands seen in bitcoin-cli helpare the following wallet RPC commands:

getlabeladdress
getreceivedbylabel
listreceivedbylabel
setlabel

For each of the above commands, there are two separate RPCs within rpc/wallet.cpp,'s ContextFreeRPCCommand (around line 4183) that share the same actor (function). I am currently looking into why this is happening.

jasonbcox requested changes to this revision.Jun 13 2019, 20:24

Investigate the duplicate RPCs in the help message. It sounds to me like some of the accounts code as either not deprecated correctly or needs to be cleaned up before this is backported.

What I recall is that the new label commands that get added are sorted in correctly, and it's the existing label entries that are wrong. So we do want this Diff, and indeed some old code isn't cleaned up right.

This was the state of Core when this backport was landed: https://github.com/instagibbs/bitcoin/blob/666597798c07d9df06681be63d2b2f51f0d5ca8a/src/wallet/rpcwallet.cpp
The important point to note is the array on line 2997. The difference from our code right now comes from an out of order backport here: https://reviews.bitcoinabc.org/D1986. The original PR for D1986 was landed in Core on March 22, 2018. The PR for this patch was originally landed in Core on March 1, 2017.
Core does not have getlabeladdress, getreceivedbylabel, listreceivedbylabel, setlabel. Reading some of the discussion related to this PR (https://github.com/bitcoin/bitcoin/pull/9894, https://github.com/bitcoin/bitcoin/pull/11536, and https://github.com/bitcoin/bitcoin/pull/12892), it appears the RPCs went from label to account in Core:

luke-jr commented on Mar 2, 2017
For historical reference: IIRC, the RPCs were originally label, and had been replaced by the account variants.

From what I can tell, the extra RPCs should not appear when calling ./bitcoin-cli help, but that is more because the extra account RPCs didn't exist in Core's code at the time. It also appears D1986 was one of the first steps to deprecating the account RPCs. https://github.com/bitcoin/bitcoin/pull/12953/files and https://github.com/bitcoin/bitcoin/pull/12892/files look to finish it and should fix the issue we are currently facing with this backport.

As mentioned in the meeting I'm happy to green this if the duplicates are going to be removed reasonably soon in some other diffs, even if landed later on.

As mentioned in the meeting I'm happy to green this if the duplicates are going to be removed reasonably soon in some other diffs, even if landed later on.

Thanks. I'm gonna start working on the other two PRs so that they can be landed quickly after each other. I'll put this up for review again after I get those two up.

jasonbcox requested changes to this revision.Sep 4 2019, 18:31

I will green this once D3950 is getting close to completion. Right now, there are some missing backports so it's difficult to estimate a timeline.

This revision now requires changes to proceed.Sep 4 2019, 18:31
jasonbcox requested changes to this revision.Oct 1 2019, 19:01

Putting this off my queue until more work is done on D3950's dependencies.

This revision now requires changes to proceed.Oct 1 2019, 19:01

Looks like the stack is clearing up. Please don't land this until D3950 is green.

This revision is now accepted and ready to land.Oct 21 2019, 16:10