feat: add non-atomic multi-msg transactions #15038
Conversation
nicolaslara
changed the title
|
hey let us know when youd like some feedback on this pr |
nicolaslara
changed the title
|
@tac0turtle I don't need a full review rn, but some feedback on the general approach would be great! Basically, do we want to:
If so, then my next steps are:
|
There was a problem hiding this comment.
The general idea of this makes sense to me. Is this something you intend to complete @nicolaslara? Really cool to see someone working on this 🙏
|
yeah, I want to complete it soon, just didn't want to spend too much time on it before validating the approach. |
github-prbot
requested review from
a team,
JeancarloBarrios and
samricotta
and removed request for
a team
|
@tac0turtle @alexanderbez This is ready for feedback now. There are two test failures that I'm not sure how to get around:
Other than that I think this covers everything we need. |
x/consensus/keeper/keeper.go
Outdated
| if err := k.event.EventManager(ctx).EmitKV( | ||
| ctx, | ||
| "update_consensus_params", | ||
| event.Attribute{Key: "authority", Value: req.Authority}, | ||
| event.Attribute{Key: "parameters", Value: consensusParams.String()}); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
what's the reason for this?
| // | ||
| // If set to true, the transaction will be executed as long as at least one | ||
| // of the messages succeeds. | ||
| bool non_atomic = 4; |
There was a problem hiding this comment.
It's on this same PR now 👌
baseapp/baseapp.go
Outdated
| if err != nil { | ||
| return nil, errorsmod.Wrapf(err, "failed to execute message; message index: %d", i) | ||
| wrappedErr := errorsmod.Wrapf(err, "failed to execute message; message index: %d", i) |
There was a problem hiding this comment.
Unfortunately, this is unsafe. The block header includes the following information as determinstic:
- Code
- Data (marshalled msg responses)
- GasWanted
- GasUsed
This combination creates a potential halting scenario. This code will include the full error string from the msg exeuction into the msg responses, thus causing the error string to know be included in the block. If the error string is undeterminstic (happens with json marshal errors) or it is changed (a backport changes error string formatting), a chain would halt. We have this same issue with writing error strings into acknowledgements for IBC, which is common occurrence with interchain accounts
To increase the readability of these error messages, I'd like to ask cometbft to include the codespace in the block header as well (I will open an issue), but I also think there should potentially be a more radical change to include the base error message as well. The original design was to use the Code as a condensed error string with the idea that that clients would lookup the code to get a more descriptive error string, but this is infeasible with IBC as a client would need to look up a different chain's error code
There was a problem hiding this comment.
This makes sense. A simple solution would be to extract error code via errorsmod.ABCIInfo
There was a problem hiding this comment.
the logic which is unsafe here seems to be the same as it was before, id opt to not return any sort of error to comet which gets included in consensus, (wiping the included data on errors that gets included in the header). I think this is fundamentally unsafe and its surprising no one has halted because of this.
There was a problem hiding this comment.
The difference between before and after is that now the error is being included in the msgResponses which will eventually be marshalled and placed into the Data field (which is a determinsitic field).
Existing flow:
- error returned on failed msg
- which is passed back from runTx
- which is put into a deliver tx response
- and the non-deterministic error is removed
Thus the non-determinstic parts of an error returned on a failed msg execution are removed for the determinstic fields of the deliver tx response. In the new flow, the full error is put into the msg response below on line 843, which eventually ends up in the Data field which could lead to an app hash mismatch
One tricky part is that you likely want the errors to end up in the deliver tx response as well since the log is non-determinstic at the moment and can provide useful information
There was a problem hiding this comment.
I think having the error that gets included in the error responses be just the ABCIInfo end emit the context as events or logs would solve this.
Also, hot take: consensus failures on error string differences should be irrelevant as long as all validators return an error, and there should be a way to fix this post-hoc at a performance penalty (though that's a much larger issue)
There was a problem hiding this comment.
Made the change so that errors are converted to ABCI errors in 5a0ffba
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@testinginprod could we get a review on this pr |
There was a problem hiding this comment.
🙅
Transactions are a concept that assert precisely one property: atomic execution. If a transaction doesn't provide atomic execution, then it is not a transaction, full stop.
You can define something else that provides partial execution, that's fine! But you can't redefine transaction to sometimes be atomic, and sometimes not. No go.
There was a problem hiding this comment.
Thank you @nicolaslara, please take a look at these few comments.
| } | ||
|
|
||
| for testNum, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { |
There was a problem hiding this comment.
Please make a fesh copy of tc and testNum that ensure that the values will be afresh even if the test is parallelized, so:
testNum, tc := testNum, tc
t.Run(tc.name, func(t *testing.T) {
...| // replace the second message with a Counter2 | ||
| tx = newTxCounter(t, suite.txConfig, NonAtomic, int64(testNum+1), 3) | ||
|
|
||
| builder := suite.txConfig.NewTxBuilder() |
There was a problem hiding this comment.
Would be nice to declare this variable close too where it is used.
| // If set to true, the transaction will be executed as long as at least one | ||
| // of the messages succeeds. |
| // If set to true, the transaction will be executed as long as at least one | ||
| // of the messages succeeds. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Description
Closes: #13911
This adds support for non-atomic multi-msg transactions.
It introduces a NonAtomic boolean field in the transaction proto that defaults to false. If this is not set, everything should behave as before. If set, the transaction will succeed if at least one message succeeds.
If all messages fail, the whole transaction will fail the same way as atomic transactions do.
State changes form failed messages should be reverted.
If the tx succeeds, but some messages failed. The resulting TxMsgData should contain an array of message responses that matches the order of the submitted messages. Successes will contain the MsgResponse and failures will contain an empty response created specifically for this:
/cosmos.tx.v1beta1.MsgFailureResponseAuthor Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change