fix: unpack errors on ReferenceBlockAttestation

[maharifu]

Related Github tickets

• Closes [BUG] No concrete type error in palomad VolumeFi/paloma#1740

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

• test coverage exists or has been added/updated
• tested in a private testnet

Breaking changes

• I have checked my code for breaking changes
• If there are breaking changes, there is a supporting migration.

[byte-bandit]

Take a look at ValidatorBalanceAttestation messages. They have the same constraints (no relaying), but the logic is handled differently (by populating some PublicAccessData I believe).

How about using the same approach here, so we don't end up with n solutions for the same problem again?

[maharifu]

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]

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]

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]

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 :)

[byte-bandit]

Noice