-
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
360 support confirm scp messages in the stellar relay when proving transaction validity #362
360 support confirm scp messages in the stellar relay when proving transaction validity #362
Conversation
@TorstenStueber can I take this assumption or do I need to implement the range like you describe in the Notion page (and implemented here). |
@ebma I will run my script and check whether this is generally the case. The actual answer will only come with a deep dive into Stellar core code, but we can postpone this. |
Turns out that it is not possible. Ledger 46647565 of the Stellar archive contains the following messages:
Here Luckily the same ledger looks different for the SatoshiPay archive:
So we could fallback to another archive in this case again. Please check, though, whether the |
@TorstenStueber thanks for investigating. Since the fallback you mentioned would have to happen in the vault client though, we don't need to consider it here probably and can live with taking the assumption of having the same n.h for all messages. It's very important though to keep it in mind for the vault client. |
Okay, but the runtime should still check equality of the |
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.
Looks good in principle but I am concerned about the requirement that all messages in the archive stem from nodes in the Tier 1 set. We need to discuss what we can do here.
use frame_support::{traits::Get, weights::Weight}; | ||
use sp_std::marker::PhantomData; | ||
use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; | ||
use core::marker::PhantomData; |
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.
maybe revert back to sp_std
?
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.
Do you think this matters? It changed because the latest template file uses core::
…lar-relay-when-proving-transaction-validity
@b-yap @TorstenStueber what do you say, can this be merged? |
Ping @b-yap @TorstenStueber |
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.
I approve of the changes. There is a small nit I mentioned in one comment but I don't find it important enough to slow down this PR any further.
match &externalized_envelope.statement.pledges { | ||
ScpStatementPledges::ScpStExternalize(externalized_statement) => | ||
(&externalized_statement.commit.value, externalized_statement.n_h), | ||
ScpStatementPledges::ScpStConfirm(confirmed_statement) => |
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.
This case cannot happen as externalized_envelope
is define in line 567 to be an ScpStExternalize
message.
I tried to adhere to the simplified approach described in this document.
The implementation deviates from the described approach as follows:
n_h
value lies within a certain range, the implementation takes the assumption that this value will be the same for all SCP messages.I had to change some of the test helper functions so that it's easier to build externalized and confirmed messages in one go, without having to duplicate code.
I recommend reading the Notion page and looking at the test cases to see if everything is covered.
Closes #360.