R4R: Multimsg ante handler Jae's proposal #3787

sunnya97 · 2019-03-02T05:18:06Z

closes: #3802

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

cmd/gaia/app/app.go

codecov · 2019-03-03T10:45:37Z

Codecov Report

Merging #3787 into develop will decrease coverage by 0.09%.
The diff coverage is 18.75%.

@@            Coverage Diff            @@
##           develop   #3787     +/-   ##
=========================================
- Coverage       61%   60.9%   -0.1%     
=========================================
  Files          191     191             
  Lines        14174   14187     +13     
=========================================
- Hits          8647    8641      -6     
- Misses        4973    4993     +20     
+ Partials       554     553      -1

zmanian · 2019-03-03T16:00:56Z

The motivating requirements here are

We want to disincentives transfers during the initial network stand up because of validators set and software instability risks.
It is desirable that users be able to create new accounts that are not in genesis such as multisigs for validator operators.

The goal here is a minimal change that will be undone during a breaking upgrade to enable transfering a small number of atoms to create new validator operator accounts.

cwgoes

I missed most of the motivating discussion here - I guess the idea is to allow un-economical transfers to make it possible to create new validators / accounts, like Zaki said? Given that, this approach seems OK to me.

A few comments on implementation, and needs PENDING.md

cmd/gaia/app/app.go

alexanderbez

Thanks @sunnya97 -- surface changes make sense mostly, but:

needs a pending log entry
needs a corresponding issue w/ a summary as this is a big change right before launch and there virtually is zero context here
comments addressed

alexanderbez · 2019-03-04T14:45:08Z

cmd/gaia/app/app.go

+			switch msg.(type) {
+			case bank.MsgSend:
+				// LCD tests requires this to be commented out
+				// return ctx, bank.ErrSendDisabled(bank.DefaultCodespace).Result(), true


Why? We should modify those tests, no?

This doesn't seem safe, now we'll allow sends (?)

Can I use the Makefile to set an environment variable or something that I can read from in the code? Is that safe?

we can't use an env variable but we can use a build flag, i'll do this.

cmd/gaia/app/app.go

x/bank/keeper.go

cwgoes · 2019-03-04T16:56:07Z

To-do (cc @sunnya97):

Actually disable MsgSend in the ante handler
~~Track total supply correctly~~
Add full integration test coverage, and figure out a way to cleanly merge this (maybe a flag)
Open an issue with instructions on how to revert this change when we enable transfers and add to the 0.34.0 milestone
Document the rationale for this change (Support New Validators at Launch #3802)

Co-Authored-By: sunnya97 <sunnya97@gmail.com>

cmd/gaia/app/app.go

cwgoes · 2019-03-04T21:46:06Z

Hey @sunnya97 - do you think we shouldn't track the total supply changes? Please explicate.

sunnya97 · 2019-03-04T21:49:29Z

@cwgoes The bank module doesn't have access to the stake keeper, which currently keeps track of the total supply. It should be done by the bank keeper. There was a PR for this (#2939) but we decided to push it to post launch. This "burn account" method is currently what is also being used with the governance deposit burns.

cwgoes · 2019-03-04T21:49:38Z

Alternatively, burned tokens could be added back as fees or added to the community pool.

cwgoes · 2019-03-04T21:50:31Z

@cwgoes The bank module doesn't have access to the stake keeper, which currently keeps track of the total supply. It should be done by the bank keeper. There was a PR for this (#2939) but we decided to push it to post launch. This "burn account" method is currently what is also being used with the governance deposit burns.

This doesn't prevent altering the staking pool in the ante handler with the staking keeper.

sunnya97 · 2019-03-04T21:53:07Z

Adding it the community pool would require giving the bank keeper access to the distribution keeper (not sure if this doable directly or would need a hook). Adding is as fees is doable, as bank keeper already has access to auth keeper which has access to FeeCollection keeper. However, by using a "burn address" we can just reuse the already written and tested functionality of MultiSend msg without adding any additional functionality to the bank module.

sunnya97 · 2019-03-04T21:55:45Z

This doesn't prevent altering the staking pool in the ante handler with the staking keeper.

We don't process the the MultiSend msg in the ante handler, we just check that it has the right amounts in the outputs. You can think of it as an extra ValidateBasic we do. The actual handling of the MultiSend is done by the normal MultiSend handler. The multisend handler checks to make sure the sending account actually has enough money to do the transfer. We can't deduct from the total supply until we know that the multisend msg actually goes through.

jaekwon · 2019-03-05T04:08:44Z

@cwgoes I don't see why we need to filter SetWithdrawAddress messages. That parameter still exists, and whether it's enabled or not, it's not the same thing as enabling transfers per se, it only applies to rewards AFAIK, so I think we can leave this PR as is.

cwgoes · 2019-03-05T08:53:41Z

This doesn't prevent altering the staking pool in the ante handler with the staking keeper.

We don't process the the MultiSend msg in the ante handler, we just check that it has the right amounts in the outputs. You can think of it as an extra ValidateBasic we do. The actual handling of the MultiSend is done by the normal MultiSend handler. The multisend handler checks to make sure the sending account actually has enough money to do the transfer. We can't deduct from the total supply until we know that the multisend msg actually goes through.

Right, I stand corrected. I guess we would need to add a new message type or override the handler itself. Neither option seems particularly clean (although this isn't either).

We can fix total supply when we hard fork to actually enable transfers (and maybe fix the x/gov burn mechanism then too), so I withdraw my objection.

cwgoes · 2019-03-05T08:55:02Z

@cwgoes I don't see why we need to filter SetWithdrawAddress messages. That parameter still exists, and whether it's enabled or not, it's not the same thing as enabling transfers per se, it only applies to rewards AFAIK, so I think we can leave this PR as is.

It seems like a proxy for (limited) transfers - I could pay someone (a validator) to set their reward address to one I control. We don't require the reward address to sign the SetWithdrawAddress message (so it can be a new account).

MsgSetWithdrawAddress is already separately disabled with a genesis parameter though - we only need to filter in this PR (and remove that logic) if we want to have everything in one place.

sunnya97 · 2019-03-05T09:24:59Z

We don't require the reward address to sign the SetWithdrawAddress message (so it can be a new account).

Although, to do the actual WithdrawTx, won't that account need to have an account in state and some coins to pay the tx fee with?

MsgSetWithdrawAddress is already separately disabled with a genesis parameter though - we only need to filter in this PR (and remove that logic) if we want to have everything in one place.

I do like putting these all in the same spot. Will be easier to activate transfers later then.

cwgoes · 2019-03-05T10:34:01Z

Although, to do the actual WithdrawTx, won't that account need to have an account in state and some coins to pay the tx fee with?

The withdraw transaction is still sent by the delegator, so no - and as soon as the reward recipient receives funds, it will be able to spend them, like after a normal transfer.

(perhaps this should be reworked in the future, it's not actually that useful)

alexanderbez

Left a question and this still warrants a pending log entry.

alexanderbez · 2019-03-05T14:05:16Z

cmd/gaia/app/app.go

+
+	authAnteHandler := auth.NewAnteHandler(app.accountKeeper, app.feeCollectionKeeper)
+
+	filterAnteHandler := func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, result sdk.Result, abort bool) {


Hmmm maybe I'm missing something glaringly obvious, but why must we have a "filterAnteHandler" in app? Why can't we just use the regular ante handler and have the sendEnabled enabled check? Then we won't need any hacky build stuff for this. Yes, the ante handler would have to be provided a "keeper contract" that implements GetSendEnabled.

What if a bunch of nodes choose to run without this build setup? What if this introduces a spam or DoS vector? (since the entire ante handler is executed first)

Ohhh because it needs access to x/bank for message validation....sigh. I still think we can avoid this though. NewAnteHandler can be passed a tx validator function (which is defined in bank, e.g. func(tx sdk.Tx) sdk.Result) . Then we dont need any filter ante handler OR hacky build steps.

Because other people are using the bank module rn, and we don't want to add Hub specific functionality to the module. We wanted to isolate this functionality change to just the gaia package.

Really, the proper way to do this is to allow for rerouting subroutes. So the route of the bank module msgs would be:
bank/send
bank/multisend

And then in the router, we route
bank/* to the bank module, but bank/multisend to a special handler defined in the gaia folder.

However, we don't have time to do this rn, as this would involve changes to the router functionality.

I guess we could just temporarily fork the entire bank module and add it as a special bank module in the gaia folder. And then to enable transfers point it back to the SDK bank module. That's actually not a bad idea.....

@cwgoes @jaekwon @zmanian

Yeah anything to avoid involving the build pipeline would be an ideal solution. Seems trivial to do @sunnya97

I have all of these same concerns about using the build pipeline. I think, while messy, that forking the bank module may be the best way to do this. Looking forward to continuing this discussion in the sdk meeting today.

Also this will be a very easy change to revert if we do that (just reimport the normal bank module into app.go)

we don't know how messy it will be, this is done, lets go with this.

Alternative as suggested by @sunnya97: #3807

It's not messy at all. I really don't think we should rely on the build process to enforce business logic (especially since it ties into consensus).

jackzampolin · 2019-03-05T19:27:00Z

Current plan is to use the solution in #3807, going to mark this will-close

Jae's proposal

c775602

sunnya97 requested review from cwgoes, ebuchman and rigelrozanski as code owners March 2, 2019 05:18

jaekwon reviewed Mar 2, 2019

View reviewed changes

cmd/gaia/app/app.go Outdated Show resolved Hide resolved

jaekwon reviewed Mar 2, 2019

View reviewed changes

cmd/gaia/app/app.go Outdated Show resolved Hide resolved

jaekwon reviewed Mar 2, 2019

View reviewed changes

cmd/gaia/app/app.go Outdated Show resolved Hide resolved

jaekwon mentioned this pull request Mar 2, 2019

WIP: Hacky multisend pre transfers #3786

Closed

5 tasks

Remove SendEnabled

ffbda5c

fix lcd test

2907ef1

jackzampolin mentioned this pull request Mar 3, 2019

Validator Creation Updates #3789

Closed

4 tasks

jackzampolin added this to the v0.33.0 (Launch) milestone Mar 3, 2019

fix bug

10af5c2

jaekwon changed the title ~~WIP: Multimsg ante handler Jae's proposal~~ R4R: Multimsg ante handler Jae's proposal Mar 4, 2019

cwgoes suggested changes Mar 4, 2019

View reviewed changes

cmd/gaia/app/app.go Outdated Show resolved Hide resolved

cmd/gaia/app/app.go Outdated Show resolved Hide resolved

cmd/gaia/app/app.go Outdated Show resolved Hide resolved

cmd/gaia/app/app.go Outdated Show resolved Hide resolved

alexanderbez suggested changes Mar 4, 2019

View reviewed changes

alexanderbez added C:baseapp ready-for-review T: State Machine Breaking State machine breaking changes (impacts consensus). labels Mar 4, 2019

cwgoes mentioned this pull request Mar 4, 2019

Support New Validators at Launch #3802

Closed

4 tasks

sunnya97 and others added 2 commits March 4, 2019 22:30

atomsToUatoms

60890da

Update cmd/gaia/app/app.go

ddbe903

Co-Authored-By: sunnya97 <sunnya97@gmail.com>

sunnya97 commented Mar 4, 2019

View reviewed changes

cmd/gaia/app/app.go Outdated Show resolved Hide resolved

Update cmd/gaia/app/app.go

e27b979

jackzampolin reviewed Mar 4, 2019

View reviewed changes

cmd/gaia/app/app.go Outdated Show resolved Hide resolved

sunnya97 commented Mar 4, 2019

View reviewed changes

cmd/gaia/app/app.go Outdated Show resolved Hide resolved

Update cmd/gaia/app/app.go

c15322c

cli tests run w/ send_enabled gotag

d2e9c5f

fix comment

5255fa6

alexanderbez suggested changes Mar 5, 2019

View reviewed changes

alexanderbez mentioned this pull request Mar 5, 2019

Custom MsgMultiSend Handler (x/bank fork) #3807

Merged

5 tasks

jackzampolin added will-close and removed ready-for-review labels Mar 5, 2019

cwgoes closed this Mar 6, 2019

cwgoes deleted the sunny/multimsg-ante-handler branch March 6, 2019 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Multimsg ante handler Jae's proposal #3787

R4R: Multimsg ante handler Jae's proposal #3787

sunnya97 commented Mar 2, 2019 •

edited by alexanderbez

Loading

codecov bot commented Mar 3, 2019 •

edited

Loading

zmanian commented Mar 3, 2019

cwgoes left a comment •

edited

Loading

alexanderbez left a comment

alexanderbez Mar 4, 2019

cwgoes Mar 4, 2019

sunnya97 Mar 4, 2019 •

edited

Loading

jaekwon Mar 5, 2019

cwgoes commented Mar 4, 2019 •

edited by sunnya97

Loading

cwgoes commented Mar 4, 2019

sunnya97 commented Mar 4, 2019

cwgoes commented Mar 4, 2019

cwgoes commented Mar 4, 2019 •

edited

Loading

sunnya97 commented Mar 4, 2019 •

edited

Loading

sunnya97 commented Mar 4, 2019

jaekwon commented Mar 5, 2019

cwgoes commented Mar 5, 2019

cwgoes commented Mar 5, 2019 •

edited

Loading

sunnya97 commented Mar 5, 2019 •

edited

Loading

cwgoes commented Mar 5, 2019 •

edited

Loading

alexanderbez left a comment

alexanderbez Mar 5, 2019 •

edited

Loading

alexanderbez Mar 5, 2019 •

edited

Loading

sunnya97 Mar 5, 2019

sunnya97 Mar 5, 2019

alexanderbez Mar 5, 2019 •

edited

Loading

jackzampolin Mar 5, 2019

jackzampolin Mar 5, 2019

jaekwon Mar 5, 2019

alexanderbez Mar 5, 2019

alexanderbez Mar 5, 2019

jackzampolin commented Mar 5, 2019


		authAnteHandler := auth.NewAnteHandler(app.accountKeeper, app.feeCollectionKeeper)

		filterAnteHandler := func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, result sdk.Result, abort bool) {

R4R: Multimsg ante handler Jae's proposal #3787

R4R: Multimsg ante handler Jae's proposal #3787

Conversation

sunnya97 commented Mar 2, 2019 • edited by alexanderbez Loading

codecov bot commented Mar 3, 2019 • edited Loading

Codecov Report

zmanian commented Mar 3, 2019

cwgoes left a comment • edited Loading

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunnya97 Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Mar 4, 2019 • edited by sunnya97 Loading

cwgoes commented Mar 4, 2019

sunnya97 commented Mar 4, 2019

cwgoes commented Mar 4, 2019

cwgoes commented Mar 4, 2019 • edited Loading

sunnya97 commented Mar 4, 2019 • edited Loading

sunnya97 commented Mar 4, 2019

jaekwon commented Mar 5, 2019

cwgoes commented Mar 5, 2019

cwgoes commented Mar 5, 2019 • edited Loading

sunnya97 commented Mar 5, 2019 • edited Loading

cwgoes commented Mar 5, 2019 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

alexanderbez Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Mar 5, 2019 • 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

jackzampolin commented Mar 5, 2019

sunnya97 commented Mar 2, 2019 •

edited by alexanderbez

Loading

codecov bot commented Mar 3, 2019 •

edited

Loading

cwgoes left a comment •

edited

Loading

sunnya97 Mar 4, 2019 •

edited

Loading

cwgoes commented Mar 4, 2019 •

edited by sunnya97

Loading

cwgoes commented Mar 4, 2019 •

edited

Loading

sunnya97 commented Mar 4, 2019 •

edited

Loading

cwgoes commented Mar 5, 2019 •

edited

Loading

sunnya97 commented Mar 5, 2019 •

edited

Loading

cwgoes commented Mar 5, 2019 •

edited

Loading

alexanderbez Mar 5, 2019 •

edited

Loading

alexanderbez Mar 5, 2019 •

edited

Loading

alexanderbez Mar 5, 2019 •

edited

Loading