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

Ignore Envelopes with Unkown Validators #546

Merged

Conversation

b-yap
Copy link
Contributor

@b-yap b-yap commented Aug 5, 2024

closes #544

Need to close pendulum-chain/substrate-stellar-sdk#32 before proceeding.

This will ignore validating envelopes with a different node_id.

@b-yap b-yap requested a review from a team August 5, 2024 11:11
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

The logic looks good to me in general. We also need to fix the CI errors though.

@@ -574,7 +579,8 @@ pub mod pallet {
.get_tx_set_hash()
.map_err(|_| Error::<T>::FailedToComputeNonGenericTxSetContentHash)?;

validate_envelopes(
// returns ONLY the validated envelopes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// returns ONLY the validated envelopes
// returns ONLY the valid envelopes

Comment on lines 178 to 181
log::warn!(
"Envelope with slot index {}: Node id {:?} is not part of validators list",
envelope.statement.slot_index,
envelope.statement.node_id
Copy link
Member

Choose a reason for hiding this comment

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

We have the same log statement in the is_node_exist() function so let's rather decide for one instead of having both

envelope.statement.slot_index,
envelope.statement.node_id
);
// ignore this ennvelope; continue to the next ones
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// ignore this ennvelope; continue to the next ones
// ignore this envelope; continue to the next ones

})
.ok_or(Error::<T>::MissingExternalizedMessage)
// it's ok to use unwrap here, since the size will be <= to the provided envelopes
Ok(UnlimitedVarArray::new(validated_envelopes).unwrap())
Copy link
Member

Choose a reason for hiding this comment

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

You are right that's technically okay to use unwrap() but let's still don't. How about of the unwrap() we return the default() implementation of UnlimitedVarArray which I assume returns an empty array?

Suggested change
Ok(UnlimitedVarArray::new(validated_envelopes).unwrap())
Ok(UnlimitedVarArray::new(validated_envelopes).unwrap())

@b-yap
Copy link
Contributor Author

b-yap commented Aug 6, 2024

https://github.com/pendulum-chain/spacewalk/actions/runs/10263219351/job/28396526378?pr=546#step:13:556
Tested in my local machine, and works with Iowa and Singapore.

Frankfurt on the otherhand:
Screenshot 2024-08-06 at 7 26 34 PM

[2024-08-06T11:27:21Z ERROR stellar_relay_lib::connection::connector::message_handler] process_raw_message(): Received ErrorMsg during authentication: Error{ code:ErrLoad message:peer rejected }
[2024-08-06T11:27:21Z ERROR stellar_relay_lib::connection::error] Stellar Node returned error: Error{ code:ErrLoad message:peer rejected }

I'm removing Frankfurt in the list, for the meantime.

@@ -7,7 +7,7 @@ name: continuous-integration-main
jobs:
ci:
strategy:
max-parallel: 2
max-parallel: 1
Copy link
Member

Choose a reason for hiding this comment

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

Should we turn this back again?

_ => false,
})
.ok_or(Error::<T>::MissingExternalizedMessage)
// it's ok to use unwrap here, since the size will be <= to the provided envelopes
Copy link
Member

Choose a reason for hiding this comment

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

Nit: remove stale comment

@b-yap b-yap force-pushed the 544-change-checks-for-unknown-validators-in-stellar-relay-pallet branch from 5f84aed to 63dce28 Compare September 4, 2024 06:04
@b-yap
Copy link
Contributor Author

b-yap commented Sep 4, 2024

@ebma after I fix the fmt issue, I'll merge it immediately.

@b-yap b-yap merged commit ff420a5 into main Sep 4, 2024
0 of 2 checks passed
@b-yap b-yap deleted the 544-change-checks-for-unknown-validators-in-stellar-relay-pallet branch September 4, 2024 13:56
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.

Change checks for unknown validators in Stellar relay pallet
2 participants