Multiple messages #1266

AdityaSripal · 2018-06-15T06:39:08Z

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG.md
Updated Gaia/Examples
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Addresses issues in #669 .

Currently all tests in types and x/auth pass.

cwgoes · 2018-06-16T02:06:02Z

Also ref #903 I believe.

This reverts commit 34ded33.

AdityaSripal · 2018-06-18T23:03:05Z

Current way multiple messages are handled:

Consider a transaction with 4 messages where the second message fails.

Currently the first message will successfully update the state, and once the second msg fails, msgs 3 and 4 are ignored. Thus, only the first message will successfully change the state and Deliver(tx) will return the error result from the second msg's handler.

I'm not sure if this is the behavior we want. We could also just refuse to update the state unless all 4 msgs are successful.

Thoughts?

cwgoes · 2018-06-19T04:02:13Z

Currently the first message will successfully update the state, and once the second msg fails, msgs 3 and 4 are ignored. Thus, only the first message will successfully change the state and Deliver(tx) will return the error result from the second msg's handler.

I do not think this is the behavior we want; I think we want all transactions, including multi-Msg transactions, to be atomic: either all Msgs are successful and are applied to the state, or any fail and the state is unchanged.

As I understand it, the primary use case for multi-Msg transactions (besides just convenience) is atomicity: perhaps I want to finalize a governance proposal and spend funds (which the proposal will release) only if it passes, or trade a DEX order (in the future) and send tokens I just purchased to pay a friend, but not if the DEX order failed to match.

Alternatively, the transaction itself could include a flag to determine what happens when one Msg errors - either "stop execution but leave existing state changes", "continue anyways", or "revert previous state changes". I don't have particular use cases for the first two in mind but that would be more flexible.

AdityaSripal · 2018-06-19T04:21:29Z

Got it. Transaction handling is now atomic on latest commit.

cwgoes · 2018-06-19T04:38:24Z

Got it. Transaction handling is now atomic on latest commit.

That was a stunning turnaround time. I think that's fine for now unless anyone has a compelling use-case for a different execution strategy.

cwgoes

Thanks! Quickly reviewed, needs a rebase against develop, some changes to multi-Msg result construction, and tests for all the multi-signer functionality.

cwgoes · 2018-06-20T19:37:38Z

baseapp/baseapp.go

-	var msg = tx.GetMsg()
-	if msg == nil {
+	var msgs = tx.GetMsgs()
+	if msgs == nil {


or len(msgs) == 0, I think, no reason to allow transactions with zero messages

cwgoes · 2018-06-20T19:40:34Z

baseapp/baseapp.go


-	// Set gas utilized
-	result.GasUsed = ctx.GasMeter().GasConsumed()
+		result = handler(ctx, msg)


We need to combine the results in some reasonably intelligent manner:

Return all codes / data / logs / info / tags

Add up gas used, return total used at the end

Good point.

Seems like we can just append data and logs together, possibly with some seperator in between individual msg data/logs.

Tags and ValidatorUpdates should also be appended but dropping redundant elements.

Where is GasWanted set? Do we have to change its behavior with multiple msgs?

The code should only be nonzero if the msg fails, right?

Maybe we should do this:

Since we abort running the tx on the first message that fails, the result on a failed transaction should just be the result of the first failed message.

In the case where the tx successfully executes, we accumulate data/logs/info/tags in the way described above and return the result with code 0.

I think this makes it much easier to parse a failed result of a transaction since the user only cares about which message caused the failure.

Thoughts?

Seems like we can just append data and logs together, possibly with some seperator in between individual msg data/logs.

I think the invariant always needs to hold that running a multi-Msg transaction has the same effect on the state as running the Msgs individually in separate transactions:

Tags should be appended, we do not want to drop elements, this may block on Switch to list of lists of tags for transaction results, begin block, and end block tendermint/abci#273

ValidatorUpdates need to be combined, not dropping redundant elements but rather adding/removing power, the updates should be cumulative

Not sure about GasWanted, I don't understand why this field is in Result anyways. For now this can probably be set to the sum of GasWanted for all the individual results.

Since we abort running the tx on the first message that fails, the result on a failed transaction should just be the result of the first failed message. In the case where the tx successfully executes, we accumulate data/logs/info/tags in the way described above and return the result with code 0.

I think this is OK (to follow ABCI, we have to do this anyways); it would be nice to log that the previous messages succeeded somehow for ease of debugging.

Quick question:

In ValidatorUpdates, do we expect address and pubkey to be set correctly. Or do we expect just one to be set correctly like we currently do in x/auth/BaseAccount?

I think the invariant always needs to hold that running a multi-Msg transaction has the same effect on the consensus state as running the Msgs individually in separate transactions

This currently is not respected with the way I handle transaction handling. If msgs are run in individual transactions, then the sequence number will increment each time.

If 5 msgs in the same transaction have the same signer, when that transaction is processed, the sequence number for the signer address is only incremented once.
This is because signer addresses are accumulated. (See stdTx.GetSigners() in x/auth/std_tx.go)

I think this is the behavior we want, but it will break the invariant.

In ValidatorUpdates, do we expect address and pubkey to be set correctly. Or do we expect just one to be set correctly like we currently do in x/auth/BaseAccount?

Send the address, I think we're getting rid of pubkeys in expectation of large future pubkeys (e.g. BLS).

If 5 msgs in the same transaction have the same signer, when that transaction is processed, the sequence number for the signer address is only incremented once.
This is because signer addresses are accumulated. (See stdTx.GetSigners() in x/auth/std_tx.go)

I stand corrected, this is an exception (should be the only one I think).

cwgoes · 2018-06-20T19:41:25Z

baseapp/baseapp_test.go

@@ -638,6 +639,318 @@ func TestValidatorChange(t *testing.T) {

 //----------------------------------------

+// Use burn and send msg types to test multiple msgs in one tx
+type testBurnMsg struct {


Will now need a GetMemo() function (which can return an empty string); rebase against develop (#1280 has since been merged).

cwgoes · 2018-06-20T19:42:27Z

docs/guides/guide.md

@@ -15,7 +15,7 @@ The design of the Cosmos SDK is based on the principles of "capabilities systems
 ## Tx & Msg

 The SDK distinguishes between transactions (Tx) and messages
-(Msg).  A Tx is a Msg wrapped with authentication and fee data.
+(Msg).  A Tx is a list of Msg's wrapped with authentication and fee data.


Msgs, no apostrophe

cwgoes · 2018-06-20T19:49:03Z

x/auth/ante_test.go

-func newTestTx(ctx sdk.Context, msg sdk.Msg, privs []crypto.PrivKey, accNums []int64, seqs []int64, fee StdFee) sdk.Tx {
-	signBytes := StdSignBytes(ctx.ChainID(), accNums, seqs, fee, msg)
-	return newTestTxWithSignBytes(msg, privs, accNums, seqs, fee, signBytes)
+func newTestTx(ctx sdk.Context, msgs []sdk.Msg, privs []crypto.PrivKey, accNums []int64, seqs []int64, fee StdFee) sdk.Tx {


Can we add a new test transaction type with multiple required signers (with different account numbers / sequences) and test all the signature validity checking for that case?

Don't believe we need a new test transaction type for that? The signers, account numbers, and sequences are accumulated from the msgs passed in.

Tests for this were strewn across the test file. In my latest commit, I have a seperate function that tests multimsg signer functionality with different account numbers/sequences.

…l/cosmos-sdk into aditya/multiple-msgs

AdityaSripal · 2018-06-21T18:45:32Z

I think all requested changes have been addressed. @cwgoes

Addressed comments

cwgoes

See comment re: validator set updates

cwgoes · 2018-06-21T19:32:33Z

baseapp/baseapp.go

+		logs = append(logs, fmt.Sprintf("Msg %d: %s", i + 1, finalResult.Log))
+
+		// Cumulate Validator updates
+		for _, val := range result.ValidatorUpdates {


I think this logic isn't quite right. Need to deal with the case of a power decrease too (which will have a lower power, not a negative power). Look at what Tendermint does to compute the change.

Can we separate this out into a function and add a few unit tests? Clearer & might be useful elsewhere.

Haha, nevermind - ValidatorSetUpdates should never have been in the result type to begin with.

So deliverTx should never change the validator set, right? It all happens in EndBlocker?

Addressed comments

cwgoes · 2018-06-21T21:30:27Z

x/auth/stdtx.go

+		AccountNumber: accnum,
+		Sequence:      sequence,
+		FeeBytes:      fee.Bytes(),
+		MsgsBytes:     msgBytes,


Why did we remove the memo?

cwgoes · 2018-06-21T21:44:22Z

Also needs a changelog update

…l/cosmos-sdk into aditya/multiple-msgs

cwgoes

utACK

We need to clean up baseapp a bit, although this PR doesn't need to block on it - #1329.

cwgoes · 2018-06-22T20:31:03Z

Note on the result.ValidatorUpdates removal:

The sdk.Result type previously had a ValidatorUpdates field for accumulated Tendermint validator updates. This isn't a part of ABCI - it was just a convenience field exposed by the baseapp API to allow transactions to change the validator set directly.

As part of this PR, we discovered that there isn't a well-defined way to accumulate validator set updates for multi-Msg transactions - depending on whether modules track updates in their own store or track a diff from the validator set sent by Tendermint in abci.RequestBeginBlock, a different accumulation function would need to be used (and the existing accumulation function in baseapp was wrong in both cases).

Gaia didn't set ValidatorUpdates on any DeliverTx results anyways, since all updates need to be handled through the staking keeper, which updates the validator store and accumulates the necessary changes to send to Tendermint at the end of the block.

We thought restricting ValidatorUpdates to EndBlock was thus preferable, since it makes what modules need to do more obvious and avoids the ambiguous case for multi-Msg transactions - so this PR removed ValidatorUpdates from the sdk.Result type.

* Started work on multiple msgs, types and x/auth tests pass * Fix issues in x, examples, and baseapp * Added baseapp tests for multiple msgs * Documentation fixes * Fix baseapp tests with sdk.Int * Modify test * Transaction handling is now atomic * Fix test comment * Minor doc fixes and code cleanup * Added baseapp result changes * Use address in validator update accumulation * Started work on multiple msgs, types and x/auth tests pass * Fix issues in x, examples, and baseapp * Added baseapp tests for multiple msgs * Documentation fixes * Fix baseapp tests with sdk.Int * Modify test * Transaction handling is now atomic * Fix test comment * Minor doc fixes and code cleanup * Added baseapp result changes * Use address in validator update accumulation * Added ante tests for multisigner * Remove validatorUpdates from tx result * Better error logs * Put Memo in StdSignBytes and formatting * Updated changelog

Started work on multiple msgs, types and x/auth tests pass

761ec7b

AdityaSripal requested a review from ebuchman as a code owner June 15, 2018 06:39

AdityaSripal added 3 commits June 15, 2018 12:58

Fix issues in x, examples, and baseapp

9675f70

go fmt

0d7cdf1

Added baseapp tests for multiple msgs

409e926

AdityaSripal added 3 commits June 16, 2018 17:56

fmt

34ded33

Revert "fmt"

dba29f5

This reverts commit 34ded33.

fmt

a70f16d

cwgoes mentioned this pull request Jun 17, 2018

Add --async option to all send commands #1193

Closed

AdityaSripal added 3 commits June 18, 2018 13:35

Documentation fixes

6c22067

Fix merge conflicts

f90c488

Fix baseapp tests with sdk.Int

5a822d1

AdityaSripal changed the title ~~WIP: Multiple messages~~ Multiple messages Jun 18, 2018

AdityaSripal added ready-for-review labels Jun 18, 2018

Modify test

fc0918f

Transaction handling is now atomic

90a6592

AdityaSripal and others added 3 commits June 18, 2018 21:58

Fix test comment

0a842ea

Minor doc fixes and code cleanup

b786e60

Merge branch 'develop' into aditya/multiple-msgs

505e73a

cwgoes previously requested changes Jun 20, 2018

View reviewed changes

AdityaSripal added 4 commits June 20, 2018 17:55

Added baseapp result changes

b5b2403

Merge branch 'aditya/multiple-msgs' of https://github.com/AdityaSripa…

00ef0ed

…l/cosmos-sdk into aditya/multiple-msgs

Use address in validator update accumulation

bda3b1d

Started work on multiple msgs, types and x/auth tests pass

ad303f7

AdityaSripal added 8 commits June 20, 2018 23:42

Transaction handling is now atomic

1631b57

Fix test comment

b596b93

Minor doc fixes and code cleanup

67e3a1a

Added baseapp result changes

c256d0b

Use address in validator update accumulation

6548b7b

Fix rebase conflicts

183a608

Merge branch 'develop' into aditya/multiple-msgs

b77ce46

Added ante tests for multisigner

aeb8e2a

cwgoes previously requested changes Jun 21, 2018

View reviewed changes

Remove validatorUpdates from tx result

a9ff08b

Better error logs

6974888

cwgoes reviewed Jun 21, 2018

View reviewed changes

Merge branch 'develop' into aditya/multiple-msgs

265867f

AdityaSripal added 3 commits June 21, 2018 14:45

Put Memo in StdSignBytes and formatting

230cb42

Merge branch 'aditya/multiple-msgs' of https://github.com/AdityaSripa…

60f7743

…l/cosmos-sdk into aditya/multiple-msgs

Updated changelog

242dd4c

cwgoes approved these changes Jun 21, 2018

View reviewed changes

cwgoes merged commit f049a56 into cosmos:develop Jun 21, 2018

This was referenced Jun 21, 2018

Tx Structure #669

Closed

Multiple Msgs in a Tx #903

Closed

Signers should only sign their own sequence #906

Closed

tx.GetSigners() accumulates from all the msg.GetSigners #907

Closed

cwgoes mentioned this pull request Jul 18, 2018

Pass error message back through sdk.Result log #1524

Closed

nicolaslara mentioned this pull request Apr 5, 2023

feat: add non-atomic multi-msg transactions #15038

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple messages #1266

Multiple messages #1266

AdityaSripal commented Jun 15, 2018 •

edited

Loading

cwgoes commented Jun 16, 2018

AdityaSripal commented Jun 18, 2018

cwgoes commented Jun 19, 2018 •

edited

Loading

AdityaSripal commented Jun 19, 2018

cwgoes commented Jun 19, 2018

cwgoes left a comment

cwgoes Jun 20, 2018

cwgoes Jun 20, 2018

AdityaSripal Jun 20, 2018

cwgoes Jun 20, 2018 •

edited

Loading

AdityaSripal Jun 21, 2018

AdityaSripal Jun 21, 2018

cwgoes Jun 21, 2018

cwgoes Jun 20, 2018

cwgoes Jun 20, 2018

cwgoes Jun 20, 2018

AdityaSripal Jun 21, 2018

AdityaSripal commented Jun 21, 2018

cwgoes left a comment

cwgoes Jun 21, 2018

cwgoes Jun 21, 2018

AdityaSripal Jun 21, 2018

cwgoes Jun 21, 2018

cwgoes commented Jun 21, 2018

cwgoes left a comment

cwgoes commented Jun 22, 2018 •

edited

Loading

Multiple messages #1266

Multiple messages #1266

Conversation

AdityaSripal commented Jun 15, 2018 • edited Loading

cwgoes commented Jun 16, 2018

AdityaSripal commented Jun 18, 2018

cwgoes commented Jun 19, 2018 • edited Loading

AdityaSripal commented Jun 19, 2018

cwgoes commented Jun 19, 2018

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Jun 21, 2018

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Jun 21, 2018

cwgoes left a comment

Choose a reason for hiding this comment

cwgoes commented Jun 22, 2018 • edited Loading

AdityaSripal commented Jun 15, 2018 •

edited

Loading

cwgoes commented Jun 19, 2018 •

edited

Loading

cwgoes Jun 20, 2018 •

edited

Loading

cwgoes commented Jun 22, 2018 •

edited

Loading