RPC: Validate name_anyupdate values

[JeremyRand]

Describe the issue

The name_anyupdate RPC methods do not perform any validation of the name or value.

Can you reliably reproduce the issue?

Yes.

If so, please list the steps to reproduce below:

1. Try to name_update to a value that's invalid JSON, name_firstupdate a d/ name that includes uppercase characters, or any name data that otherwise violates the namespace specifications.

Expected behaviour

An error should be given, unless the user passes an optional flag to disable validity checking.

Actual behaviour

No validity checking occurs.

What version of namecoin-core are you using?

nc0.13.99-name-tab-beta1

Machine specs:

• OS: Whonix 13 inside Qubes 3.2
• CPU: Haswell i7
• RAM: 16 GiB for Qubes, up to 4 GiB for Whonix
• Disk size: 20 GB assigned to Whonix
• Disk Type (HD/SDD): HD

Any extra information that might be useful in the debugging process.

Obviously Namecoin's consensus rules allow arbitrary binary blobs in the name or the value, so validity checking should be disableable by people who know what they're doing. However, it's still useful for preventing accidental user errors. I'm open to suggestions on what exactly the UX should be here, but I suggest that we might consider replacing the destination address in the name_anyupdate methods with a JSON object that allows setting the address, as well as setting whether validation of name and value is performed. This JSON object might also be a useful way to allow the user to control whether ASCII or binary data is being provided in the name and value fields, as discussed in #164 .

[domob1812]

We now have a JSON options argument, so we could implement this. However, I don't think it should really be Namecoin Core's job to validate anything here. That would intermingle the core daemon with rules about specific usecases of Namecoin, even if it is just the RPC layer and opt-out.

@JeremyRand, do you still believe this should be done directly in Namecoin Core?

[yanmaani]

@domob1812 This was closed by accident and should be re-opened because it's still relevant (cf. the discussion with @JeremyRand in #436).

I should also add, in response to Mr. Rand's comment:

(E.g. Tor Project policy is that user perception of the Tor/Firefox segregation should be minimized, because that segregation confuses users.)

Even so, Tor Project do not endeavor to merge the Tor and the Firefox codebases, nor to add in web-aware code in the tor daemon. The integration is strictly one way.

[JeremyRand]

We now have a JSON options argument, so we could implement this. However, I don't think it should really be Namecoin Core's job to validate anything here. That would intermingle the core daemon with rules about specific usecases of Namecoin, even if it is just the RPC layer and opt-out.

@JeremyRand, do you still believe this should be done directly in Namecoin Core?

@domob1812 Are you arguing that validation of user-entered name values should not be in Namecoin Core at all, or are you arguing that it should only be in Namecoin-Qt (not namecoind)? If the former, how do you propose solving the UX issue of users entering bad data? If the latter, what are your grounds for deviating from standard Bitcoin Core policy (i.e. new functionality should be added to RPC before being added to GUI)?

[domob1812]

I would like to see validation in the Qt only. The reason is that this is functionality that is IMHO unrelated to the core function of namecoind, and thus ideally should not make the codebase there more complex. Bitcoin Core is also working on separating out the Qt, and there are features that are only really available in the UI already, e.g. coin control. (Which may be a bad example as it has code in the core codebase to support it, but the feature as such is only available from the Qt and not e.g. from sendtoaddress.)

[JeremyRand]

there are features that are only really available in the UI already, e.g. coin control. (Which may be a bad example as it has code in the core codebase to support it, but the feature as such is only available from the Qt and not e.g. from sendtoaddress.)

@domob1812 I don't think this is actually the case. The send RPC method does include Coin Control. I suspect that the Coin Control functionality was added to RPC after GUI due to grandfathering (the RPC-first policy was put in place by bitcoin/bitcoin@b8c06ef, which was ~2 years after Coin Control was merged in bitcoin/bitcoin#3253 ).

[domob1812]

The code exists, but the RPC does not actually expose the functionality, does it? Maybe that was changed and I missed it, though.

In any case, I think I made my point clear, that I don't like the idea of having any application-specific code in the core codebase. If you disagree, that's fine. And as long as the code is clearly separated (e.g. in its own rpc* cpp file) and won't cause too much extra maintenance burden, I'm not completely opposed to including it as RPC functionality in that case.

[JeremyRand]

Okay, segregating namespace-aware RPC methods into their own source code file seems like a reasonable compromise.

Running help send yields this example:

Create a transaction that should confirm the next block, with a specific input, and return result without adding to wallet or broadcasting to the network
> namecoin-cli send '{"nc1qkunnuwx82hj7pf48x88c7u78g7jf6dc9uyqxl8": 0.1}' 1 economical '{"add_to_wallet": false, "inputs": [{"txid":"a08e6907dbbd3d809776dbfc5d82e371b764ed838b5655e72f463568df1aadf0", "vout":1}]}'