Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

keyring API doesn't work as expected #11368

Closed
4 tasks
Tracked by #12665
frumioj opened this issue Mar 12, 2022 · 3 comments · Fixed by #13145
Closed
4 tasks
Tracked by #12665

keyring API doesn't work as expected #11368

frumioj opened this issue Mar 12, 2022 · 3 comments · Fixed by #13145
Assignees
Labels

Comments

@frumioj
Copy link
Contributor

frumioj commented Mar 12, 2022

Summary of Bug

keyring List() method call returns a panic:

2022/03/12 13:06:15 keyring: {0xc000894ed0 0xc000737620 {[{}] [{}]}}
2022/03/12 13:06:15 Keys?: [%!v(PANIC=String method: reflect.Value.Interface: cannot return value obtained from unexported field or method)]
2022/03/12 13:06:15 New keystore: []
2022/03/12 13:06:15 Keys: [%!v(PANIC=String method: reflect.Value.Interface: cannot return value obtained from unexported field or method) %!v(PANIC=String method: reflect.Value.Interface: cannot return value obtained from unexported field or method)]

This may indicate that under some circumstances that the keyring New function fails to create or use the keyring correctly but does not return an error, so you can't tell that something didn't happen as expected (note my explicit tests for err != nil, and failure anyway). Both using an existing keystore, and trying to create a new keystore and then list the empty keys both fail.

Version

25feb23

Steps to Reproduce

  1. git clone master
  2. make build && make install
  3. Follow Cosmos tutorial to bring up a local test node
  4. Build and run the test program in https://gist.github.com/frumioj/be41e2c40dcf72bd9161fa7616b85953 (probably will need to change the directory names used since these are relative to my own directory structure).

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@tac0turtle tac0turtle added the C:Keys Keybase, KMS and HSMs label Mar 14, 2022
@ainhoa-a ainhoa-a mentioned this issue Aug 16, 2022
24 tasks
@raynaudoe
Copy link
Contributor

I was able to reproduce it, it is not a panic related to the List() method but rather when doing log.Printf("Keys: %v\n", keyS).
This is related to a gogoproto issue when it tries to convert to String the field

@raynaudoe
Copy link
Contributor

raynaudoe commented Sep 6, 2022

Some details that I've found:
the problem comes when trying to print a Record type, which is a protobuf generated struct:

type Record struct {
	// name represents a name of Record
	Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"`
	// pub_key represents a public key in any format
	PubKey *types.Any `protobuf:"bytes,2,opt,name=pub_key,json=pubKey,proto3" json:"pub_key,omitempty"`
	// Record contains one of the following items
	//
	// Types that are valid to be assigned to Item:
	//	*Record_Local_
	//	*Record_Ledger_
	//	*Record_Multi_
	//	*Record_Offline_
	Item isRecord_Item `protobuf_oneof:"item"`
}

the failing field here is the Item one, so that when the protobuf String() function is called, the decoder ends up with a reflect.Value error of the kind reflect.Value.Interface: cannot return value obtained from unexported field or method.

This, I think, is due to a bug in regen-network/protobuf@v1.3.3-alpha.regen.1/proto/text.go which handles badly unexported fields. Otherwise, when adding to record.proto the following line:

option (gogoproto.gogoproto_import) = false;

the code ends up calling:
https://github.com/golang/protobuf/blob/ae97035608a719c7a1c1c41bed0ae0744bdb0c6f/proto/text_encode.go#L43
instead of:
https://github.com/regen-network/protobuf/blob/8a70ddfdaf94e68497f7b26a6896ad75e03b6616/proto/text.go#L930

which handles the String() func without any errors

@tac0turtle
Copy link
Member

@aaronc @julienrbrt we should try to fix this in the fork of gogoproto (cosmos/gogoproto)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants