privval: add ctx to privval interface#6240
Conversation
…/tendermint into marko/privval_context
Codecov Report
@@ Coverage Diff @@
## master #6240 +/- ##
==========================================
+ Coverage 60.75% 60.80% +0.04%
==========================================
Files 277 277
Lines 25921 25937 +16
==========================================
+ Hits 15749 15771 +22
- Misses 8533 8536 +3
+ Partials 1639 1630 -9
|
tychoish
left a comment
There was a problem hiding this comment.
In short: "yay contexts!"
| pubKey, err := pv.GetPubKey() | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), ctxTimeout) | ||
| defer cancel() |
There was a problem hiding this comment.
I think eventually there should probably be one context.Context object per-process invocation and that all contexts should be descended from (to support being able to cancel all of the contexts from a signal handler, or some such,) rather than making these one off contexts.
Having said that, (of course), this is really fine and I don't have any functional changes to this patch, except that I'd change context.Background() to context.TODO() to indicate an intention to eventually move toward a single context tree.
There was a problem hiding this comment.
Changed the backgrounds, not in tests, to TODO(). Thanks for the pointer
alexanderbez
left a comment
There was a problem hiding this comment.
Changes look good, I left a few comments.
consensus/state.go
Outdated
| // set a hard timeout for 2 seconds. This helps in avoiding blocking of the remote signer connection | ||
| ctx, cancel := context.WithTimeout(context.TODO(), 2*time.Second) |
There was a problem hiding this comment.
Are we sure 2 seconds is sufficient. Do we have any heuristics for this?
There was a problem hiding this comment.
We dont have any data, I can test different values but with gRPC 2 seconds would be more than enough.
There was a problem hiding this comment.
In testing this works fine. Not sure how to get results to build more confidence. I can leave a comment mentioning this is double the default time of prevote/precommit timeouts.
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Description
context.Contextto Privval interfaceThis pr does not introduce context into our custom privval connection protocol because this will be removed in the next release. When this pr is released.