cache validator signatures #415

feuGeneA · 2024-08-07T04:43:54Z

Why this should be merged

fixes #382
fixes #389

How this was tested

see included test modifications

signature-aggregator/aggregator/cache/cache.go

iansuvak · 2024-08-07T13:56:04Z

signature-aggregator/aggregator/cache/cache.go

+
+const (
+	initialCapacity = 1024
+	maxSize         = 1024 * 1024


MaxSize should be configurable via the application configs. We should have a sane default though

done. I'm not sure what would be sane to you though. I left this number in place. Happy to change it to whatever the team thinks.

cam-schultz

My understanding was that we wanted to cache individual BLS signatures so that we could reconstruct the aggregate signature from the cache even if the validator set changes. By caching the full signed Warp message, we skip fetching signatures of already requested messages only if the validator set hasn't changed. We also need to be careful when caching the full signed message, since it will no longer be valid after the validator set changes.

iansuvak · 2024-08-07T15:08:10Z

My understanding was that we wanted to cache individual BLS signatures so that we could reconstruct the aggregate signature from the cache even if the validator set changes. By caching the full signed Warp message, we skip fetching signatures of already requested messages only if the validator set hasn't changed. We also need to be careful when caching the full signed message, since it will no longer be valid after the validator set changes.

Indeed this is the case. We want to cache individual signatures as well as full signature only (not the message). Before returning the full signature we can query for the current validator set to confirm if it's still correct

signature-aggregator/aggregator/cache/cache.go

fixes #382

signature-aggregator/config/keys.go

now that we're caching individual signatures rather than the aggregate signature

config/config.go

signature-aggregator/aggregator/cache/cache.go

signature-aggregator/api/api.go

config/config.go

geoff-vball · 2024-08-09T18:16:39Z

signature-aggregator/aggregator/cache/cache.go

+type cacheKey ids.ID // warp message ID
+type cacheValue map[PublicKeyBytes]SignatureBytes


Can we just use the regular types directly? It seems odd to have to wrap everything before passing them in

For the public keys, we can use the regular types directly in cache.go but when the interactions happen from aggregator.go they will need to be wrapped in something either way, because warp.Validator.PublicKeyBytes is a slice, and a map key can only be a fixed length array, so even if we change to using regular types directly, we'll still have to wrap the arguments to Get and Add with [bls.PublicKeyLen]byte(...).

I'm fine with going either way on it, but at the time I wrote this it felt "cleaner" to have a name for a type that needs to be repeated several times over. (Note this is just like what's done with the type blsSignatureBuf [bls.SignatureLen]byte in aggregator.go.)

I tried, without any luck:

gene-dev 08/13 22:21:21 (signature-aggregation-api-cache) 1 ~/dev/awm-relayer$ git diff signature-aggregator/ diff --git a/signature-aggregator/aggregator/aggregator.go b/signature-aggregator/aggregator/aggregator.go index d500e82..73ee1da 100644 --- a/signature-aggregator/aggregator/aggregator.go +++ b/signature-aggregator/aggregator/aggregator.go @@ -412,7 +412,7 @@ func (s *SignatureAggregator) handleResponse( signatureMap[vdrIndex] = signature s.cache.Add( unsignedMessage.ID(), - cache.PublicKeyBytes(validator.PublicKeyBytes), + validator.PublicKeyBytes[:], cache.SignatureBytes(signature), ) accumulatedSignatureWeight.Add(accumulatedSignatureWeight, new(big.Int).SetUint64(validator.Weight)) diff --git a/signature-aggregator/aggregator/cache/cache.go b/signature-aggregator/aggregator/cache/cache.go index e0fa57d..6d7a4e8 100644 --- a/signature-aggregator/aggregator/cache/cache.go +++ b/signature-aggregator/aggregator/cache/cache.go @@ -52,7 +52,7 @@ func (c *Cache) Get(msgID ids.ID) (cacheValue, bool) { func (c *Cache) Add( msgID ids.ID, - pubKey PublicKeyBytes, + pubKey [bls.PublicKeyLen]byte, signature SignatureBytes, ) { var ( gene-dev 08/13 22:21:27 (signature-aggregation-api-cache) 0 ~/dev/awm-relayer$ scripts/build.sh Running protobuf fmt... Running protobuf lint check... Re-generating protobuf... Building AWM Relayer Version: v1.4.0-rc.2 at /home/gene/dev/awm-relayer/build/awm-relayer # github.com/ava-labs/awm-relayer/signature-aggregator/aggregator signature-aggregator/aggregator/aggregator.go:415:28: cannot use validator.PublicKeyBytes[:] (value of type []byte) as [48]byte value in argument to s.cache.Add Building Signature Aggregator Version: v1.4.0-rc.2 at /home/gene/dev/awm-relayer/signature-aggregator/build/signature-aggregator # github.com/ava-labs/awm-relayer/signature-aggregator/aggregator aggregator/aggregator.go:415:28: cannot use validator.PublicKeyBytes[:] (value of type []byte) as [48]byte value in argument to s.cache.Add

Feel free to give it a try and let me know what you prefer.

Yeah, okay I guess
type PublicKeyBytes [bls.PublicKeyLen]byte type SignatureBytes [bls.SignatureLen]byte
Is the easiest way to do that part, but I think
type cacheKey ids.ID // warp message ID type cacheValue map[PublicKeyBytes]SignatureBytes
are not necessary.

addressed in aa631b6

signature-aggregator/config/config.go

addresses review comment #415 (comment)

addresses review comments #415 (comment) and #415 (comment)

addresses review comment #415 (comment)

iansuvak · 2024-08-12T13:59:53Z

Looks like this includes functionality of #389 . I want to take a more thorough look before shipping it but looks good so far.
I'm not familiar with the differences between that and the hashicorp implementation but did you consider using the LRU cache that's defined in avalanchego?

feuGeneA · 2024-08-12T14:40:39Z

did you consider using the LRU cache that's defined in avalanchego?

No, I didn't even realize that existed. I just took Geoff's suggestion and ran with it. Happy to convert it over though if that's what we think makes sense. It should be a pretty self-contained change.

Signed-off-by: F. Eugene Aumson <feuGeneA@users.noreply.github.com>

cam-schultz

A handful of minor comments, but generally LGTM

cam-schultz · 2024-08-13T00:05:40Z

signature-aggregator/aggregator/cache/cache.go

+		return cachedValue, true
+	} else {
+		c.logger.Debug("cache miss", zap.Stringer("msgID", msgID))
+		return make(cacheValue), false


We can just return nil instead of allocating.

addressed in d77300b

cam-schultz · 2024-08-13T00:13:50Z

signature-aggregator/aggregator/cache/cache.go

+	if signatures, ok := c.Get(msgID); ok {
+		signatures[pubKey] = signature
+	} else {
+		signatures := make(cacheValue)
+		signatures[pubKey] = signature
+		c.signatures.Add(cacheKey(msgID), signatures)
+	}


This control flow is a bit awkward due to the multiple uses of signatures and the repeated assignment to the map. The following rewrite cleans it up a bit I think:

var ( sigs cacheValue ok bool ) if sigs, ok = c.Get(msgID); !ok { sigs = make(cacheValue) } sigs[pubKey] = signature c.signatures.Add(cacheKey(msgID), sigs)

I like this, thanks. addressed in 7196197

signature-aggregator/aggregator/cache/cache.go

addresses review comment #415 (comment)

signature-aggregator/aggregator/aggregator.go

cam-schultz · 2024-08-15T14:09:48Z

signature-aggregator/aggregator/aggregator.go

@@ -151,7 +151,13 @@ func (s *SignatureAggregator) CreateSignedMessage(
 				)
 			}
 		}
+		s.metrics.SignatureCacheHits.Add(float64(len(signatureMap)))


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.

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.

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.

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

signature-aggregator/metrics/metrics.go

Co-authored-by: cam-schultz <78878559+cam-schultz@users.noreply.github.com> Signed-off-by: F. Eugene Aumson <feuGeneA@users.noreply.github.com>

addresses review comment #415 (comment)

cam-schultz · 2024-08-16T15:28:55Z

signature-aggregator/aggregator/aggregator.go

+		if len(signatureMap) > 0 {
+			s.metrics.SignatureCacheMisses.Add(float64(responsesExpected))
+		}


Maybe I'm misunderstanding something, but this just seems like we're collecting the same metric multiple times (once per retry). The way you had it before seems like it achieves the stated goal of having the metric reflect the number of signatures that need to be requested from the network

you're right! thank you! addressed in 0d6da49

addresses review comment https://github.com/ava-labs/awm-relayer/pull/415/files#r1719999408

feuGeneA marked this pull request as ready for review August 7, 2024 04:55

feuGeneA requested a review from a team as a code owner August 7, 2024 04:55

feuGeneA requested review from iansuvak, geoff-vball, minghinmatthewlam, bernard-avalabs, michaelkaplan13 and cam-schultz August 7, 2024 04:55

feuGeneA force-pushed the signature-aggregation-api-cache branch 3 times, most recently from 542b851 to 546c918 Compare August 7, 2024 05:17

iansuvak reviewed Aug 7, 2024

View reviewed changes

cam-schultz reviewed Aug 7, 2024

View reviewed changes

geoff-vball reviewed Aug 7, 2024

View reviewed changes

signature-aggregator/aggregator/cache/cache.go Outdated Show resolved Hide resolved

signature-aggregator/aggregator/cache/cache.go Outdated Show resolved Hide resolved

Base automatically changed from signature-aggregation-api-metrics to main August 9, 2024 15:12

cache SignatureRequest responses

cd8e923

fixes #382

feuGeneA force-pushed the signature-aggregation-api-cache branch from 546c918 to cd8e923 Compare August 9, 2024 16:27

feuGeneA requested review from iansuvak, cam-schultz and geoff-vball August 9, 2024 16:29

github-advanced-security bot found potential problems Aug 9, 2024

View reviewed changes

signature-aggregator/config/keys.go Dismissed Show dismissed Hide dismissed

signature-aggregator/config/keys.go Dismissed Show dismissed Hide dismissed

remove logger.Debug statements

53a0694

feuGeneA changed the title ~~cache SignatureRequest responses~~ cache validator signatures Aug 9, 2024

re-word comment

2ca2006

now that we're caching individual signatures rather than the aggregate signature

geoff-vball reviewed Aug 9, 2024

View reviewed changes

feuGeneA added 3 commits August 9, 2024 21:27

add return after writing JSON error

acb3e80

addresses review comment #415 (comment)

specify sizes of uints for cache size variables

c0a6a0a

addresses review comments #415 (comment) and #415 (comment)

simplify Cache.Add()

b8862e7

addresses review comment #415 (comment)

feuGeneA requested a review from geoff-vball August 9, 2024 22:22

Merge branch 'main' into signature-aggregation-api-cache

460b46f

Signed-off-by: F. Eugene Aumson <feuGeneA@users.noreply.github.com>

cam-schultz reviewed Aug 13, 2024

View reviewed changes

feuGeneA added a commit that referenced this pull request Aug 13, 2024

Cache.Get: on miss, return nil; don't allocate

9da1266

addresses review comment #415 (comment)

feuGeneA added a commit that referenced this pull request Aug 13, 2024

Simplify Cache.Add() further

5b15c6b

addresses review comment #415 (comment)

feuGeneA added 2 commits August 13, 2024 16:28

Cache.Get: on miss, return nil; don't allocate

d77300b

addresses review comment #415 (comment)

Simplify Cache.Add() further

7196197

addresses review comment #415 (comment)

geoff-vball reviewed Aug 13, 2024

View reviewed changes

signature-aggregator/aggregator/aggregator.go Show resolved Hide resolved

feuGeneA added 2 commits August 13, 2024 22:08

add metrics for cache hits and misses

6be5cd5

Merge branch 'main' into signature-aggregation-api-cache

4cb1c0c

feuGeneA requested review from cam-schultz and geoff-vball August 13, 2024 22:32

cam-schultz reviewed Aug 15, 2024

View reviewed changes

count cache misses based on nodes queried

70b7722

feuGeneA force-pushed the signature-aggregation-api-cache branch from a7066df to 70b7722 Compare August 15, 2024 18:23

feuGeneA and others added 4 commits August 15, 2024 18:25

rm mistakenly committed debug log statement

fdc53c2

Apply suggestions from code review

402d8ca

Co-authored-by: cam-schultz <78878559+cam-schultz@users.noreply.github.com> Signed-off-by: F. Eugene Aumson <feuGeneA@users.noreply.github.com>

rm cacheKey and cacheValue

aa631b6

addresses review comment #415 (comment)

Merge branch 'main' into signature-aggregation-api-cache

fb73eea

feuGeneA requested a review from cam-schultz August 16, 2024 15:13

geoff-vball approved these changes Aug 16, 2024

View reviewed changes

cam-schultz reviewed Aug 16, 2024

View reviewed changes

don't count cache misses on every retry

0d6da49

addresses review comment https://github.com/ava-labs/awm-relayer/pull/415/files#r1719999408

feuGeneA requested a review from cam-schultz August 16, 2024 18:12

cam-schultz approved these changes Aug 16, 2024

View reviewed changes

iansuvak approved these changes Aug 16, 2024

View reviewed changes

feuGeneA merged commit 21e89af into main Aug 16, 2024
7 checks passed

feuGeneA deleted the signature-aggregation-api-cache branch August 16, 2024 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cache validator signatures #415

cache validator signatures #415

feuGeneA commented Aug 7, 2024 •

edited

Loading

iansuvak Aug 7, 2024

feuGeneA Aug 9, 2024

cam-schultz left a comment

iansuvak commented Aug 7, 2024

geoff-vball Aug 9, 2024

feuGeneA Aug 9, 2024

feuGeneA Aug 13, 2024

geoff-vball Aug 14, 2024

feuGeneA Aug 16, 2024

iansuvak commented Aug 12, 2024

feuGeneA commented Aug 12, 2024

cam-schultz left a comment

cam-schultz Aug 13, 2024

feuGeneA Aug 13, 2024

cam-schultz Aug 13, 2024

feuGeneA Aug 13, 2024

cam-schultz Aug 15, 2024

iansuvak Aug 15, 2024

cam-schultz Aug 15, 2024

feuGeneA Aug 16, 2024

cam-schultz Aug 16, 2024

feuGeneA Aug 16, 2024

		type cacheKey ids.ID // warp message ID
		type cacheValue map[PublicKeyBytes]SignatureBytes

cache validator signatures #415

cache validator signatures #415

Conversation

feuGeneA commented Aug 7, 2024 • edited Loading

Why this should be merged

How this was tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cam-schultz left a comment

Choose a reason for hiding this comment

iansuvak commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iansuvak commented Aug 12, 2024

feuGeneA commented Aug 12, 2024

cam-schultz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feuGeneA commented Aug 7, 2024 •

edited

Loading