-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cache validator signatures #415
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
cd8e923
cache SignatureRequest responses
feuGeneA 53a0694
remove `logger.Debug` statements
feuGeneA 2ca2006
re-word comment
feuGeneA acb3e80
add return after writing JSON error
feuGeneA c0a6a0a
specify sizes of uints for cache size variables
feuGeneA b8862e7
simplify `Cache.Add()`
feuGeneA 460b46f
Merge branch 'main' into signature-aggregation-api-cache
feuGeneA d77300b
Cache.Get: on miss, return nil; don't allocate
feuGeneA 7196197
Simplify `Cache.Add()` further
feuGeneA 6be5cd5
add metrics for cache hits and misses
feuGeneA 4cb1c0c
Merge branch 'main' into signature-aggregation-api-cache
feuGeneA 70b7722
count cache misses based on nodes queried
feuGeneA fdc53c2
rm mistakenly committed debug log statement
feuGeneA 402d8ca
Apply suggestions from code review
feuGeneA aa631b6
rm `cacheKey` and `cacheValue`
feuGeneA fb73eea
Merge branch 'main' into signature-aggregation-api-cache
feuGeneA 0d6da49
don't count cache misses on every retry
feuGeneA File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
package cache | ||
|
||
import ( | ||
"math" | ||
|
||
"github.com/ava-labs/avalanchego/ids" | ||
"github.com/ava-labs/avalanchego/utils/crypto/bls" | ||
"github.com/ava-labs/avalanchego/utils/logging" | ||
lru "github.com/hashicorp/golang-lru/v2" | ||
"github.com/pingcap/errors" | ||
"go.uber.org/zap" | ||
) | ||
|
||
type Cache struct { | ||
cam-schultz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
logger logging.Logger | ||
|
||
// map of warp message ID to a map of public keys to signatures | ||
signatures *lru.Cache[ids.ID, map[PublicKeyBytes]SignatureBytes] | ||
} | ||
|
||
type PublicKeyBytes [bls.PublicKeyLen]byte | ||
type SignatureBytes [bls.SignatureLen]byte | ||
|
||
func NewCache(size uint64, logger logging.Logger) (*Cache, error) { | ||
if size > math.MaxInt { | ||
return nil, errors.New("cache size too big") | ||
} | ||
|
||
signatureCache, err := lru.New[ids.ID, map[PublicKeyBytes]SignatureBytes](int(size)) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return &Cache{ | ||
signatures: signatureCache, | ||
logger: logger, | ||
}, nil | ||
} | ||
|
||
func (c *Cache) Get(msgID ids.ID) (map[PublicKeyBytes]SignatureBytes, bool) { | ||
cachedValue, isCached := c.signatures.Get(msgID) | ||
|
||
if isCached { | ||
c.logger.Debug("cache hit", zap.Stringer("msgID", msgID)) | ||
return cachedValue, true | ||
} else { | ||
c.logger.Debug("cache miss", zap.Stringer("msgID", msgID)) | ||
return nil, false | ||
} | ||
} | ||
|
||
func (c *Cache) Add( | ||
msgID ids.ID, | ||
pubKey PublicKeyBytes, | ||
signature SignatureBytes, | ||
) { | ||
var ( | ||
sigs map[PublicKeyBytes]SignatureBytes | ||
ok bool | ||
) | ||
if sigs, ok = c.Get(msgID); !ok { | ||
sigs = make(map[PublicKeyBytes]SignatureBytes) | ||
} | ||
sigs[pubKey] = signature | ||
c.signatures.Add(msgID, sigs) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning behind incrementing based on the number of signatures (i.e. the cache value) rather than the number of key hits? Signature-based metrics would make it more difficult to identify appropriate cache sizes based on usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not my PR but not sure what do you mean by this. The
signatureMap
here is just the number of key hits from my understanding.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache is keyed by
warpMessageID
, but we're adding metrics based on the size of the corresponding value. If the Warp message is signed by 1 or 1000 validators, it's still a cache hit or miss, but the measuring the number of signatures would tell a wildly different story than measuring the actual cache hit/miss rate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed on standup, I left the metric in place on a per-signature basis. also, I changed the "cache miss" metric to reflect the number of signatures that need to be requested from the network, in 70b7722