-
Notifications
You must be signed in to change notification settings - Fork 7
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
Ignore Envelopes with Unkown Validators #546
Conversation
There was a problem hiding this 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.
pallets/stellar-relay/src/lib.rs
Outdated
@@ -574,7 +579,8 @@ pub mod pallet { | |||
.get_tx_set_hash() | |||
.map_err(|_| Error::<T>::FailedToComputeNonGenericTxSetContentHash)?; | |||
|
|||
validate_envelopes( | |||
// returns ONLY the validated envelopes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// returns ONLY the validated envelopes | |
// returns ONLY the valid envelopes |
log::warn!( | ||
"Envelope with slot index {}: Node id {:?} is not part of validators list", | ||
envelope.statement.slot_index, | ||
envelope.statement.node_id |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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()) |
There was a problem hiding this comment.
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?
Ok(UnlimitedVarArray::new(validated_envelopes).unwrap()) | |
Ok(UnlimitedVarArray::new(validated_envelopes).unwrap()) |
https://github.com/pendulum-chain/spacewalk/actions/runs/10263219351/job/28396526378?pr=546#step:13:556
I'm removing Frankfurt in the list, for the meantime. |
.github/workflows/ci-main.yml
Outdated
@@ -7,7 +7,7 @@ name: continuous-integration-main | |||
jobs: | |||
ci: | |||
strategy: | |||
max-parallel: 2 | |||
max-parallel: 1 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove stale comment
5f84aed
to
63dce28
Compare
@ebma after I fix the fmt issue, I'll merge it immediately. |
closes #544
Need to close pendulum-chain/substrate-stellar-sdk#32 before proceeding.
This will ignore validating envelopes with a different
node_id
.