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

Checking all the transaction validity of a bundle within one runtime api call #2184

Closed
Tracked by #1706
NingLin-P opened this issue Oct 31, 2023 · 6 comments
Closed
Tracked by #1706
Assignees

Comments

@NingLin-P
Copy link
Member

NingLin-P commented Oct 31, 2023

From a previous discussion with @vedhavyas and @ParthDesai, we realized when checking the transaction validity of a bundle, we need to perform the check for the whole bundle within one runtime api call instead of one runtime api call for one transaction. This is mainly required by the transaction payment check, which will check if the tx signer has enough balance to cover the transaction fee (including tip).

Consider there is a bundle containing 10 tx from the same signer and each tx's fee consumes 2/3 of the signer's total balance, so the signer can afford each of the tx but it can only afford one. While performing the transaction payment check with one transaction one runtime api call, the bundle can pass the check since the signer can afford each of the tx, but we can know only one tx can be executed successfully and the rest of the tx just consumed block space with no cost.

This can be fixed by performing the check for whole bundle within one runtime api call, because the transaction payment check will withdraw the fee from the singer while performing the check, the state changes can be observed within the same runtime call and will be discarded after the call. So when checking the second tx we can know that the singer only has 1/3 of balance left and can't cover the second tx's fee, thus the bundle will fail to pass the check and will be considered as an invalid bundle.

While the fix can be implemented easily, it brings more work to the illegal tx fraud proof, because now the fraud proof will need to prove the bundle as a whole can't pass the check instead of proving a single transaction can't pass the check, so the fraud proof needs to:

  • Carry the whole bundle or get the whole bundle through the host function
  • Carry the domain state witness (i.e. account balance state) for the signer of the first illegal tx
  • Perform the check for the bundle, skip the check for tx that is signed by other signers since the proof doesn't contain the domain state witness for other signers
  • Verified if the check will fail at a particular index that matches the one claim by the fraud proof

cc @vedhavyas @ParthDesai

@ParthDesai
Copy link
Contributor

ParthDesai commented Nov 13, 2023

@NingLin-P We may need to provide the proof of the keys used during validation. And we can use intermediate state root recorded after n-1 tx execution to do that.

@NingLin-P
Copy link
Member Author

Currently, we just assume the storage kay is unchanged for simplicity. Plz leave it as it is and add a TODO instead, we can revisit this along with the storage kay used in other fraud proofs later.

@ParthDesai
Copy link
Contributor

@NingLin-P I am not talking about the bundle storage key in consensus runtime. I am talking about the storage keys accessed by the validate method for which I wrote runtime api earlier. We need storage proof for keys used by SignedExtension::Validate against the intermediate state root recorded after n-1 tx execution.

@NingLin-P
Copy link
Member Author

I took a closer look at storage_keys_for_verifying_tx_validity_inner, it seems to be a function that takes some info of the transaction as input and outputs the storage kays, we may be unable to construct storage proof for this, because the storage proof is for something that stored on the chain state.

I'm thinking of getting the storage keys through the storage_keys_for_verifying_transaction_validity runtime API, since it is a stateless API we don't need storage proof.

@ParthDesai
Copy link
Contributor

ParthDesai commented Nov 13, 2023

I took a closer look at storage_keys_for_verifying_tx_validity_inner, it seems to be a function that takes some info of the transaction as input and outputs the storage kays, we may be unable to construct storage proof for this, because the storage proof is for something that stored on the chain state.

I'm thinking of getting the storage keys through the storage_keys_for_verifying_transaction_validity runtime API, since it is a stateless API we don't need storage proof.

We both are talking about the same thing. :) Cool. Will open a PR soon enough for this.

@ParthDesai
Copy link
Contributor

Closed via #2327

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

No branches or pull requests

2 participants