Skip to content

[R4R] Add adr-034: PrivValidator file structure#2751

Merged
ebuchman merged 4 commits intotendermint:developfrom
yutianwu:feature/adr-034
Nov 11, 2018
Merged

[R4R] Add adr-034: PrivValidator file structure#2751
ebuchman merged 4 commits intotendermint:developfrom
yutianwu:feature/adr-034

Conversation

@yutianwu
Copy link
Contributor

@yutianwu yutianwu commented Nov 3, 2018

ADR 034: PrivValidator file structure

Changelog

03-11-2018: Initial Draft

Context

For now, the PrivValidator file priv_validator.json contains mutable and immutable parts.
Even in an insecure mode which does not encrypt private key on disk, it is reasonable to separate
the mutable part and immutable part.

References:
#1181
#2657
#2313

Proposed Solution

We can split mutable and immutable parts with two structs:

// PVKey stores the immutable part of PrivValidator
type PVKey struct {
	Address types.Address  `json:"address"`
	PubKey  crypto.PubKey  `json:"pub_key"`
	PrivKey crypto.PrivKey `json:"priv_key"`

	filePath string
	mtx      sync.Mutex
}

// PVState stores the mutable part of PrivValidator
type PVState struct {
	LastHeight    int64        `json:"last_height"`
	LastRound     int          `json:"last_round"`
	LastStep      int8         `json:"last_step"`
	LastSignature []byte       `json:"last_signature,omitempty"`
	LastSignBytes cmn.HexBytes `json:"last_signbytes,omitempty"`

	filePath string
	mtx      sync.Mutex
}

Then we can combine PVKey with PVState and will get the original FilePV.

type FilePV struct {
	Key   PVKey
	State PVState
}

What we need to do next is changing the methods of FilePV.

Status

Draft.

Consequences

Positive

  • separate the mutable and immutable of PrivValidator

Negative

  • need to add more config for file path

Neutral

@codecov-io
Copy link

codecov-io commented Nov 3, 2018

Codecov Report

Merging #2751 into develop will decrease coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2751      +/-   ##
===========================================
- Coverage    62.29%   62.28%   -0.01%     
===========================================
  Files          212      212              
  Lines        17185    17126      -59     
===========================================
- Hits         10705    10667      -38     
+ Misses        5583     5568      -15     
+ Partials       897      891       -6
Impacted Files Coverage Δ
libs/clist/clist.go 66.66% <0%> (-1.52%) ⬇️
consensus/reactor.go 67.01% <0%> (-1.05%) ⬇️
state/errors.go 0% <0%> (ø) ⬆️
p2p/pex/pex_reactor.go 73.37% <0%> (+0.32%) ⬆️
libs/db/fsdb.go 68.47% <0%> (+0.42%) ⬆️
libs/events/events.go 98.05% <0%> (+4.85%) ⬆️
privval/ipc_server.go 69.81% <0%> (+5.66%) ⬆️

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

🍋 🍎 🍐

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.

Thanks for writing this!! We should add mention that the two parts are stored in different locations. The immutable piece should be in config, the mutable piece should be in data.

PrivKey crypto.PrivKey `json:"priv_key"`

filePath string
mtx sync.Mutex
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think we need the mtx, since this is immutable. Right?

LastRound int `json:"last_round"`
LastStep int8 `json:"last_step"`
LastSignature []byte `json:"last_signature,omitempty"`
LastSignBytes cmn.HexBytes `json:"last_signbytes,omitempty"`
Copy link
Contributor

@ebuchman ebuchman Nov 6, 2018

Choose a reason for hiding this comment

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

last_sign_bytes.

Thought maybe instead of FilePVState we call this struct LastSignInfo or LastSignState or similar and then we can remove the Last prefix from all the fields?

@yutianwu
Copy link
Contributor Author

@ebuchman , I did some changes according to you comments. Sorry for the delay, I am really busy lately.

@ebuchman ebuchman merged commit e116990 into tendermint:develop Nov 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants