Skip to content

Conversation

kallewoof
Copy link
Contributor

This PR initiates the transition to using normalized parameter names in objects in RPC commands.

Because this transition all at once would be huge, the PR also includes a during-transition temporary structure which will make it transparent to the user whether the object has been normalized or not.

There is also one conversion in here, for the send RPC, to show what conversion would entail.

This is an alternative to #19957. (A much better alternative, IMO.)

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

As an intermediate step I was thinking request.params could have both keys, the original and the normalized (obviously should assert that the normalized key doesn't conflict with another one). This way it's not necessary to call NormalizedObject on the RPC methods.

A next intermediate step would be to remove original keys and only keep the normalized ones, this behavior could be turned on in RPCHelpMan on each RPC method to support small changes.

@kallewoof
Copy link
Contributor Author

@promag I tried to work on a solution based on always providing the entire params object recursively normalized, keeping the originals around as well, but it is very easy to mess up. For example if I convert send and leave FundTransaction as it is, the updated send will now put new normalized keys in the options and FundTransaction will fail to find those, even though the initial code had copied everything.

Since it also adds to named parameters, patches are needed in various places to make it not yell about missing or unknown parameters.

Still digging, but this seems to increase complexity quite a bit.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 25, 2020

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

When using -named, this converts all variants of case and underscores into a normalized lower case no underscore parameter name.
@kallewoof
Copy link
Contributor Author

kallewoof commented Sep 29, 2020

I extended this a slight bit to now also normalize the -named functionality.

$ ./bitcoin-cli -regtest -named send out_puts='[{"bcrt1qkfveed8g8vnktsc3490aarzh302l2qc2ymhlge":1}]' conftarget=1 estim_ate_mode=sat/b

In the process, I ran into linker issues with QT build, so I inlined the NormalizeParameterName function. Seems moving it to a shared place between rpc/client and rpc/util, but not clear where.

@kallewoof
Copy link
Contributor Author

Expanding on this, I am coming to the conclusion that this is error prone and messy -- did you normalize or didn't you? That would be a continuous problem. @promag's #20017 seems like a better approach.

@kallewoof kallewoof closed this Sep 30, 2020
@kallewoof kallewoof deleted the 202009-rpc-normalize branch September 30, 2020 07:39
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants