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.

Sign up for GitHub

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

Jump to bottom

docs: RFC 001 tx validation #15363

Merged
merged 7 commits into from
Mar 17, 2023
Merged

docs: RFC 001 tx validation #15363

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7e2aeed
add rfc 01
tac0turtle Mar 12, 2023
1efb6bc
update
tac0turtle Mar 12, 2023
3e1b365
Merge branch 'main' into marko/rfc-01-tx
tac0turtle Mar 13, 2023
8948c38
Merge branch 'main' into marko/rfc-01-tx
tac0turtle Mar 13, 2023
f9ed74f
Merge branch 'main' into marko/rfc-01-tx
tac0turtle Mar 17, 2023
728e641
update template
tac0turtle Mar 17, 2023
27e168c
Merge branch 'main' into marko/rfc-01-tx
tac0turtle Mar 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions docs/rfc/rfc-001-tx-validation.md
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend following Hashicorp's RFC template -> https://works.hashicorp.com/articles/rfc-template

There is no predefined structure to the body. Just a background and then whatever else you want. I think we should follow the table section.

Copy link
Member Author

Choose a reason for hiding this comment

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

I recommend following Hashicorp's RFC template -> https://works.hashicorp.com/articles/rfc-template

There is no predefined structure to the body. Just a background and then whatever else you want. I think we should follow the table section.

thanks for this, ill adjust the template!

Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# RFC 001: Transaction Validation

## Changelog

* 2023-03-12: Proposed

## Background

Transation Validation is crucial to a functioning state machine. Within the Cosmos SDK there are two validation flows, one is outside the message server and the other within. The flow outside of the message server is the `ValidateBasic` function. It is called in the antehandler on both `CheckTx` and `DeliverTx`. There is an overhead and sometimes duplication of validation within these two flows. This extra validation provides an additional check before entering the mempool.

With the deprecation of [`GetSigners`](https://github.com/cosmos/cosmos-sdk/issues/11275) we have the optionality to remove [sdk.Msg](https://github.com/cosmos/cosmos-sdk/blob/16a5404f8e00ddcf8857c8a55dca2f7c109c29bc/types/tx_msg.go#L16) and the `ValidateBasic` function.

With the separation of CometBFT and Cosmos-SDK, there is a lack of control of what transactions get broadcasted and included in a block. This extra validation in the antehandler is meant to help in this case. In most cases the transaction is or should be simulated against a node for validation. With this flow transactions will be treated the same.
Copy link
Collaborator

@yihuang yihuang Mar 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

does that mean the tx need to be executed when entering mempool, will that introduce DoS vulnerability to mempool? Or the msgs are not checked at all in mempool, only verify tx wrapping structure like signatures and fees?

Copy link
Member Author

Choose a reason for hiding this comment

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

there wouldnt be an introduction of another way to run this before entering the mempool. ideally the application mempool can run its own checks in parallel. This way the tx wouldnt enter the mempool and the tx would be removed from comets mempool after a recheck.

In the future it would be ideal if comet asked the application which txs it would like to broadcast so that the app can run these check more smoothly


## Proposal

The acceptance of this RFC would move validation within `ValidateBasic` to the message server in modules, update tutorials and docs to remove mention of using `ValidateBasic` in favour of handling all validation for a message where it is executed.

We can and will still support the `Validatebasic` function for users and provide an extension interface of the function once `sdk.Msg` is depreacted.

> Note: This is how messages are handled in VMs like Ethereum and CosmWasm.

### Consequences

The consequence of updating the transaction flow is that transaction that may have failed before with the `ValidateBasic` flow will now be included in a block and fees charged.
Comment on lines +23 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there discussion of why ValidateBasic was chosen as a design pattern originally for the SDK? I understand there is likely no original public discussion of it, but it might be useful to retroactively discuss what benefits its existence provides. This commit is the only historical trace I could find.

I have concerns with this direction. I think not including ValidateBasic in the sdk.Msg interface is a valid design decision, if it is the original choice. I don't find ValidateBasic or sdk.Msg to be problematic in any way and I'm not sure why we are changing fundamental design patterns? This change appears to be introducing additional complexity/context when there is likely a non-breaking alternative which doesn't require redefining fundamental design patterns of the SDK. Is there a reason why the msg.ValidateBasic cannot be called in a msg server interceptor?

While this RFC indicates that removal of ValidateBasic is backwards compatible, I find this problematic. It introduces additional context and/or required changes in order to avoid pitfalls. Any caller of the msg server needs to also support this backwards compatible approach of calling ValidateBasic on msgs which implement this interface. We would need to add this to ICA and presumably authz/gov? Removing this backwards compatible support is quite difficult since there is no programmatic way of telling users that they must finalize their migration to stateless checks being called directly in the msg handler.

Users also need to be aware not to change the function naming of ValidateBasic to any other function name to ensure that essential checks are still run. This is additional implicit context that can be easily forgotten or overlooked, thus developers will likely need to make these changes regardless (rendering the backwards compatible support not so useful)

@DimitrisJim pointed out a nice benefit of ValidateBasic to me. It is a strong reminder to developers to do stateless validation of the arguments they are receiving. It seems quite possible to me that new developers could easily forget to check that fields are non-empty/valid. Has there been discussion on this point?

Can we add an explanation on the positives of this proposal and why they outweigh the negatives? I'd like to push back on refactoring when there is no particular reason to do so

Copy link
Member Author

Choose a reason for hiding this comment

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

I have concerns with this direction. I think not including ValidateBasic in the sdk.Msg interface is a valid design decision, if it is the original choice. I don't find ValidateBasic or sdk.Msg to be problematic in any way and I'm not sure why we are changing fundamental design patterns? This change appears to be introducing additional complexity/context when there is likely a non-breaking alternative which doesn't require redefining fundamental design patterns of the SDK. Is there a reason why the msg.ValidateBasic cannot be called in a msg server interceptor?

i would argue the opposite this simplifies the current approach. If we look at any other ecosystem they do not have this sort of check it is handled when the execution happens. the current design pattern was chosen due do limitations construed when this was written but as the ecosystem evolved we see that this isnt the case and the state less validation doesnt add much value. You can still use validate basic it wasnt removed and wont be removed, but it will be removed from sdk.Message as we are trying to remove global interface registries from the sdk for a few reasons.

While this RFC indicates that removal of ValidateBasic is backwards compatible, I find this problematic. It introduces additional context and/or required changes in order to avoid pitfalls. Any caller of the msg server needs to also support this backwards compatible approach of calling ValidateBasic on msgs which implement this interface. We would need to add this to ICA and presumably authz/gov? Removing this backwards compatible support is quite difficult since there is no programmatic way of telling users that they must finalize their migration to stateless checks being called directly in the msg handler.

users will get an error that msg.Validatebasic doesnt exist on sdk.message and they should remove it or update it to the extension interface. In your comment it seems you are missing the extension interface, if a message implements the extension interface, i.e. doesnt change anything in their repo then it will still work.

pointed out a nice benefit of ValidateBasic to me. It is a strong reminder to developers to do stateless validation of the arguments they are receiving. It seems quite possible to me that new developers could easily forget to check that fields are non-empty/valid. Has there been discussion on this point?

these checks should and can happen at the message server level, it is actually safer to do it this way because modules now dont assume validation is handled for them via validate basic like some teams have done but instead then can rely on it checking the validation with certainty. How is this less safe?

Can we add an explanation on the positives of this proposal and why they outweigh the negatives? I'd like to push back on refactoring when there is no particular reason to do so

happy to have a longer chat about this but sdk.Msg will be deprecated in the near future for a few reasons. Validatebasic in the current form prevents removal of globalbech32 as well. Users can still implement the extension interface if they would like but it is recommended all checks happen in the msg server to make the system less complex and safer