Repository: bitcoin/bitcoin
File: src/rpc/client.cpp (C++)

Code before:

        std::string name{s.substr(0, pos)};
        std::string_view value{s.substr(pos+1)};

        // Handle RPC methods that have optional string parameters; for these, the parameter index and name mapping may not be fixed
        if (rpcCvtTable.IsOptionalStringParam(strMethod)) {
            // If the parameter name and index match an entry in vRPCStringParams, or the parameter is a known string parameter for this method, treat it as a named parameter
            if (rpcCvtTable.HasStringParamForMethodAndIndex(strMethod, name, positional_args.size()) || rpcCvtTable.IsStringParam(strMethod, name)) {
                params.pushKV(name, rpcCvtTable.ArgToUniValue(value, strMethod, name));
            }
            // If the parameter is a known conversion parameter described in the vRPCConvertParams table by name, treat it as a named parameter
            else if(rpcCvtTable.IsConvertParamByName(strMethod, name)) {
                params.pushKV(name, rpcCvtTable.ArgToUniValue(value, strMethod, name));
            }
            // If the parameter is not recognized as a named parameter, treat it as a positional
            else {
                positional_args.push_back(rpcCvtTable.ArgToUniValue(s, strMethod, name));
            }
            continue;
        }

        if (rpcCvtTable.HasKnownStringParams(strMethod, positional_args.size())) {
            // If this method has known string parameters, check if this specific parameter is one of them
            if (rpcCvtTable.IsStringParam(strMethod, name)) {
                // Pass only the value as positional
                positional_args.push_back(rpcCvtTable.ArgToUniValue(value, strMethod, positional_args.size()));
            } else {
                // If parameter is unknown for a given known string method, then treat the whole string as positional
                positional_args.push_back(rpcCvtTable.ArgToUniValue(s, strMethod, positional_args.size()));
            }
            continue;
        }
        // Intentionally overwrite earlier named values with later ones as a
        // convenience for scripts and command line users that want to merge
        // options.
        params.pushKV(name, rpcCvtTable.ArgToUniValue(value, strMethod, name));
    }

    if (!positional_args.empty()) {
        // Use pushKVEnd instead of pushKV to avoid overwriting an explicit
        // "args" value with an implicit one. Let the RPC server handle the
        // request as given.
        params.pushKVEnd("args", std::move(positional_args));
    }

    return params;
}


Review comment (bug):
In commit "rpc: Handle -named argument parsing where Base64 encoding is used" (bc2241ab3d01a38f19588cb85134e5330b77e3f4)

Thanks for the updates!

Looking at more closely at this solution, some things don't make sense to me. This code from lines 513-515 is checking to see if a command line argument that parses as NAME=VALUE has a NAME that is listed in the table above as a known string parameter. But if it is instead of adding the parameter to `params` and passing the argument by name, it is discarding the name, adding the parameter to `positional_args` and passing the argument by position, which seems like the opposite of what it should be doing. This looks like a bug, but it might be easy to fix and the else condition below does look right.

I'm also pretty confused by the IsOptionalStringParam code above. It seems complex and I don't actually understand what cases that is intended to handle.

Also when experimenting with this, I realized the original bug of `=` characters causing positional parameters to be mistakenly interpreted as named parameters is not just limited to string parameters, but can also happen with JSON parameters. This easy to see with following example:

```
build/bin/bitcoin-cli -regtest -named echojson '["ok"]'
build/bin/bitcoin-cli -regtest -named echojson '["fail=yes"]'
```

which fails with `Unknown named parameter ["fail`. Fixing this might be out of scope for this PR, but it should be easier to fix than the original problem because the JSON representation is not ambiguous. So it could be nice to fix here.

To understand the problem better, I implemented df4f39143d868fe4303ab7b3875757a26b7ac5a5 ([tag](https://github.com/ryanofsky/bitcoin/commits/review.32821.2-edit.1)) on top of your PR which I think takes a simpler and more general approach and should fix this problem more fully.

If it would be helpful, feel free to use any of that code in this PR. And if you think the current approach is better, that could also be the case. I mostly just don't understand the optional string aspect of bc2241ab3d01a38f19588cb85134e5330b77e3f4 and what cases it is handling.

Code after:
            const CRPCConvertParam* positional_param = rpc_convert::FromPosition(strMethod, positional_args.size());
            UniValue parsed_value;
            if (positional_param && positional_param->format == ParamFormat::JSON && parsed_value.read(s)) {
                positional_args.push_back(std::move(parsed_value));
                continue;
            } else if (positional_param && positional_param->format == ParamFormat::STRING) {
                positional_args.push_back(s);
                continue;
            }
        }

        // Intentionally overwrite earlier named values with later ones as a
        // convenience for scripts and command line users that want to merge
        // options.
        params.pushKV(name, ParseParam(named_param, value));
    }

    if (!positional_args.empty()) {
        // Use pushKVEnd instead of pushKV to avoid overwriting an explicit
        // "args" value with an implicit one. Let the RPC server handle the
        // request as given.
        params.pushKVEnd("args", std::move(positional_args));
    }

    return params;
}