-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Signature Aggregator ACP-118 updates #422
Signature Aggregator ACP-118 updates #422
Conversation
Co-authored-by: cam-schultz <78878559+cam-schultz@users.noreply.github.com> Signed-off-by: Ian Suvak <ian.suvak@avalabs.org>
Co-authored-by: Geoff Stuart <geoff.vball@gmail.com> Signed-off-by: Ian Suvak <ian.suvak@avalabs.org>
@@ -88,6 +88,9 @@ func (h *RelayerExternalHandler) HandleInbound(_ context.Context, inboundMessage | |||
zap.Stringer("from", inboundMessage.NodeID()), | |||
) | |||
if inboundMessage.Op() == message.AppResponseOp || inboundMessage.Op() == message.AppErrorOp { | |||
if inboundMessage.Op() == message.AppErrorOp { | |||
h.log.Debug("Received AppError message", zap.Stringer("message", inboundMessage.Message())) |
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.
This is messy checking but I used this to see error codes received from validator nodes which we don't log anywhere right now
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.
LGTM! Happy to see it working!
I think you may have accidentally included an update to the |
Thanks, I had teleporter updated to a branch commit that was not necessary so I downgraded it v1.0.4 which is what it is on |
|
||
return signature, true | ||
blsSig := blsSignatureBuf{} | ||
copy(blsSig[:], signature[:]) |
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.
Should we have a check higher up that the length of signature
is the expected length?
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.
Possibly, we do call bls.SignatureFromBytes
and bls.Verify
on it's result so presumably those wouldn't work if it wasn't correct lenght but an explicit check wouldn't hurt
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.
Addressed in 811c97a
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.
LGTM
This should pass the tests but not necessarily be merged into the PR until all of the required commits make it into default branches. Right now the main outstanding one would be the subnet-evm that references newer avalanchego commit that includes latest coreth changes.The commits have been updated to reference default branches ones now.
Includes a bugfix on the validation logic.
I'm open to suggestions on cleaner prefix packing and to avoid copying into
blsSignatureBuf
if anyone has ideas.