privval: Add IPCPV and fix SocketPV by hendrikhofstadt · Pull Request #2568 · tendermint/tendermint

hendrikhofstadt · 2018-10-07T14:24:05Z

Ref: #2563

I added IPC as an unencrypted alternative to SocketPV.

Besides I fixed the following aspects of SocketPV:

Added locking since we are operating on a single socket
The connection deadline is extended every time a successful packet exchange happens; otherwise the connection would always die permanently x seconds after the connection was established.
Added a ping/heartbeat mechanism to keep the connection alive; native TCP keepalives do not work in this use-case

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

codecov-io · 2018-10-07T18:07:34Z

Codecov Report

Merging #2568 into develop will increase coverage by 0.07%.
The diff coverage is 77.07%.

@@            Coverage Diff             @@
##           develop   #2568      +/-   ##
==========================================
+ Coverage    61.33%   61.4%   +0.07%     
==========================================
  Files          203     207       +4     
  Lines        16781   16846      +65     
==========================================
+ Hits         10292   10344      +52     
- Misses        5621    5627       +6     
- Partials       868     875       +7

Impacted Files	Coverage Δ
node/node.go	`66.05% <0%> (ø)`	⬆️
privval/wire.go	`100% <100%> (ø)`	⬆️
privval/tcp_socket.go	`92% <100%> (ø)`
privval/ipc_server.go	`64.15% <64.15%> (ø)`
privval/tcp.go	`71.26% <71.26%> (ø)`
privval/tcp_server.go	`78.57% <78.57%> (ø)`
privval/ipc.go	`80% <80%> (ø)`
privval/remote_signer.go	`82.85% <82.85%> (ø)`
libs/events/events.go	`93.2% <0%> (-4.86%)`	⬇️
libs/db/remotedb/remotedb.go	`36.84% <0%> (-4.69%)`	⬇️
... and 8 more

xla

Hej @SLAMPER, thanks a lot for taking the time and putting together this changeset. Most of it should be good as is.

The RemoterSignerClient in its current form doesn't seem to be fit in entirely. I understand that it is the shared abstraction so the actual message handling doesn't need to be duplciated. What strikes me as odd that it is another piece that implements BaseService, besides that I firmly believe we need to drop the service idea as means for lifecycle management, we should not nest those services further. Ideally the RemoteSignerClient is stateless in nature, not shared and concerns like heartbeating (calling Ping) is handled by the wrapping implementation which already has to take care of listener setup and teardown and would be the perfect place to also have a ticker running which gets cleaned up.

Another issue we should address is to carefully craft and return errors so callers gain more control. Let me know if you need references.

privval/ipc.go

privval/ipc_test.go

xla · 2018-10-08T08:46:43Z

privval/rs_client.go

+	connHeartbeat time.Duration
+
+	conn       net.Conn
+	lock       sync.Mutex


What is the reason for the lock? net.Conn is threadsafe.

The idea was that if the KMS does asynchronous receive the following could happen:

TM sends a sign request.
TM send a getpubkey request.

The KMS handles both but for whichever reason it finishes the getpubkey request first.

Then the sign method would receive the packet for the getpubkey then since we have no package IDs etc.

As long as the KMS side does proper synchronizing this is not an issue but we should take care of this here.

Or do I have misconceptions about how this works ?

xla · 2018-10-08T08:48:58Z

privval/rs_client.go

+	if resp.Error != nil {
+		return fmt.Errorf("remote error occurred: code: %v, description: %s",
+			resp.Error.Code,
+			resp.Error.Description)


Needs proper error type.

Odd formatting, should be in the form of:

return fmt.Errorf( "remote error occurred: code: %v, description: %s", resp.Error.Code, resp.Error.Description, )

I will create a dedicated error type.

privval/rs_client.go

privval/socket.go

xla · 2018-10-08T09:00:18Z

@liamsi Could you review the proposed heartbeat messages in this PR?

liamsi · 2018-10-08T14:05:23Z

@liamsi Could you review the proposed heartbeat messages in this PR?

They are sane in the sense that they do the job: every connHeartbeat period a PingRequest is sent to keep the connection alive. What I found confusing is that I would have expected the SignHeartbeatRequest to be used exactly for this purpose though. Now we have two "hearbeat messages", one which is signed (and holds info about the current round/height etc) and one which is a simple (empty) registered amino message. Am I the only one who finds this confusing?

hendrikhofstadt · 2018-10-08T14:37:54Z

@liamsi
While the pings I implemented are there to maintain and check the health of the connection, the Heartbeat you mention is an part of consensus and imho should not be used for connection maintenance. Not only because it is a part of the consensusreactor and not the connection but also because it might not be suited for every connection type.

Also the pings are sent regularly at fixed intervals opposed to the Heartbeat (at least from what I have read).

liamsi · 2018-10-09T08:06:22Z

Thanks for the prompt reply! Although you are right and we do not have a go-routine or alike which periodically sends Heartbeat messages, I thought the name alone was already an indicator that it is meant to be send periodically (fixed intervals). I might be wrong though (cc @ebuchman, @milosevic). If the orig. heartbeat message is meant to be sent periodically, would we still need an additional message to keep the connection alive? For which connection types would it be unsuited?

Although the reactor deals with a ProposalHeartbeatMessage, the core consensus doesn't know about the heartbeat either (at least the spec of the core consensus doesn't explicitly mention a heartbeat message).

hendrikhofstadt · 2018-10-09T08:59:02Z

@liamsi
Thanks for explaining more in-depth what you meant.

What I saw was that the heartbeats are only sent if waitForTxs is true.

tendermint/consensus/state.go

Lines 783 to 787 in cd172ac

    
           if waitForTxs { 
        
           	if cs.config.CreateEmptyBlocksInterval > 0 { 
        
           		cs.scheduleTimeout(cs.config.EmptyBlocksInterval(), height, round, cstypes.RoundStepNewRound) 
        
           	} 
        
           	go cs.proposalHeartbeat(height, round)

A connection relevant heartbeat/keepalive should not rely on a consensus reactor setting but have to sole purpose to maintain the connection also the period should be deterministic/fixed which the proposeHeartbeat is not from what I could see.

liamsi · 2018-10-09T09:23:57Z

Thanks again. Yeah, I was expecting that any remote signer implementation would periodically send (and sign) the following messages instead (not the ProposalHeartbeatMessages used in the reactor / consensus state):

tendermint/privval/socket.go

Lines 571 to 579 in 4b2bf02

    
           // SignHeartbeatRequest is a PrivValidatorSocket message containing a Heartbeat. 
        
           type SignHeartbeatRequest struct { 
        
           	Heartbeat *types.Heartbeat 
        
           } 
        
           type SignedHeartbeatResponse struct { 
        
           	Heartbeat *types.Heartbeat 
        
           	Error     *RemoteSignerError 
        
           }

If this isn't the case and those do not work for all connection types (?), you are absolutely right and we'd probably need an actual heartbeat message (as the one you've introduced).

ebuchman · 2018-10-11T14:06:03Z

@SLAMPER do you mind if we merge #2548 first to get the small fix out of the way?

hendrikhofstadt · 2018-10-11T14:27:43Z

@ebuchman
No. I think it makes sense to merge your fix first 👍 I did not seen it when I created this PR.

ebuchman · 2018-10-11T17:56:15Z

Cool thanks - merged

privval/ipc.go

privval/ipc_test.go

privval/remote_signer.go

privval/tcp.go

hendrikhofstadt · 2018-10-12T09:02:08Z

Btw this is still WIP since we decided to implement a simple MAC auth in IPCVal #2563

ValarDragon · 2018-10-14T19:42:15Z

You could add the MAC in a separate PR :)

melekes · 2018-10-16T06:18:29Z

@SLAMPER will you have time to address my comments? After these are fixed, we can merge this PR.

hendrikhofstadt · 2018-10-16T08:36:00Z

@melekes I will do so today and then we can merge 👍

melekes

🍰 🌮 🍉

xla

👍 🍡

Thanks so much @SLAMPER for the great change-set and proper improvement of the package. The sustained collaboration on this PR over the last weeks was inspiring. To move forward I captured a smalls et of follow-ups in #2651 and will probably pick up myself shortly.

xla · 2018-10-17T08:26:29Z

Follow-up in #2651

hendrikhofstadt requested review from ebuchman, melekes and xla as code owners October 7, 2018 14:24

hendrikhofstadt changed the title ~~[WIP] Add IPCPV and fix SocketPV~~ WIP: Add IPCPV and fix SocketPV Oct 7, 2018

hendrikhofstadt changed the title ~~WIP: Add IPCPV and fix SocketPV~~ [WIP] Add IPCPV and fix SocketPV Oct 7, 2018

hendrikhofstadt force-pushed the develop branch 3 times, most recently from 9a954bb to 3e95047 Compare October 7, 2018 18:05

hendrikhofstadt changed the title ~~[WIP] Add IPCPV and fix SocketPV~~ [R4R] Add IPCPV and fix SocketPV Oct 7, 2018

xla self-assigned this Oct 8, 2018

xla changed the title ~~[R4R] Add IPCPV and fix SocketPV~~ privval: Add IPCPV and fix SocketPV Oct 8, 2018

xla suggested changes Oct 8, 2018

View reviewed changes

xla requested a review from liamsi October 8, 2018 08:59

xla mentioned this pull request Oct 8, 2018

privval: set deadline in readMsg #2548

Merged

amrali mentioned this pull request Oct 11, 2018

IPC: Unix domain socket support tendermint/tmkms#63

Merged

melekes reviewed Oct 12, 2018

View reviewed changes

xla changed the title ~~privval: Add IPCPV and fix SocketPV~~ [WIP] privval: Add IPCPV and fix SocketPV Oct 15, 2018

hendrikhofstadt added 6 commits October 17, 2018 00:02

Extend the SecureConn socket to extend its deadline

cf77980

Add locking & ping/heartbeat packets to SocketPV

2e4cae1

Implement IPC PV and abstract socket signing

03c1fd7

Refactored IPC and SocketPV

ec299c2

Implement @melekes comments

45cd8eb

Fixes to rebase

b42d258

hendrikhofstadt force-pushed the develop branch from d10715a to b42d258 Compare October 16, 2018 22:05

melekes approved these changes Oct 17, 2018

View reviewed changes

melekes changed the title ~~[WIP] privval: Add IPCPV and fix SocketPV~~ privval: Add IPCPV and fix SocketPV Oct 17, 2018

xla mentioned this pull request Oct 17, 2018

privval: Follow-up to #2568 #2651

Closed

3 tasks

xla approved these changes Oct 17, 2018

View reviewed changes

xla merged commit f60713b into tendermint:develop Oct 17, 2018

liamsi mentioned this pull request Oct 17, 2018

Add Ping(Request | Response) message handling tendermint/tmkms#70

Closed

liamsi mentioned this pull request Dec 18, 2018

privval: remove GetAddress(), memoize pubkey, don't panic on GetPubKey #2948

Merged

4 tasks

Conversation

hendrikhofstadt commented Oct 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xla Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

hendrikhofstadt Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

xla Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

hendrikhofstadt Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xla commented Oct 8, 2018

Uh oh!

liamsi commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hendrikhofstadt commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liamsi commented Oct 9, 2018

Uh oh!

hendrikhofstadt commented Oct 9, 2018

Uh oh!

liamsi commented Oct 9, 2018

Uh oh!

ebuchman commented Oct 11, 2018

Uh oh!

hendrikhofstadt commented Oct 11, 2018

Uh oh!

ebuchman commented Oct 11, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hendrikhofstadt commented Oct 12, 2018

Uh oh!

ValarDragon commented Oct 14, 2018

Uh oh!

melekes commented Oct 16, 2018

Uh oh!

hendrikhofstadt commented Oct 16, 2018

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

xla commented Oct 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

hendrikhofstadt commented Oct 7, 2018 •

edited

Loading

codecov-io commented Oct 7, 2018 •

edited

Loading

liamsi commented Oct 8, 2018 •

edited

Loading

hendrikhofstadt commented Oct 8, 2018 •

edited

Loading