Message inclusion check #216
Conversation
app/process_proposal.go
Outdated
| // extract the commitments from any MsgPayForMessages in the block | ||
| commitments := make(map[string]struct{}) | ||
| for _, rawTx := range req.BlockData.Txs { | ||
| tx, err := app.txConfig.TxDecoder()(rawTx) | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| for _, msg := range tx.GetMsgs() { | ||
| if sdk.MsgTypeURL(msg) != types.URLMsgPayforMessage { | ||
| continue | ||
| } | ||
|
|
||
| pfm, ok := msg.(*types.MsgPayForMessage) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| commitments[string(pfm.MessageShareCommitment)] = struct{}{} | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
using this mechanism also makes it so that two identical messages included in the same block are invalid. Is that something we want? If so, I can add a specific test for this, along with making it so block producers don't accidentally do this. If not, then we can fix this before merging.
cc @adlerjohn
There was a problem hiding this comment.
I don't see any reason to forbid including two of the same message in the same block, so long as they're paid for. That being said, a block proposer could be tricky and make both PayForMessage txs in the block point to the same message! An interesting corner case. Can you file an issue...somewhere...to re-visit this? And document your decision (forbid or allow, doesn't really matter, impl detail) in the code with a link to the issue.
There was a problem hiding this comment.
will update this PR tmrw to make the default behavior to exclude the identical message, but keep the payments
There was a problem hiding this comment.
update to this, I just ended up taking the easier option, f3b543f in which we count the commitments using a counter instead of relying on a map, this way we also count identical commitments and I think things work intuitively. Intuitively being that identical messages get included twice and each PayForData must have a corresponding message.
we should still make a formal decision with #226
|
If there are outstanding TODOs can we convert this to draft? |
Codecov Report
@@ Coverage Diff @@
## master #216 +/- ##
=======================================
Coverage 25.86% 25.86%
=======================================
Files 12 12
Lines 1840 1840
=======================================
Hits 476 476
Misses 1326 1326
Partials 38 38 Continue to review full report at Codecov.
|
Co-authored-by: Ismail Khoffi <[email protected]>
Co-authored-by: Ismail Khoffi <[email protected]>
Co-authored-by: John Adler <[email protected]>
Co-authored-by: John Adler <[email protected]>
Co-authored-by: John Adler <[email protected]>
Co-authored-by: John Adler <[email protected]>
Co-authored-by: Ismail Khoffi <[email protected]>
Co-authored-by: John Adler <[email protected]>
Co-authored-by: John Adler <[email protected]>
Description
This PR creates a consensus time message inclusion check using the new
ProcessProposalABCI++ method.It's worth noting that this check does not actually check that the commitment to the data is an actual subtree root to the data commitment, because 1) we aren't actually filling the square according to the non-interactive default rules yet and 2) the specs specifically describes the non-interactive rules as not consensus critical, unlike message inclusion. However, we can eventually make those rules consensus critical using the same ABCI method if that's something we want to do.
It's also worth noting that this check does not ensure that the
MsgPayForMessagethat it is checking the messages against is actually valid. This means that if a validator wanted to, they could produce a block that has an invalidMsgPayForMessage, which therefore wouldn't pay for the message, but the message does get included in the block. In order to check if theMsgPayForMessageis valid, I think we might have wait for immediate execution.This PR also only uses the normal integration test, meaning that the happy case is tested for implicitly if the test passes, but the cases where the block should be rejected do not have integration/liveness tests. We can either write an issue to do this in a different PR or we can add these integration tests to this PR.
closes #205