grpc: Return more info for ValidateAddress. #2091

JoeGruffins · 2021-10-01T03:41:25Z

Extract all possible data from unknown as well as known addresses. In
particular, always return the correct address type. Also allow creating
an address id for PubKeyEd25519V0 and PubKeySchnorrSecp256k1V0 types.

JoeGruffins · 2021-10-01T08:53:45Z

It would be possible to write unit tests if the server accepted a wallet interface. But in the mean time here are the comparisons of script types for addresses that are not owned before and after:

P2PK ECDSA secp256k1
DkM3ZigNyiwHrsXRjkDQ8t8tW6uKGW9g61qEkG3bMqQPQWYEf5X3J
master:
"script_type": "NonStandardTy",
this diff:
"script_type": "PubKeyTy",

P2PK Ed25519
DkM5zR8tqWNAHngZQDTyAeqzabZxMKrkSbCFULDhmvySn3uHmm221
master:
error: invalid operation
this diff:
"script_type": "PubkeyAltTy",

P2PKH ECDSA secp256k1
DsUZxxoHJSty8DCfwfartwTYbuhmVct7tJu
master:
"script_type": "NonStandardTy",
this diff:
"script_type": "PubKeyHashTy",

P2PKH Schnorr secp256k1
DSXcZv4oSRiEoWL2a9aD8sgfptRo1YEXNKj
master:
"script_type": "NonStandardTy",
this diff:
"script_type": "PubkeyHashAltTy",

P2SH
DcuQKx8BES9wU7C6Q5VmLBjw436r27hayjS
master:
"script_type": "NonStandardTy",
this diff:
"script_type": "ScriptHashTy",

matheusd

Tested fine

matheusd · 2021-10-04T13:26:29Z

rpc/api.proto

@@ -1100,6 +1100,8 @@ message ValidateAddressResponse {
 		StakeSubChangeTy = 9;
 		PubkeyAltTy = 10;
 		PubkeyHashAltTy = 11;
+		TreasuryAddTy = 12;
+		TreasuryGenTy = 13;


These were added but not used. Is this correct?

I believe that if you verified a script hash of a treasury add or gen that your wallet has the script to, this would be returned. On master, testing for treasury is off, but this pr also turns it on. I will try it out and see if I get the return.

yeah, this looks right. these constants correlate to those of txscript.ScriptClass and must be kept in sync. There's a type conversion between the txscirpt.ScriptClass to this protobuf enum so the values must be the same.

I believe I used the reverse order of these two in the stdscript PR @JoeGruffins, sorry. Note that there's now a scProto helper to translate from the more-fine-grained stdscript.ScriptType to the values in both this enum and the decode enum in the same file. (the txscript.ScriptClasses are gone and that order is now irrelevant afaict, but we can switch both enums around to be compatiblish with consumers that might be looking for the old txscript order)

JoeGruffins · 2021-10-05T05:08:59Z

Owned scripts (simnet):

ScixyRDPYRjX8R9hoCfQD8ouZjEVZvdhD5Z
master
{
"is_valid": true,
"is_mine": true,
"account_number": 2147483647,
"pub_key_addr": "",
"pub_key": "",
"is_script": true,
"pk_script_addrs": [],
"script_type": "NonStandardTy",
"pay_to_addr_script": "qRRZFOMBLdI8T9TKQNkYIoz3yxT9rYg=",
"sigs_required": 0,
"is_internal": false,
"index": 0
}

this diff
{
"is_valid": true,
"is_mine": true,
"account_number": 2147483647,
"pub_key_addr": "",
"pub_key": "",
"is_script": true,
"pk_script_addrs": [
"SsoSgQJSTvn4ytD7Yhv6Qqkcc2ZTpWphQoN"
],
"script_type": "TreasuryGenTy",
"pay_to_addr_script": "qRRZFOMBLdI8T9TKQNkYIoz3yxT9rYg=",
"sigs_required": 0,
"is_internal": false,
"index": 0
}

multisig
ScvKiNrS71VhbEykSE6u1Jv2kGYMua5wdWB
master
{
"is_valid": true,
"is_mine": true,
"account_number": 2147483647,
"pub_key_addr": "",
"pub_key": "",
"is_script": true,
"pk_script_addrs": [],
"script_type": "NonStandardTy",
"pay_to_addr_script": "qRTVqnNAaaSW9F3ouZPJRkdOXctG2Ig=",
"sigs_required": 0,
"is_internal": false,
"index": 0
}

this diff
{
"is_valid": true,
"is_mine": true,
"account_number": 2147483647,
"pub_key_addr": "",
"pub_key": "",
"is_script": true,
"pk_script_addrs": [
"SkQmogHLgvAs19JiYRg77ySi3NTNKhDAWsR3vcZsFMCLptiBWsa4M",
"SkQkhTHAMNnt9FsE3ekRGevPbFiibD7Y8VkNFRWyuos1itFwpcvNC"
],
"script_type": "MultiSigTy",
"pay_to_addr_script": "qRTVqnNAaaSW9F3ouZPJRkdOXctG2Ig=",
"sigs_required": 2,
"is_internal": false,
"index": 0
}

JoeGruffins · 2021-10-05T05:10:57Z

It's seems odd that the script itself is not returned although it is over rpc.

JoeGruffins · 2021-10-05T05:13:53Z

These are the scripts I imported to get these returns:
c376a914d17043c104a57393aa7353e1510e39eab811e3db88ac (~~TADD~~ TSPEND)
522103d484eb60ad03549e731ae9045281f8ee14ff6ea11b697f32cde3d8a18992261b210342b0b9c0ecb53cb9761beb0d010bbf08b5049d2a4d3bea5d3a1d95eb664931cb52ae (2 of 2 multisig)

internal/rpc/rpcserver/server.go

JoeGruffins · 2021-10-06T01:09:33Z

wallet/addresses.go

-		22: txscript.OP_EQUALVERIFY,
+		22: txscript.OP_EQUAL,


afaict this is correct. It was producing non-standard scripts according to decodescript, and the value was different than what stdaddr will return.

https://github.com/decred/dcrd/blob/931a579e127b466e0776dedf3ee4ae1b5cc25e14/txscript/stdaddr/addressv0.go#L990-L1012

Agree, it is not OP_EQUALVERIFY

internal/rpc/rpcserver/server.go

JoeGruffins · 2021-12-13T08:18:00Z

Will rebase this soon.

chappjc · 2021-12-13T15:14:26Z

internal/rpc/rpcserver/server.go

+	semverString = "7.13.0"
 	semverMajor  = 7
-	semverMinor  = 12
+	semverMinor  = 13


When you rebase, you'll find it's already at 7.13, so you may wish to bump (or just leave it since it is recently bumped on master)

Will leave as it is then.

chappjc · 2021-12-13T15:17:33Z

internal/rpc/rpcserver/server.go

-		result.IsScript = true
-		_, script := ka.PaymentScript()
-		result.PayToAddrScript = script
+		_, redeem := ka.RedeemScript()


I'm glad you're fixing this. You'll see my comments when you rebase:
dbdf7aa#diff-a23056f9a8432e77f80e98bb56a4207adbd2412877c66818836b2256602beb39R2031
This was one of a few things wrong with this method.

chappjc · 2021-12-13T15:23:20Z

internal/rpc/rpcserver/server.go

+		result.ScriptType = pb.ValidateAddressResponse_ScriptHashTy
+		result.IsScript = true
+	}
+	_, result.PayToAddrScript = addr.PaymentScript()


Another thing you'll see when rebasing is the stdscript changes in this method. dbdf7aa#diff-a23056f9a8432e77f80e98bb56a4207adbd2412877c66818836b2256602beb39R1993-R1997

I noted how I believed this would be handled, but without consideration to PubKey and PubKeyAddr, so possibly your type switch is fine, except perhaps for limiting to v0 scripts.

ver, scr := addr.PaymentScript() class, _ := stdscript.ExtractAddrs(ver, scr, s.wallet.ChainParams()) result.ScriptType = pb.ValidateAddressResponse_ScriptType(scProto(class)) result.PayToAddrScript = scr

And for the PubKey field, you could assert addr.(stdaddr.SerializedPubKeyer) to catch any relevant address type.

You got result.PayToAddrScript assigned in here twice with that last commit.

Thank you. weird. Just combined it all into one commit.

chappjc · 2021-12-13T15:27:40Z

rpc/api.proto

@@ -1100,6 +1100,8 @@ message ValidateAddressResponse {
 		StakeSubChangeTy = 9;
 		PubkeyAltTy = 10;
 		PubkeyHashAltTy = 11;
+		TreasuryAddTy = 12;
+		TreasuryGenTy = 13;


I believe I used the reverse order of these two in the stdscript PR @JoeGruffins, sorry. Note that there's now a scProto helper to translate from the more-fine-grained stdscript.ScriptType to the values in both this enum and the decode enum in the same file. (the txscript.ScriptClasses are gone and that order is now irrelevant afaict, but we can switch both enums around to be compatiblish with consumers that might be looking for the old txscript order)

chappjc · 2021-12-13T15:29:18Z

wallet/addresses.go

-		22: txscript.OP_EQUALVERIFY,
+		22: txscript.OP_EQUAL,


Agree, it is not OP_EQUALVERIFY

chappjc · 2021-12-13T15:30:29Z

wallet/udb/addressmanager.go

@@ -106,6 +106,10 @@ func normalizeAddress(addr stdaddr.Address) stdaddr.Address {
 	switch addr := addr.(type) {


Might consider addr.(stdaddr.AddressPubKeyHasher) here, both for robustness with >v0 scripts, and other address types.

https://github.com/decred/dcrd/blob/169413c65ae0fd8d61d997fea59d5009bda3dc27/internal/staging/stdaddr/example_test.go#L61-L66

chappjc · 2021-12-13T15:35:59Z

It would be possible to write unit tests if the server accepted a wallet interface. But in the mean time here are the comparisons of script types for addresses that are not owned before and after:

Please see the rpc/grpc_example cli app I added.
I was using this to validate several gRPC responses, including ValidateAddress:

dcrwallet/rpc/grpc_example/main.go

Lines 89 to 96 in e2bce3d

    
           	Address: "TcpEWwGdCN3RCNAQUhBn8f2Xdko2JzcQSQs", // ValidateAddress only sets ScriptType for owned and P2SH addresses. 
        
           } 
        
           validateAddrResp, err := wsClient.ValidateAddress(context.Background(), validateAddrRequest) 
        
           if err != nil { 
        
           	fmt.Println(err) 
        
           	return 
        
           } 
        
           fmt.Println(validateAddrResp.ScriptType) // unset / non-standard unless wallet-owned AND P2SH!

This is no unit test, but it's better than nothing.

JoeGruffins · 2021-12-14T07:19:10Z

This is no unit test, but it's better than nothing.

If it gets my pr merged I'll do it...

I see I don't need the first commit any more, popping it off.

rpc/grpc_example/main.go

Extract all possible data from unknown as well as known addresses. In particular, always return the correct address type. Also allow creating an address id for PubKeyEd25519V0 and PubKeySchnorrSecp256k1V0 types. Extract addrs from redeem script when found in the db.

jrick · 2021-12-16T14:08:51Z

internal/rpc/rpcserver/server.go

@@ -2050,7 +2050,7 @@ func (s *walletServer) ValidateAddress(ctx context.Context, req *pb.ValidateAddr
 	switch ka := ka.(type) {
 	case wallet.BIP0044Address:
 		_, branch, child := ka.Path()
-		result.IsInternal = branch == 1
+		result.IsInternal = branch == udb.InternalBranch


For future reference, udb should not be used anywhere outside of the wallet package. This is a layering violation and it's making it more difficult to convert the package to being internal (but this particular case is not as bad as say, depending on a type defined by that package).

JoeGruffins force-pushed the returnscripttype branch 2 times, most recently from 91b11e6 to 6723e4c Compare October 1, 2021 08:38

JoeGruffins force-pushed the returnscripttype branch from 6723e4c to a465ed6 Compare October 1, 2021 09:03

JoeGruffins marked this pull request as ready for review October 1, 2021 09:04

matheusd reviewed Oct 4, 2021

View reviewed changes

jrick reviewed Oct 5, 2021

View reviewed changes

internal/rpc/rpcserver/server.go Outdated Show resolved Hide resolved

JoeGruffins commented Oct 6, 2021

View reviewed changes

jrick reviewed Oct 8, 2021

View reviewed changes

internal/rpc/rpcserver/server.go Outdated Show resolved Hide resolved

JoeGruffins force-pushed the returnscripttype branch from 9e8f552 to ba65332 Compare October 11, 2021 01:08

JoeGruffins force-pushed the returnscripttype branch from ba65332 to 2973e01 Compare November 2, 2021 02:54

JoeGruffins mentioned this pull request Dec 13, 2021

jsonrpc: Return more info for validateaddress. #2096

Merged

chappjc suggested changes Dec 13, 2021

View reviewed changes

JoeGruffins force-pushed the returnscripttype branch from 2973e01 to 0ee400d Compare December 14, 2021 07:11

JoeGruffins force-pushed the returnscripttype branch 2 times, most recently from cf3c986 to 69c9106 Compare December 15, 2021 03:25

JoeGruffins commented Dec 15, 2021

View reviewed changes

rpc/grpc_example/main.go Show resolved Hide resolved

JoeGruffins force-pushed the returnscripttype branch 2 times, most recently from bd6c477 to 65ce480 Compare December 15, 2021 10:11

chappjc reviewed Dec 15, 2021

View reviewed changes

rpc/grpc_example/main.go Outdated Show resolved Hide resolved

JoeGruffins force-pushed the returnscripttype branch from 65ce480 to b529f68 Compare December 16, 2021 00:57

JoeGruffins added 2 commits December 16, 2021 10:26

rpc: Add more grpc examples.

60ce70a

JoeGruffins force-pushed the returnscripttype branch from b529f68 to 60ce70a Compare December 16, 2021 01:26

jrick approved these changes Dec 16, 2021

View reviewed changes

jrick merged commit 60ce70a into decred:master Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grpc: Return more info for ValidateAddress. #2091

grpc: Return more info for ValidateAddress. #2091

JoeGruffins commented Oct 1, 2021

JoeGruffins commented Oct 1, 2021

matheusd left a comment

matheusd Oct 4, 2021

JoeGruffins Oct 4, 2021 •

edited

jrick Oct 5, 2021

chappjc Dec 13, 2021 •

edited

JoeGruffins commented Oct 5, 2021 •

edited

JoeGruffins commented Oct 5, 2021

JoeGruffins commented Oct 5, 2021 •

edited

JoeGruffins Oct 6, 2021 •

edited

chappjc Dec 13, 2021

JoeGruffins commented Dec 13, 2021

chappjc Dec 13, 2021

JoeGruffins Dec 14, 2021

chappjc Dec 13, 2021

chappjc Dec 13, 2021 •

edited

JoeGruffins Dec 14, 2021

chappjc Dec 14, 2021

JoeGruffins Dec 15, 2021

chappjc Dec 13, 2021 •

edited

chappjc Dec 13, 2021

chappjc Dec 13, 2021 •

edited

JoeGruffins Dec 16, 2021

chappjc commented Dec 13, 2021 •

edited

JoeGruffins commented Dec 14, 2021 •

edited

jrick Dec 16, 2021

		@@ -106,6 +106,10 @@ func normalizeAddress(addr stdaddr.Address) stdaddr.Address {
		switch addr := addr.(type) {

grpc: Return more info for ValidateAddress. #2091

grpc: Return more info for ValidateAddress. #2091

Conversation

JoeGruffins commented Oct 1, 2021

JoeGruffins commented Oct 1, 2021

matheusd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins Oct 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Dec 13, 2021 • edited

Choose a reason for hiding this comment

JoeGruffins commented Oct 5, 2021 • edited

JoeGruffins commented Oct 5, 2021

JoeGruffins commented Oct 5, 2021 • edited

JoeGruffins Oct 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins commented Dec 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Dec 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Dec 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Dec 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Dec 13, 2021 • edited

JoeGruffins commented Dec 14, 2021 • edited

Choose a reason for hiding this comment

JoeGruffins Oct 4, 2021 •

edited

chappjc Dec 13, 2021 •

edited

JoeGruffins commented Oct 5, 2021 •

edited

JoeGruffins commented Oct 5, 2021 •

edited

JoeGruffins Oct 6, 2021 •

edited

chappjc Dec 13, 2021 •

edited

chappjc Dec 13, 2021 •

edited

chappjc Dec 13, 2021 •

edited

chappjc commented Dec 13, 2021 •

edited

JoeGruffins commented Dec 14, 2021 •

edited