Skip to content
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.

Sign up for GitHub

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

Jump to bottom

R4R: Multimsg ante handler Jae's proposal #3787

Closed
wants to merge 10 commits into from
Closed

R4R: Multimsg ante handler Jae's proposal #3787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c775602
Jae's proposal
sunnya97 Mar 2, 2019
ffbda5c
Remove SendEnabled
jaekwon Mar 3, 2019
2907ef1
fix lcd test
jaekwon Mar 3, 2019
10af5c2
fix bug
jaekwon Mar 4, 2019
60890da
atomsToUatoms
sunnya97 Mar 4, 2019
ddbe903
Update cmd/gaia/app/app.go
alexanderbez Mar 4, 2019
e27b979
Update cmd/gaia/app/app.go
sunnya97 Mar 4, 2019
c15322c
Update cmd/gaia/app/app.go
sunnya97 Mar 4, 2019
d2e9c5f
cli tests run w/ send_enabled gotag
jaekwon Mar 5, 2019
5255fa6
fix comment
jaekwon Mar 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
PACKAGES_NOSIMULATION=$(shell go list ./... | grep -v '/simulation')
# TODO: After transfers are enabled, remove references to send_enabled in this Makefile

PACKAGES_NOSIMULATION=$(shell go list ./... | grep -v '/simulation' | grep -v '/cmd/gaia/cli_test')
PACKAGES_SIMTEST=$(shell go list ./... | grep '/simulation')
VERSION := $(shell echo $(shell git describe --tags) | sed 's/^v//')
COMMIT := $(shell git log -1 --format='%H')
BUILD_TAGS = netgo
BUILD_TAGS = netgo $(BUILD_TAGS_ENV)
CAT := $(if $(filter $(OS),Windows_NT),type,cat)
BUILD_FLAGS = -tags "$(BUILD_TAGS)" -ldflags \
'-X github.com/cosmos/cosmos-sdk/version.Version=$(VERSION) \
Expand Down Expand Up @@ -63,6 +65,10 @@ else
go build $(BUILD_FLAGS) -o build/gaiakeyutil ./cmd/gaia/cmd/gaiakeyutil
endif

# TODO remove after sends are enabled; temporary
build_send_enabled:
BUILD_TAGS_ENV=send_enabled $(MAKE) build

build-linux: vendor-deps
LEDGER_ENABLED=false GOOS=linux GOARCH=amd64 $(MAKE) build

Expand Down Expand Up @@ -92,6 +98,7 @@ check_tools:

update_tools:
@echo "--> Updating tools to correct version"
# TODO is this flag used, and does it need to be a recursive call?
$(MAKE) --always-make tools

update_dev_tools:
Expand Down Expand Up @@ -140,10 +147,12 @@ godocs:
########################################
### Testing

test: test_unit
test: test_unit test_cli

test_cli:
@go test -p 4 `go list github.com/cosmos/cosmos-sdk/cmd/gaia/cli_test` -tags=cli_test
test_cli: build_send_enabled
@go test -p 4 `go list ./cmd/gaia/cli_test/...` -tags=cli_test
# defensively delete build which has send enabled.
rm build/gaiad

test_ledger:
# First test with mock
Expand All @@ -152,10 +161,10 @@ test_ledger:
@go test -v `go list github.com/cosmos/cosmos-sdk/crypto` -tags='cgo ledger'

test_unit:
@VERSION=$(VERSION) go test $(PACKAGES_NOSIMULATION) -tags='ledger test_ledger_mock'
@VERSION=$(VERSION) go test $(PACKAGES_NOSIMULATION) -tags='ledger test_ledger_mock send_enabled'

test_race:
@VERSION=$(VERSION) go test -race $(PACKAGES_NOSIMULATION)
@VERSION=$(VERSION) go test -race $(PACKAGES_NOSIMULATION) -tags='send_enabled'

test_sim_gaia_nondeterminism:
@echo "Running nondeterminism test..."
Expand Down
2 changes: 1 addition & 1 deletion client/lcd/test_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,7 @@ func doTransfer(
resp, body, recvAddr := doTransferWithGas(
t, port, seed, name, memo, pwd, addr, "", 1.0, false, true, fees,
)
require.Equal(t, http.StatusOK, resp.StatusCode, resp)
require.Equal(t, http.StatusOK, resp.StatusCode, body)

var txResp sdk.TxResponse
err := cdc.UnmarshalJSON([]byte(body), &txResp)
Expand Down
58 changes: 57 additions & 1 deletion cmd/gaia/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ const (
DefaultKeyPass = "12345678"
)

var (
// Temporary vars for cosmos launch w/ send disabled.
atomsToUatoms = int64(1000000)
// Sends are enabled for cli tests via go tags.
sendEnabled = false
)

// default home directories for expected binaries
var (
DefaultCLIHome = os.ExpandEnv("$HOME/.gaiacli")
Expand Down Expand Up @@ -170,7 +177,32 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest b
)
app.SetInitChainer(app.initChainer)
app.SetBeginBlocker(app.BeginBlocker)
app.SetAnteHandler(auth.NewAnteHandler(app.accountKeeper, app.feeCollectionKeeper))

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) {
Copy link
Contributor

@alexanderbez alexanderbez Mar 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@alexanderbez alexanderbez Mar 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@alexanderbez alexanderbez Mar 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative as suggested by @sunnya97: #3807

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

newCtx, result, abort = authAnteHandler(ctx, tx, simulate)
if abort == true {
return
}

// TODO remove this once transfers are enabled.
if !sendEnabled {
for _, msg := range tx.GetMsgs() {
switch msg.(type) {
case bank.MsgSend:
return ctx, bank.ErrSendDisabled(bank.DefaultCodespace).Result(), true
case bank.MsgMultiSend:
if !validateMultiSendTransfersDisabled(msg.(bank.MsgMultiSend)) {
return ctx, bank.ErrSendDisabled(bank.DefaultCodespace).Result(), true
}
}
}
}
return
}

app.SetAnteHandler(filterAnteHandler)
app.SetEndBlocker(app.EndBlocker)

if loadLatest {
Expand All @@ -183,6 +215,30 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest b
return app
}

// CheckTransferDisabledBurnMultiSend
func validateMultiSendTransfersDisabled(msg bank.MsgMultiSend) bool {
nineAtoms := sdk.Coins{sdk.NewInt64Coin("uatom", 9*atomsToUatoms)}
oneAtom := sdk.Coins{sdk.NewInt64Coin("uatom", 1*atomsToUatoms)}

if len(msg.Inputs) != 1 {
return false
}
if len(msg.Outputs) != 2 {
return false
}

if !msg.Outputs[0].Address.Equals(bank.BurnedCoinsAccAddr) {
return false
}
if !msg.Outputs[0].Coins.IsEqual(nineAtoms) {
return false
}
if !msg.Outputs[1].Coins.IsEqual(oneAtom) {
return false
}
return true
}

// custom tx codec
func MakeCodec() *codec.Codec {
var cdc = codec.New()
Expand Down
2 changes: 1 addition & 1 deletion cmd/gaia/app/sim_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ func appStateRandomizedFn(r *rand.Rand, accs []simulation.Account, genesisTimest
}
fmt.Printf("Selected randomly generated auth parameters:\n\t%+v\n", authGenesis)

bankGenesis := bank.NewGenesisState(r.Int63n(2) == 0)
bankGenesis := bank.NewGenesisState()
fmt.Printf("Selected randomly generated bank parameters:\n\t%+v\n", bankGenesis)

// Random genesis states
Expand Down
7 changes: 7 additions & 0 deletions cmd/gaia/app/tag_send_enabled.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// +build send_enabled

package app

func init() {
sendEnabled = true
}
Loading