Skip to content

Added Protobuf marshal and unmarshal compatiablity function in block.g…#2705

Closed
vlasfama wants to merge 1 commit intotendermint:developfrom
vlasfama:grpcdev
Closed

Added Protobuf marshal and unmarshal compatiablity function in block.g…#2705
vlasfama wants to merge 1 commit intotendermint:developfrom
vlasfama:grpcdev

Conversation

@vlasfama
Copy link
Contributor

@vlasfama vlasfama commented Oct 26, 2018 •
edited
Loading

Added Protbuf marshal and unmarshal compatiablity function in block.go,blockmeta.go,peerround and roundstate.go

As you have mentioned the methods using (https://github.com/gogo/protobuf) for marshal and unmarshal here #2660 , I am using same in my proto file.

   import "github.com/gogo/protobuf/gogoproto/gogo.proto";
         option (gogoproto.marshaler_all) = true;
         option (gogoproto.unmarshaler_all) = true;
         option (gogoproto.sizer_all) = true;
         option (gogoproto.goproto_registration) = true;

   // Generate tests
          option (gogoproto.populate_all) = true;
          option (gogoproto.equal_all) = true;
          option (gogoproto.testgen_all) = true;,

but when i create protobuf using above code in my proto, Its generate MarshalTo(),Size(),Unmarshal ()and marshal() methods in protobuf. And gives error from protobuf file saying that its unable to detect MarshalTo() Size() function for particular types, which is not present in tendermint block,blockmeta.go

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

Added Protbuf marshal and unmarshal Compatiablity function in block.g…
f3cbdef 
…o,blockmeta.go,peerround and roundstate.go
@vlasfama vlasfama mentioned this pull request Oct 26, 2018
Closed
@vlasfama vlasfama changed the title Added Protbuf marshal and unmarshal compatiablity function in block.g… Added Protobuf marshal and unmarshal compatiablity function in block.g… Oct 26, 2018
@b00f
Copy link
Contributor

b00f commented Nov 1, 2018

@melekes Could you please check this PR?

@melekes
Copy link
Contributor

melekes commented Nov 3, 2018

And gives error from protobuf file saying that its unable to detect MarshalTo() Size() function for particular types, which is not present in tendermint block,blockmeta.go

That's weird. Could you share your .proto file? It should look similar to https://github.com/tendermint/tendermint/blob/4f61b97bbe69a08c3406e3b04148ef8fa866641d/types/proto3/block.proto.

@b00f
Copy link
Contributor

b00f commented Nov 3, 2018

my colleague is working on grpc and as he explained to me he is unable to get block information due to some functions are missed (and it looks they are necessary for marshaling and unmarshaling)
the rpc method is:

rpc GetBlock(BlockRequest) returns(BlockResponse);

message BlockRequest {
	uint64 height = 1;
}

message BlockResponse {
  bytes  BlockMeta  = 1  [(gogoproto.customtype) = "github.com/tendermint/tendermint/types.BlockMeta"];
  bytes Block  = 2  [(gogoproto.customtype) = "github.com/tendermint/tendermint/types.Block"];
}

@melekes
Copy link
Contributor

melekes commented Nov 5, 2018

You're using custom types. Now everything makes sense. Thank you

@vlasfama
Copy link
Contributor Author

vlasfama commented Nov 19, 2018

@melekes Hi, any update on GRPC for using custom types in proto.

@codecov-io
Copy link

codecov-io commented Nov 21, 2018

Codecov Report

Merging #2705 into develop will decrease coverage by 0.23%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2705      +/-   ##
===========================================
- Coverage    61.72%   61.49%   -0.24%     
===========================================
  Files          207      207              
  Lines        16942    16865      -77     
===========================================
- Hits         10458    10371      -87     
- Misses        5622     5631       +9     
- Partials       862      863       +1
Impacted Files Coverage Δ
consensus/state.go 75.93% <0%> (-3.06%) ⬇️
consensus/reactor.go 71.09% <0%> (-0.91%) ⬇️
tools/tm-monitor/monitor/network.go 88.5% <0%> (-0.75%) ⬇️
mempool/mempool.go 74.5% <0%> (+0.32%) ⬆️
privval/tcp_server.go 81.42% <0%> (+2.85%) ⬆️
privval/ipc_server.go 73.58% <0%> (+3.77%) ⬆️

ebuchman
ebuchman reviewed Nov 26, 2018
View reviewed changes
Copy link
Contributor

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding a godoc comment to each of these methods stating the interface they satisfy? Even if it's the same comment on each method, ie. // Implements XXX interface for Protobuf compatibility or something, whatever XXX is.

consensus/types/peer_round_state.go
return cdc.MarshalBinaryBare(ps)
}

func (ps *PeerRoundState) MarshalTo(data []byte) (int, error) {
Copy link
Contributor

@ebuchman ebuchman Nov 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea that Size() is called before calling MarshalTo to ensure the data buffer is correctly sized?

Copy link
Contributor Author

@vlasfama vlasfama Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ebuchman, yes its used to check the size/len of the buffer.

types/block.go
}
// Protobuf Compatiablity

func (bc *Block) Unmarshal(bs []byte) error {
Copy link
Contributor

@ebuchman ebuchman Nov 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be moved up to where the rest of the Block methods live

Copy link
Contributor Author

@vlasfama vlasfama Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ebuchman, I have update the methods #2918, Sorry for inconvenience, I created new pull request due to changes occurred in Nodeinfo.go. Kindly request you verify it.

@vlasfama vlasfama mentioned this pull request Nov 27, 2018
Merged
@melekes melekes closed this Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@melekes melekes Awaiting requested review from melekes

@xla xla Awaiting requested review from xla

1 more reviewer

@ebuchman ebuchman ebuchman left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vlasfama @b00f @melekes @codecov-io @ebuchman