Skip to content
This repository has been archived by the owner on Jun 12, 2023. It is now read-only.

Add heartbeat interval to local POC key lifespan #1743

Merged

Conversation

PaulVMo
Copy link
Contributor

@PaulVMo PaulVMo commented Jun 14, 2022

This change addresses an edge case which can cause a validator to garbage collect a local poc private key before the public key is removed from the ledger leading to a failed PoC. The issue is caused by the fact that the local private key is stored with its height at the time of key/heartbeat creation while the public key is stored with the height at which the heartbeat is accepted on the blockchain. In typical cases, this differences between heartbeat creation and acceptance is only a few blocks. However, it can be longer under heavy network load or with an improperly functioning validator..

To address this case, this change extends the time that local poc key remains in the validator's key store by the validator liveliness heartbeat interval and grace period (currently 100 + 50 = 150 blocks). Assuming that all heartbeats are no more than the 150 blocks old, this will ensure PoCs are not GC'ed to early.

See related but independent blockchain-core PR 1393 that ensures heartbeats are no more than the liveliness interval + grace period old.

Copy link
Contributor

@evanmcc evanmcc left a comment

Choose a reason for hiding this comment

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

This seems reasonable. Would it be possible to just use one data store or use the "remote" height when we store this so we can unify the checking into core?

@PaulVMo
Copy link
Contributor Author

PaulVMo commented Jun 15, 2022

I think the use of a single data store for private and public keys would be complex because one is in the ledger and the other within the miner poc manager. And as far as storing the local key with the remote height, that would require the miner to listen for it's heartbeats and update the key store. Again maybe adding more complexity than needed. This change results in 1 - 2 more heartbeats worth of PoC keys its store, which really shouldn't be much added load.

@andymck , could you provide some thoughts on evan's questions as well?

@andymck
Copy link
Contributor

andymck commented Jun 15, 2022

the private keys are known only to the local miner and due to that a local store distinct from the ledger was chosen, the keys are of no interest outside of miner_poc_mgr. I dont little value in adding these keys to the same store as that of the public keys or the key proposals. Like @PaulVMo suggests it would unnecessarily complicate things, especially around key promotion and GC.

@evanmcc evanmcc merged commit 2c28990 into helium:master Aug 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants