rpcserver: assert network for validateaddress rpc.

[dnldd]

resolves #962

[dajohi]

is this correct? should it return the error?

[davecgh]

It definitely should not return an error.

[dajohi]

could remove the if and just do
result.Address = addr.EncodeAddress()
result.IsValid = addr.IsForNet(s.server.chainParams)

[dnldd]

@dajohi updated.

[davecgh]

This would be a breaking change. It is supposed to return an empty result (where the isvalid field is to to false as the comment indicates) when it fails to decode, not an error.

[davecgh]

I think this should return an error when it's not for the current network as opposed to just being false. It is still a valid address, just not for the current network, so just returning false in that case would be misleading and incorrect.

It should match other handlers which do:

	if !addr.IsForNet(s.server.chainParams) {
		return nil, rpcAddressKeyError("Wrong network: %v", addr)
	}
}

It should also go just after the call to DecodeAddress, before spending cycles encoding the address into the result.

[davecgh]

Alright, after further discussion with the team, it seems that everyone would prefer it to not error in this case so it is consistent with both dcrwallet and other code in the ecosystem which expects that behavior.

Would you mind changing this back to the following?

result.IsValid = addr.IsForNet(s.server.chainParams)

Sorry for the back and forth!

[davecgh]

This shouldn't be set when it's for the wrong network.

Notice the difference between dcrwallet and dcrd below.

dcrd:

$ dcrctl validateaddress TsYEVnkLPSniXpqUNqPZKEktCLkBzjXjK6n
{
  "isvalid": false,
  "address": "TsYEVnkLPSniXpqUNqPZKEktCLkBzjXjK6n"
}

dcrwallet:

$ dcrctl --wallet validateaddress TsYEVnkLPSniXpqUNqPZKEktCLkBzjXjK6n
{
  "isvalid": false,
}

[davecgh]

The following diff against master will result in the desired behavior:

diff --git a/rpcserver.go b/rpcserver.go
index 4ab9bee4..d1ec41c0 100644
--- a/rpcserver.go
+++ b/rpcserver.go
@@ -5566,7 +5566,7 @@ func handleValidateAddress(s *rpcServer, cmd interface{}, closeChan <-chan struc

        result := dcrjson.ValidateAddressChainResult{}
        addr, err := dcrutil.DecodeAddress(c.Address)
-       if err != nil {
+       if err != nil || !addr.IsForNet(s.server.chainParams) {
                // Return the default value (false) for IsValid.
                return result, nil
        }

[davecgh]

I've tested this on both mainnet and testnet with the following results:

Mainnet:

Address	Description	Expected Valid?	Result
18cBEMRxXHqzWWCxZNtU91F5sbUNKhL5PX	Valid Bitcoin p2pkh mainnet	false	{"isvalid": false}
18cBEMRxXHqzWWCxZNtU91F5sbUNKhL5PY	Invalid Bitcoin p2pkh mainnet	false	{"isvalid": false}
Dsb5EYhZBjGCcVrciBjbNLg3AiypAUxW76n	Valid Decred p2pkh mainnet	true	{"isvalid": true, "address": "Dsb5EYhZBjGCcVrciBjbNLg3AiypAUxW76n"}
Dsb5EYhZBjGCcVrciBjbNLg3AiypAUxW76o	Invalid Decred p2pkh mainnet	false	{"isvalid": false}
Dcur2mcGjmENx4DhNqDctW5wJCVyT3Qeqkx	Valid Decred p2sh mainnet	true	{"isvalid": true, "address": "Dcur2mcGjmENx4DhNqDctW5wJCVyT3Qeqkx"}
Dcur2mcGjmENx4DhNqDctW5wJCVyT3Qeqky	Invalid Decred p2sh mainnet	false	{"isvalid": false}
TsYEVnkLPSniXpqUNqPZKEktCLkBzjXjK6n	Valid Decred p2pkh testnet	false	{"isvalid": false}
TsYEVnkLPSniXpqUNqPZKEktCLkBzjXjK6o	Invalid Decred p2pkh testnet	false	{"isvalid": false}

Testnet:

Address	Description	Expected Valid?	Result
Dsb5EYhZBjGCcVrciBjbNLg3AiypAUxW76n	Valid Decred p2pkh mainnet	false	{"isvalid": false}
Dsb5EYhZBjGCcVrciBjbNLg3AiypAUxW76o	Invalid Decred p2pkh mainnet	false	{"isvalid": false}
Dcur2mcGjmENx4DhNqDctW5wJCVyT3Qeqkx	Valid Decred p2sh mainnet	false	{"isvalid": false}
Dcur2mcGjmENx4DhNqDctW5wJCVyT3Qeqky	Invalid Decred p2sh mainnet	false	{"isvalid": false}
TsYEVnkLPSniXpqUNqPZKEktCLkBzjXjK6n	Valid Decred p2pkh testnet	true	{"isvalid": true, "address": "TsYEVnkLPSniXpqUNqPZKEktCLkBzjXjK6n"}
TsYEVnkLPSniXpqUNqPZKEktCLkBzjXjK6o	Invalid Decred p2pkh testnet	false	{"isvalid": false}

The results are all as expected.

OK

[davecgh]

I will merge this as soon as @dajohi approves since it's blocked due to his requested changes.