-
Notifications
You must be signed in to change notification settings - Fork 346
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
feat: add additonal validity checks during process proposal #1128
Changes from all commits
7b53588
2eb7534
924156a
4c4608a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,25 +79,63 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) abci.ResponsePr | |
|
||
tx, err := app.txConfig.TxDecoder()(malleatedTx.Tx) | ||
if err != nil { | ||
// 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, | ||
} | ||
} | ||
|
||
var checkedTx bool | ||
|
||
for _, msg := range tx.GetMsgs() { | ||
if sdk.MsgTypeURL(msg) != types.URLMsgPayForBlob { | ||
continue | ||
} | ||
|
||
if !checkedTx { | ||
err = tx.ValidateBasic() | ||
if err != nil { | ||
app.Logger().Error("Tx including MsgPayForBlob is invalid") | ||
return abci.ResponseProcessProposal{ | ||
Result: abci.ResponseProcessProposal_REJECT, | ||
} | ||
} | ||
|
||
response := app.BaseApp.CheckTx(abci.RequestCheckTx{ | ||
Tx: rawTx, | ||
Type: abci.CheckTxType_New, | ||
}) | ||
Comment on lines
+103
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [blocking] we also only need to check the signature, and therefore also looking up the account. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah makes sense, I will revert to previous changes |
||
|
||
if response.GasUsed > response.GasWanted { | ||
return abci.ResponseProcessProposal{ | ||
Result: abci.ResponseProcessProposal_REJECT, | ||
} | ||
} | ||
Comment on lines
+108
to
+112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
checkedTx = true | ||
} | ||
|
||
pfb, ok := msg.(*types.MsgPayForBlob) | ||
if !ok { | ||
app.Logger().Error("Msg type does not match MsgPayForBlob URL") | ||
continue | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we rejecting txs that have more than one signer? |
||
logInvalidPropBlockError(app.Logger(), req.Header, "cannot have multiple signers for MsgPayForBlob", err) | ||
return abci.ResponseProcessProposal{ | ||
Result: abci.ResponseProcessProposal_REJECT, | ||
} | ||
} | ||
|
||
signer, err := sdk.AccAddressFromBech32(pfb.Signer) | ||
if err != nil { | ||
// this panic should be unreachable | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we checking the signer here? |
||
logInvalidPropBlockError(app.Logger(), req.Header, "invalid signer for MsgPayForBlob", err) | ||
return abci.ResponseProcessProposal{ | ||
Result: abci.ResponseProcessProposal_REJECT, | ||
} | ||
|
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.
still thinking about this 🤔 but its probably the correct move. All transactions with an index wrapped around them must be decodable