search

palomachain / paloma

The fast blockchain messenger protocol
Apache License 2.0
290 stars 136 forks source link

fix: unpack errors on ReferenceBlockAttestation #1199

Closed maharifu closed 2 months ago

maharifu commented 2 months ago

Related Github tickets

Background

ReferenceBlockAttestation messages currently do not need an assignee, however they are still queried for messages to relay, leading to unpack error messages in palomad logs. By first checking for publicAccessData, ReferenceBlockAttestation messages are filtered before we try to unpack them as TurnstoneMsg and avoid the errors.

Testing completed

Breaking changes

maharifu commented 2 months ago

I think ValidatorBalancesAttestation works by having Assignee defined in the proto although it doesn't need it as well. This makes it comply with the TurnstoneMsg interface.

I didn't want to add the field to ReferenceBlockAttestation as it's not needed. We can remove it from ValidatorBalancesAttestation and implement an empty function as well, but didn't want to change it unnecessarily. Do you think we should remove it?

edit

We set PublicAccessData in ReferenceBlockAttestation messages, but pigeon still queries all queues for relay here. I think we could also address this in pigeon by splitting SupportedQueues but that also doesn't feel like addressing the root of the problem.

byte-bandit commented 2 months ago

I think ValidatorBalancesAttestation works by having Assignee defined in the proto although it doesn't need it as well. This makes it comply with the TurnstoneMsg interface.

I didn't want to add the field to ReferenceBlockAttestation as it's not needed. We can remove it from ValidatorBalancesAttestation and implement an empty function as well, but didn't want to change it unnecessarily. Do you think we should remove it?

edit

We set PublicAccessData in ReferenceBlockAttestation messages, but pigeon still queries all queues for relay here. I think we could also address this in pigeon by splitting SupportedQueues but that also doesn't feel like addressing the root of the problem.

Pigeon will still query it, but Paloma won't hand the message out for delivery, since the PublicAccessData is already set. See GetMessagesForRelaying in the consensus keeper. Essentially, setting the PublicAccessData field to anything but nil should prevent this message from ever being handed over to the assignee. It will also trigger the "attestation" stage of the message lifecycle, since Paloma will start handing it over to all Pigeons for attestation once either PublicAccessData or ErrorData are filled.

To be honest, I think it's more in line with the current architecture to stick with this approach. Can you see any reasons why you'd want to move away from it?

Splitting queues is likely something we want to explore in the future, but out of scope for this one.

maharifu commented 2 months ago

I think ValidatorBalancesAttestation works by having Assignee defined in the proto although it doesn't need it as well. This makes it comply with the TurnstoneMsg interface. I didn't want to add the field to ReferenceBlockAttestation as it's not needed. We can remove it from ValidatorBalancesAttestation and implement an empty function as well, but didn't want to change it unnecessarily. Do you think we should remove it? edit We set PublicAccessData in ReferenceBlockAttestation messages, but pigeon still queries all queues for relay here. I think we could also address this in pigeon by splitting SupportedQueues but that also doesn't feel like addressing the root of the problem.

Pigeon will still query it, but Paloma won't hand the message out for delivery, since the PublicAccessData is already set. See GetMessagesForRelaying in the consensus keeper. Essentially, setting the PublicAccessData field to anything but nil should prevent this message from ever being handed over to the assignee. It will also trigger the "attestation" stage of the message lifecycle, since Paloma will start handing it over to all Pigeons for attestation once either PublicAccessData or ErrorData are filled.

To be honest, I think it's more in line with the current architecture to stick with this approach. Can you see any reasons why you'd want to move away from it?

Splitting queues is likely something we want to explore in the future, but out of scope for this one.

We have that, and it's working. I agree we should keep it.

The current issue is that in GetMessagesForRelaying we will try to unpack these messages as TurnstoneMsg here, but this will fail as we don't have the GetAssignee() method, as there's no Assignee field. Everything still works fine, but we're getting a bunch of errors in the logs, which is what I'm trying to avoid here.

I realize now a simpler solution could be to push this check for PublicAccessData to happen before checking the assignee. This would filter these messages before we try to unpack them as TurnstoneMsg and get rid of the errors as well. Do you see an issue with doing this?

byte-bandit commented 2 months ago

I think ValidatorBalancesAttestation works by having Assignee defined in the proto although it doesn't need it as well. This makes it comply with the TurnstoneMsg interface. I didn't want to add the field to ReferenceBlockAttestation as it's not needed. We can remove it from ValidatorBalancesAttestation and implement an empty function as well, but didn't want to change it unnecessarily. Do you think we should remove it? edit We set PublicAccessData in ReferenceBlockAttestation messages, but pigeon still queries all queues for relay here. I think we could also address this in pigeon by splitting SupportedQueues but that also doesn't feel like addressing the root of the problem.

Pigeon will still query it, but Paloma won't hand the message out for delivery, since the PublicAccessData is already set. See GetMessagesForRelaying in the consensus keeper. Essentially, setting the PublicAccessData field to anything but nil should prevent this message from ever being handed over to the assignee. It will also trigger the "attestation" stage of the message lifecycle, since Paloma will start handing it over to all Pigeons for attestation once either PublicAccessData or ErrorData are filled. To be honest, I think it's more in line with the current architecture to stick with this approach. Can you see any reasons why you'd want to move away from it? Splitting queues is likely something we want to explore in the future, but out of scope for this one.

We have that, and it's working. I agree we should keep it.

The current issue is that in GetMessagesForRelaying we will try to unpack these messages as TurnstoneMsg here, but this will fail as we don't have the GetAssignee() method, as there's no Assignee field. Everything still works fine, but we're getting a bunch of errors in the logs, which is what I'm trying to avoid here.

I realize now a simpler solution could be to push this check for PublicAccessData to happen before checking the assignee. This would filter these messages before we try to unpack them as TurnstoneMsg and get rid of the errors as well. Do you see an issue with doing this?

I think it sounds like a great idea :)