Types and refactoring for ACP-77 ValidatorUptime messages #1292
Conversation
…bs/subnet-evm into always-sign-uptime
| @@ -142,15 +157,23 @@ func (b *backend) AddMessage(unsignedMessage *avalancheWarp.UnsignedMessage) err | |||
| return nil | |||
| } | |||
|
|
|||
| func (b *backend) GetMessageSignature(messageID ids.ID) ([bls.SignatureLen]byte, error) { | |||
| func (b *backend) GetMessageSignature(unsignedMessage *avalancheWarp.UnsignedMessage) ([bls.SignatureLen]byte, error) { | |||
There was a problem hiding this comment.
why we replaced messageID with unsignedMessage? it seems we only use it for the messageID.
There was a problem hiding this comment.
it is so the backend can sign arbitrary messages on the fly (it will need to inspect their contents, such as for validator uptime message, so knowing the ID aka hash is not enough).
I didn't want to make another struct that has access to the bls signer key.
Do you suggest a different code organization here?
warp/backend.go
Outdated
| log.Debug("Getting warp message from backend", "messageID", messageID) | ||
| if sig, ok := b.messageSignatureCache.Get(messageID); ok { | ||
| return sig, nil | ||
| } | ||
|
|
||
| unsignedMessage, err := b.GetMessage(messageID) | ||
| var err error | ||
| for _, v := range append(b.messageValidators, b) { |
There was a problem hiding this comment.
do we expect other interfaces? Rather than adding interfaces via AddMessageValidator should message payloads define their own validateMessage function?
warp/backend.go
Outdated
| // GetMessageSignature returns the signature of the requested message hash. | ||
| GetMessageSignature(messageID ids.ID) ([bls.SignatureLen]byte, error) | ||
| // AddMessageValidator adds a validator to the backend. The backend will sign | ||
| // messages that pass any of the validators, in addition to those known in the db. |
There was a problem hiding this comment.
IMHO it's kind-of odd to expect message to be validated if "any" of these validators pass. Maybe I did not fully understand the case but shouldn't we just need to have validators on payload types rather than adding them to here?
|
|
||
| // Signable is an optional interface that payloads can implement to allow | ||
| // on-the-fly signing of incoming messages by the warp backend. | ||
| type Signable interface { |
There was a problem hiding this comment.
I've made this an optional interface but we can make it required and have non-implementers always return an error if there is a preference for that
There was a problem hiding this comment.
how do we make it required?
|
|
||
| // Signable is an optional interface that payloads can implement to allow | ||
| // on-the-fly signing of incoming messages by the warp backend. | ||
| type Signable interface { |
There was a problem hiding this comment.
how do we make it required?
| // Signable is an optional interface that payloads can implement to allow | ||
| // on-the-fly signing of incoming messages by the warp backend. | ||
| type Signable interface { | ||
| VerifyMesssage(sourceAddress []byte) error |
| addressedCall, err := payload.ParseAddressedCall(unsignedMessage.Payload) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse unknown message as AddressedCall: %w", err) | ||
| } |
There was a problem hiding this comment.
what happens if that message type is not supposed to comfort any types. i.e a supposed to be a pure addressedcall without any specific type? (like current teleporter messages)
| } | ||
|
|
||
| // Check if the message should be signed according to its type | ||
| if err := signable.VerifyMesssage(addressedCall.SourceAddress); err != nil { |
There was a problem hiding this comment.
we don't need to sort it out right not but seems we might need a handler to take different interfaces (VM, providers etc). One example could be the uptime request needs the uptime manager (and/or the state) to verify the message.
Why this should be merged
Types and refactoring for ACP-77 ValidatorUptime messages
How this works
Refactors the warp backend to sign messages that return true for a
MessageHandlerinterface.How this was tested
CI
How is this documented
Not yet