Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: relayer large incoming message support #9

Merged
merged 9 commits into from
Dec 23, 2024

Conversation

tilacog
Copy link
Contributor

@tilacog tilacog commented Dec 21, 2024

Summary:

  • Update Solana deps to the next main release.
  • Make the Relayer populate a MessagePayload PDA account before executing the message, and close it afterwards.

Open topics for discussion:

  • Tracing & monitoring vs error handling (eyre::WrapErr)

@tilacog tilacog marked this pull request as ready for review December 21, 2024 21:18
@tilacog tilacog requested a review from frenzox December 21, 2024 21:18
Comment on lines -364 to +375
// this is a security check, because the relayer is a signer, we don't want to
// sign a tx where a malicious destination contract could drain the account. This is
// because the `AxelarMessagePayload` defines an interface where the accounts get
// dynamically appended, thus it could also include the relayers account.
if let Ok(decoded_payload) = AxelarMessagePayload::decode(&payload) {
let relayer_signer_acc_included = decoded_payload
.account_meta()
.iter()
.any(|acc| acc.pubkey == signer);
if relayer_signer_acc_included {
eyre::bail!(
"relayer will not execute a transaction where its own key is included"
);
}
}
validate_relayer_not_in_payload(&payload, signer)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this to a function because it was cluttering the main flow.
This step is important, so having its own function makes it a clear and explicit part of the process.

Comment on lines +18 to +20
/// Maximum size for payload chunks in bytes.
// TODO: we should either fine tune this or make this configurable
const CHUNK_SIZE: usize = 500;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this should be configured or not, given that he maximum size of a transaction is 1232 bytes1.

If we stick to send a transaction with only a single "write" instruction, we can determine the exact, optimal value for this.

Footnotes

  1. https://solana.com/docs/core/transactions#key-points

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure there's much value in allowing this to be configurable 🤔 . IMHO figuring out the optimal value and setting it here with a comment explaining how it was obtained would be a good solution.

Copy link
Contributor

@frenzox frenzox left a comment

Choose a reason for hiding this comment

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

LGTM

@tilacog tilacog merged commit 3ff3593 into main Dec 23, 2024
6 checks passed
@tilacog tilacog deleted the 585-relayer-large-message-support branch December 23, 2024 12:42
use solana_sdk::signature::Keypair;
use solana_sdk::signer::Signer as _;

use super::send_transaction;
Copy link
Member

Choose a reason for hiding this comment

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

You should use send_gateway_tx instead.
This is important for idempotency purposes, as it will check the GatewayError to figure out if we have a hard error or just a task that had already been executed.

Copy link
Member

Choose a reason for hiding this comment

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

#12 I created a new issue for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants