feat: add additonal validity checks during process proposal #1128
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1128 +/- ##
==========================================
+ Coverage 47.43% 47.74% +0.31%
==========================================
Files 71 71
Lines 3970 4017 +47
==========================================
+ Hits 1883 1918 +35
- Misses 1915 1931 +16
+ Partials 172 168 -4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
we actually don't have to do this atm, as we are currently only supporting sdk.Tx with a single sdk.Msg, that being a PFB |
| // we don't reject the block here because it is not a block validity | ||
| // rule that all transactions included in the block data are | ||
| // decodable | ||
| continue | ||
| return abci.ResponseProcessProposal{ | ||
| Result: abci.ResponseProcessProposal_REJECT, | ||
| } |
There was a problem hiding this comment.
still thinking about this 🤔 but its probably the correct move. All transactions with an index wrapped around them must be decodable
| response := app.BaseApp.CheckTx(abci.RequestCheckTx{ | ||
| Tx: rawTx, | ||
| Type: abci.CheckTxType_New, | ||
| }) |
There was a problem hiding this comment.
[blocking]
We can not use CheckTx here, as it uses its own copy of the state, not the actual state! I think its very probable that this would not be deterministic, since the check tx state depends on which txs that validator has added to their mempool.
we also only need to check the signature, and therefore also looking up the account.
There was a problem hiding this comment.
Ah makes sense, I will revert to previous changes
| if response.GasUsed > response.GasWanted { | ||
| return abci.ResponseProcessProposal{ | ||
| Result: abci.ResponseProcessProposal_REJECT, | ||
| } | ||
| } |
There was a problem hiding this comment.
let's only check the signature for now as state in the target issue. As a side note, the gas used in check tx isn't 100% accurate
| if err = pfb.ValidateBasic(); err != nil { | ||
| logInvalidPropBlockError(app.Logger(), req.Header, "invalid MsgPayForBlob", err) | ||
| signers := msg.GetSigners() | ||
| if len(signers) != 1 { |
There was a problem hiding this comment.
why are we rejecting txs that have more than one signer?
| panic("signer address validation should not fail after basic validation has already been done") | ||
| } | ||
|
|
||
| if !bytes.Equal(signer, signers[0]) { |
There was a problem hiding this comment.
why are we checking the signer here?
|
closing in favor of #1300 |
Adds additional validity checks during process proposal to make sure that all transactions that include
MsgPayForBlobmessages should be valid. See #979 for detailsMsgPayForBlobmessagesValidateBasicMsgPAyForBlobmessages