Skip to content

Conversation

StephenButtolph
Copy link
Contributor

@StephenButtolph StephenButtolph commented Oct 2, 2025

Why this should be merged

Currently warp_getMessageAggregateSignature and warp_getBlockAggregateSignature incorrectly override the subnetID twice.

Specifically, the intended behavior is:

  • If the subnetID is not overridden, use this chain's validator set
    • This is the "normal" case as this chain is sending the message.
  • If the subnetID is overridden, use the validator set requested by the caller
    • This allows the C-chain's APIs to be used to generate a signature for a subnet which is using their own validator set to verify the message.

This is almost what the logic does currently, but there is a weird edge case. If the subnetID is overridden to use the primary network's validator set and this chain has enabled the optimization to use the local validator set for the primary network, then the subnetID is overridden for a second time and the chain's validator set will be used.

We should not override the subnetID based on this chain's configuration. If the caller overrides the subnetID, that override should be honored.

This edge case has likely never been hit, because no one would reasonably override the validator set to the primary network, as that would just make the message more expensive to issue.

How this works

Removes the second override along with the resulting dead code.

How this was tested

Existing CI still passes. I think this API is planned on being removed at some point in favor of the standalone relayer implementation, so I didn't add a regression test here.

Need to be documented?

No.

Need to update RELEASES.md?

Probably not? I doubt anyone has hit this.

@StephenButtolph StephenButtolph added bug Something isn't working acp181 labels Oct 2, 2025
@StephenButtolph StephenButtolph self-assigned this Oct 2, 2025
@StephenButtolph StephenButtolph marked this pull request as ready for review October 2, 2025 18:52
@StephenButtolph StephenButtolph requested a review from a team as a code owner October 2, 2025 18:52
Copy link
Collaborator

@ARR4N ARR4N left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're gonna hate me for asking, but why doesn't this change break any existing tests nor require any new ones to cover it? It would also really help understand said edge case because I'm confused by the description (granted I'm not familiar with this code and it's late here).

EDIT: I missed the description about why you're not adding a regression test.

@StephenButtolph StephenButtolph added this pull request to the merge queue Oct 3, 2025
Merged via the queue into master with commit 81b31df Oct 3, 2025
9 checks passed
@StephenButtolph StephenButtolph deleted the cleanup-warp-validator-set-logic branch October 3, 2025 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acp181 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants