Aligning RPC API with Bitcoin Core conventions

[JeremyRand]

The Namecoin Core RPC API for name operations has a bunch of inconsistencies with what Bitcoin Core does. Some of these inconsistencies are due to Vince doing weird things, others are due to Namecoin Core adding some features before Bitcoin Core did so. Other parts of the API are confusing for historical reasons (though not specifically in conflict with Bitcoin Core).

Some notable examples of API issues:

• "Method naming: use consecutive lower-case names such as getrawtransaction and submitblock."
  • name_checkdb, name_firstupdate, name_history, name_list, name_new, name_pending, name_scan, name_show, and name_update don't follow this.
• Method naming: Bitcoin Core RPC methods tend to be named in imperative form, e.g. abandontransaction.
  • I don't see any formal guideline in Bitcoin Core's developer notes for this, maybe we should ask them to document it?
  • The name_ methods don't follow this.
  • Suggested new names: registername (registration and pre-registration are more intuitive than firstupdate and new), getnamehistory (consistent with getmempoolancestors), listnames (consistent with listtransactions), preregistername (ibid), getmempoolname (consistent with getmempoolentry), scannameset (consistent with scantxoutset), getname (consistent with getrawtransaction/gettransaction), and updatename.
• "Argument and field naming: ... use snake case fee_delta (and not, e.g. feedelta or camel case feeDelta)."
  • allowExisting, allowExpired, byHash, destAddress, minConf, maxConf, nameEncoding, sendCoins, and valueEncoding don't follow this.
• Bitcoin Core supports name-based arguments now.
  • options does the same thing, but worse.
• "Try to make the RPC response a JSON object."
  • name_checkdb, name_firstupdate, name_history, name_list, name_new, name_pending, name_scan, name_update, and queuerawtransaction don't follow this.
• rand argument
  • Should be renamed to salt, since this value is typically not random anymore.
• allowExisting vs allow_active
  • allow_active is probably more precise.
• tx argument
  • Should be renamed to txid, since tx could be confused with rawtx

Would there be any objections to updating the API to fix these issues (leaving the existing API methods in place to avoid disrupting consumers of the current API)?

[domob1812]

My main objection would be added complexity if we keep the old names around and just "add" new ones.

[JeremyRand]

My main objection would be added complexity if we keep the old names around and just "add" new ones.

@domob1812 I was figuring we could add the new methods, mark the old ones as deprecated, then a release or two later we could mark the old ones as invisible (I think Bitcoin Core supports invisible RPC methods that don't show up in help?), and then after another 1-2 releases we could remove the old methods.

If you strongly prefer a faster schedule to remove the old methods, I might be amenable to that.

[domob1812]

No, I definitely do not prefer to remove the old methods quickly, because that will break everyone out there. And I also don't like having the methods duplicated, as that will be quite a bit of extra complexity in the code (not specifically "complex" code, but just bloat that needs to be updated whenever upstream Bitcoin updates the RPC doc code again and all that).

To be honest, my impression is that either way is "bad" and not worth it just to change the naming convention. I'd prefer to keep the names and arguments as they are.

[JeremyRand]

So, I'm not sure if this has been reaching you, but we're getting a lot of unhappy support requests from users who find the current RPC API to be confusing / bad UX due to its dissimilarity to Bitcoin Core. Some of these complaints are from funders. So I think it is worth fixing.

For the specific concern about having duplicate methods, I can think of two workarounds:

1. Have the old method function simply act as a shim that calls the new method function.
2. I believe Bitcoin Core's RPC system supports method aliases, i.e. the same function has two names, and only one of them shows up in the help.

Option 2 seems like it would be better for avoiding the bloat that needs to be kept in sync with upstream. I've never actually tried to use method aliases, but I don't mind looking into how it works.

Thoughts?

[domob1812]

I am not an expert about Bitcoin's RPC code, either - but if method aliases work that way, then I have no objection to adding them. However, another concern would then be the rename of method arguments, which entails with it the full RPC doc code (which is self-verifying and thus needs to be set correctly or it will break).

I assumed that we would want to leave the old method as-is, and add the new method with changed argument names, too. And in that case, we could use neither option 1 nor 2, as we would (at the least) have to duplicate the RPC doc logic.

[JeremyRand]

@domob1812 Looking at this from another angle -- how often do these breaking changes from upstream affect us? I'm looking at the Namecoin Core Git history for commits that affect src/rpc/names.cpp, and I see the following hits that look like adjustments due to upstream changes that this issue would be likely to exacerbate (I didn't check src/wallet/rpc/walletnames.cpp but I'd expect the hits to be the same):

• 2023 Jan 23
• 2022 Apr 7
• 2020 Sep 7
• 2019 Jul 15
• 2019 Mar 5
• 2018 Nov 26
• 2018 Aug 9

I stopped looking in 2017 Sep.

So that looks like between one incident per two years, and two incidents per year (with the rate appearing to decrease over time). Does that sound about right?

If we're targeting 4 major releases before the old methods are removed from the code base (2 before they're invisible, and another 2 before we wipe them out), that would be 2 years of maintenance effort for the old methods? Which would be between 1 and 4 merge incidents (closer to 1 given the time correlation)?

If implementing this issue is covered by an external funder (to be clear, I do not recommend implementing this without funding), could we simply price the maintenance effort into the funding proposal, so that the funder commits to fund our maintenance effort (based on what we figure 4 merge incidents would cost us in terms of dev time)?

I personally wouldn't mind handling those incidents when they happen (as long as that work is funded), it'd be a good excuse to gain some more familiarity with the RPC subsystem (which is something I've wanted to do anyway).

[domob1812]

How do you see this working with external funding? I'm fine to let someone else do this (and be paid for it) - but what shall I do if a weekly merge results in merge conflicts and/or broken build or tests? I would rather not block the merge for perhaps a few weeks until someone else can get it fixed, because that will then just cause more merge conflicts and work to be piling up.

Or do you think if in such a case I just push the merge with the conflicts still in, you could go and fix them within the week before the next merge?

[JeremyRand]

@domob1812 This kind of event is sufficiently rare that, as long as I'm not away at a conference, I don't mind dropping everything and sorting it out as soon as you notify me that it's needed. In the unlikely event that I'm away at a conference, I won't be able to do that -- but perhaps that's a sufficiently rare combination of circumstances that you could take care of it if I'm at a conference when the breakage happens?