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

Implement ADR05 (Handlers validation and execution separation) #221

Closed
3 of 9 tasks
hu55a1n1 opened this issue Nov 7, 2022 · 6 comments
Closed
3 of 9 tasks

Implement ADR05 (Handlers validation and execution separation) #221

hu55a1n1 opened this issue Nov 7, 2022 · 6 comments
Labels
A: tracking Admin: tracking/meta issue

Comments

@hu55a1n1
Copy link
Member

hu55a1n1 commented Nov 7, 2022

Summary

Complete ADR05 implementation -> adr-005-handlers-redesign.md


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@hdevalence
Copy link

hdevalence commented Dec 6, 2022

Maybe I'm missing something, but how does separating validation and execution work with the common cases of how IBC is used today (cc @avahowell)? Specifically, it's common to create a transaction that (1) updates a client and then (2) relays a message that was included in the newly updated state. If validation and execution are completely separate, with all validation checks occurring prior to any execution checks, than it's not possible to do this, because the message in (2) has to be validated against the client state prior to executing (1), which will fail validation. Avoiding this means either (a) splitting client updates and relayed messages into separate transactions, and coordinating with the block proposer to correctly order them within the block, or [more likely] (b) adding an additional 1-block latency for relayed IBC messages.

In Penumbra, we actually started with a state model that fully separated validation and execution, but ended up moving away from it because it didn't work with our IBC implementation for this reason, so I'm curious if there's a solution we missed. (We now have a three-phase execution model, with check_stateless, check_stateful, and execute, all of which are fallible, so that execute is really check-and-execute; for other components we move as much of the verification logic as possible "upwards", but the IBC component does most of its checks in execute).

@plafer
Copy link
Contributor

plafer commented Dec 6, 2022

with all validation checks occurring prior to any execution checks

This indeed won't work; you always need to run the validation and execution before moving on to the next message/transaction.

The only architecture we know of today that makes use of this distinction is Namada. For each transaction in the block, they:

  1. "execute" the transaction (our ExecutionContext::execute() for IBC packets)
  2. Run all validation functions to run per transaction (their so-called validity predicates); the ibc ValidationContext::validation() is only one of them. They get a performance boost by running all these validity predicates in parallel. Only if all these validity predicates pass is the transaction included in the block.

@hdevalence
Copy link

with all validation checks occurring prior to any execution checks

This indeed won't work; you always need to run the validation and execution before moving on to the next message/transaction.

Yes, that's what I meant, sorry, I should have phrased it more clearly as "with all validation checks (for a specific transaction) occurring prior to any execution (for that specific transaction).

The only architecture we know of today that makes use of this distinction is Namada.

We also make use of this architecture in Penumbra to do parallel checks, but your comment highlights the key difference, which is that we perform validation prior to execution, and they perform validation after execution. Running validation after execution rather than before does seem like a solution to this problem, so that answers my question, thanks!

@plafer
Copy link
Contributor

plafer commented Dec 6, 2022

Now I'm confused!

I should have phrased it more clearly as "with all validation checks (for a specific transaction) occurring prior to any execution (for that specific transaction).

Do you assume a model where a transaction holds many messages (similar to how the SDK does it? Note that Namada has only one message per transaction.

The mental model I was using here was "one message per transaction". So when you say:

Specifically, it's common to create a transaction that (1) updates a client and then (2) relays a message that was included in the newly updated state. If validation and execution are completely separate, with all validation checks occurring prior to any execution checks,

I think of this as 2 "transactions". In your case, it would be: first run all the validation checks for the UpdateClient, then the execution for UpdateClient. Then the validation checks for the other IBC message (e.g. some packet), followed by execution. This model doesn't run in the issue you then describe.

Hence, in this model, for what IBC is concerned, either validation or execution could be run first without affecting correctness (since our validation functions don't look at what execution did). The reason Namada does it like that is roughly that execution is actually untrusted code, and so they want to make sure that execution actually did the right thing (in a separate validity predicate).

Or am I missing something?

@Farhad-Shabani
Copy link
Member

@plafer
Is it right that we have a tailored validation and execution function for each of the messages like MsgCreateClient, MsgUpdateClient, etc.?
Couldn’t we use the following pattern (as a general concept) and implement them on each message type?

impl validator<Ctx: RouterContext> {
	pub fn validate(&self, ctx: &mut Ctx) -> Result<(), Error>;
}

impl executor<Ctx: RouterContext> {
	pub fn execute(&self, ctx: &mut Ctx) -> Result<(), Error>;
}

// consider it for MsgCreateClient 
impl validator<MockContext> for MsgCreateClient {...}
impl executor<MockContext> for MsgCreateClient {...}

It looks more modular, maintainable, and extensible if it works! (please direct me to the places if I am missing something. tnx)

@github-project-automation github-project-automation bot moved this to 📥 To Do in ibc-rs Feb 2, 2023
@Farhad-Shabani Farhad-Shabani moved this from 📥 To Do to 🏗️ In Progress in ibc-rs Feb 2, 2023
@Farhad-Shabani Farhad-Shabani modified the milestones: v0.28.0, Support Anoma's onboarding efforts Feb 3, 2023
@plafer
Copy link
Contributor

plafer commented Feb 21, 2023

Closing as done, even though we didn't implement the host-based API. If we think it is still relevant, we can implement it in the future.

@plafer plafer closed this as completed Feb 21, 2023
@github-project-automation github-project-automation bot moved this from 🏗️ In Progress to ✅ Done in ibc-rs Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: tracking Admin: tracking/meta issue
Projects
None yet
Development

No branches or pull requests

4 participants