From 59f4bfe90556ee3b13f6a99fb27759d1ceedfa49 Mon Sep 17 00:00:00 2001 From: kakysha Date: Mon, 10 Jun 2024 20:01:22 +0300 Subject: [PATCH 1/5] feat: SendCoins - fine-grained sendRestriction per coin --- x/bank/keeper/send.go | 91 +++++++++++++++++++++++------------- x/bank/types/restrictions.go | 6 +-- 2 files changed, 62 insertions(+), 35 deletions(-) diff --git a/x/bank/keeper/send.go b/x/bank/keeper/send.go index 3435c4c6bc79..eb34e8491e05 100644 --- a/x/bank/keeper/send.go +++ b/x/bank/keeper/send.go @@ -217,46 +217,73 @@ func (k BaseSendKeeper) SendCoins(ctx context.Context, fromAddr, toAddr sdk.AccA return err } - toAddr, err := k.sendRestriction.apply(ctx, fromAddr, toAddr, amt) - if err != nil { - return err - } + var ( + err error + isSuccess bool // if at least one send succeedes, we proceed + toAddresses = make([]sdk.AccAddress, len(amt)) + ) + // bech32 encoding is expensive! Only do it once for fromAddr + fromAddrString := fromAddr.String() + sdkCtx := sdk.UnwrapSDKContext(ctx) + events := sdk.Events{} - err = k.subUnlockedCoins(ctx, fromAddr, amt, true) - if err != nil { - return err - } + for i, coin := range amt { + toAddr, err = k.sendRestriction.apply(ctx, fromAddr, toAddr, coin) - err = k.addCoins(ctx, toAddr, amt) - if err != nil { - return err + if err != nil { + continue + } + + toAddresses[i] = toAddr + + isSuccess = true } - // Create account if recipient does not exist. - // - // NOTE: This should ultimately be removed in favor a more flexible approach - // such as delegated fee messages. - accExists := k.ak.HasAccount(ctx, toAddr) - if !accExists { - defer telemetry.IncrCounter(1, "new", "account") - k.ak.SetAccount(ctx, k.ak.NewAccountWithAddress(ctx, toAddr)) + if !isSuccess { + return err // returning last err from sendRestrictionFn (does it matter which one?) } - // bech32 encoding is expensive! Only do it once for fromAddr - fromAddrString := fromAddr.String() - sdkCtx := sdk.UnwrapSDKContext(ctx) - sdkCtx.EventManager().EmitEvents(sdk.Events{ - sdk.NewEvent( + for i, coin := range amt { + // skip restricted coin + if toAddresses[i] == nil { + continue + } + + coinAmt := sdk.NewCoins(coin) + toAddr = toAddresses[i] + + err = k.subUnlockedCoins(ctx, fromAddr, coinAmt, true) // only sub this coin + if err != nil { + return err + } + + err = k.addCoins(ctx, toAddr, coinAmt) + if err != nil { + return err + } + + // Create account if recipient does not exist. + // + // NOTE: This should ultimately be removed in favor a more flexible approach + // such as delegated fee messages. + accExists := k.ak.HasAccount(ctx, toAddr) + if !accExists { + defer telemetry.IncrCounter(1, "new", "account") + k.ak.SetAccount(ctx, k.ak.NewAccountWithAddress(ctx, toAddr)) + } + + events = events.AppendEvent(sdk.NewEvent( types.EventTypeTransfer, sdk.NewAttribute(types.AttributeKeyRecipient, toAddr.String()), sdk.NewAttribute(types.AttributeKeySender, fromAddrString), - sdk.NewAttribute(sdk.AttributeKeyAmount, amt.String()), - ), - sdk.NewEvent( - sdk.EventTypeMessage, - sdk.NewAttribute(types.AttributeKeySender, fromAddrString), - ), - }) + sdk.NewAttribute(sdk.AttributeKeyAmount, coinAmt.String()), + )) + } + events = events.AppendEvent(sdk.NewEvent( + sdk.EventTypeMessage, + sdk.NewAttribute(types.AttributeKeySender, fromAddrString), + )) + sdkCtx.EventManager().EmitEvents(events) return nil } @@ -529,7 +556,7 @@ func (r *sendRestriction) clear() { var _ types.SendRestrictionFn = (*sendRestriction)(nil).apply // apply applies the send restriction if there is one. If not, it's a no-op. -func (r *sendRestriction) apply(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.AccAddress, error) { +func (r *sendRestriction) apply(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coin) (sdk.AccAddress, error) { if r == nil || r.fn == nil { return toAddr, nil } diff --git a/x/bank/types/restrictions.go b/x/bank/types/restrictions.go index 2ec4489a0978..8cfd760d6015 100644 --- a/x/bank/types/restrictions.go +++ b/x/bank/types/restrictions.go @@ -52,7 +52,7 @@ func ComposeMintingRestrictions(restrictions ...MintingRestrictionFn) MintingRes } // A SendRestrictionFn can restrict sends and/or provide a new receiver address. -type SendRestrictionFn func(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) (newToAddr sdk.AccAddress, err error) +type SendRestrictionFn func(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coin) (newToAddr sdk.AccAddress, err error) // IsOnePerModuleType implements the depinject.OnePerModuleType interface. func (SendRestrictionFn) IsOnePerModuleType() {} @@ -60,7 +60,7 @@ func (SendRestrictionFn) IsOnePerModuleType() {} var _ SendRestrictionFn = NoOpSendRestrictionFn // NoOpSendRestrictionFn is a no-op SendRestrictionFn. -func NoOpSendRestrictionFn(_ context.Context, _, toAddr sdk.AccAddress, _ sdk.Coins) (sdk.AccAddress, error) { +func NoOpSendRestrictionFn(_ context.Context, _, toAddr sdk.AccAddress, _ sdk.Coin) (sdk.AccAddress, error) { return toAddr, nil } @@ -89,7 +89,7 @@ func ComposeSendRestrictions(restrictions ...SendRestrictionFn) SendRestrictionF case 1: return toRun[0] } - return func(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.AccAddress, error) { + return func(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coin) (sdk.AccAddress, error) { var err error for _, r := range toRun { toAddr, err = r(ctx, fromAddr, toAddr, amt) From 818721fe8694f2072bbe1f791bdcb5a83eee62bd Mon Sep 17 00:00:00 2001 From: kakysha Date: Wed, 12 Jun 2024 00:56:30 +0300 Subject: [PATCH 2/5] feat: InputOutputCoins is now per-denom too to align with SendRestrictionFn --- x/bank/keeper/send.go | 136 +++++++++++++++++++++++------------------- 1 file changed, 74 insertions(+), 62 deletions(-) diff --git a/x/bank/keeper/send.go b/x/bank/keeper/send.go index eb34e8491e05..5119b6b0121d 100644 --- a/x/bank/keeper/send.go +++ b/x/bank/keeper/send.go @@ -157,22 +157,49 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input, return err } - outAddresses := make([]sdk.AccAddress, len(outputs)) + outAddresses := make([][]sdk.AccAddress, len(outputs)) + isSuccess := false + for i, out := range outputs { - outAddress, err := k.ak.AddressCodec().StringToBytes(out.Address) + var ( + outAddress, newOutAddress sdk.AccAddress + ) + outAddress, err = k.ak.AddressCodec().StringToBytes(out.Address) if err != nil { return err } + outAddresses[i] = make([]sdk.AccAddress, len(out.Coins)) // out address per denom - outAddresses[i], err = k.sendRestriction.apply(ctx, inAddress, outAddress, out.Coins) - if err != nil { - return err + for j, coin := range out.Coins { + newOutAddress, err = k.sendRestriction.apply(ctx, inAddress, outAddress, coin) + if err != nil { + continue + } + + outAddresses[i][j] = newOutAddress + + isSuccess = true } } - err = k.subUnlockedCoins(ctx, inAddress, input.Coins, true) - if err != nil { - return err + if !isSuccess { + return err // returning last err from sendRestrictionFn (does it matter which one?) + } + + for i, out := range outputs { + for j, coin := range out.Coins { + // skip restricted coin + if outAddresses[i][j] == nil { + continue + } + + toAddr := outAddresses[i][j] + + err := k.sendCoin(ctx, inAddress, toAddr, coin) + if err != nil { + return err + } + } } sdkCtx := sdk.UnwrapSDKContext(ctx) @@ -183,30 +210,6 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input, ), ) - for i, out := range outputs { - if err := k.addCoins(ctx, outAddresses[i], out.Coins); err != nil { - return err - } - - sdkCtx.EventManager().EmitEvent( - sdk.NewEvent( - types.EventTypeTransfer, - sdk.NewAttribute(types.AttributeKeyRecipient, outAddresses[i].String()), - sdk.NewAttribute(sdk.AttributeKeyAmount, out.Coins.String()), - ), - ) - - // Create account if recipient does not exist. - // - // NOTE: This should ultimately be removed in favor a more flexible approach - // such as delegated fee messages. - accExists := k.ak.HasAccount(ctx, outAddresses[i]) - if !accExists { - defer telemetry.IncrCounter(1, "new", "account") - k.ak.SetAccount(ctx, k.ak.NewAccountWithAddress(ctx, outAddresses[i])) - } - } - return nil } @@ -219,13 +222,11 @@ func (k BaseSendKeeper) SendCoins(ctx context.Context, fromAddr, toAddr sdk.AccA var ( err error - isSuccess bool // if at least one send succeedes, we proceed - toAddresses = make([]sdk.AccAddress, len(amt)) + isSuccess bool // if at least one send succeedes, we proceed + toAddresses = make([]sdk.AccAddress, len(amt)) // out address per denom ) // bech32 encoding is expensive! Only do it once for fromAddr fromAddrString := fromAddr.String() - sdkCtx := sdk.UnwrapSDKContext(ctx) - events := sdk.Events{} for i, coin := range amt { toAddr, err = k.sendRestriction.apply(ctx, fromAddr, toAddr, coin) @@ -249,45 +250,56 @@ func (k BaseSendKeeper) SendCoins(ctx context.Context, fromAddr, toAddr sdk.AccA continue } - coinAmt := sdk.NewCoins(coin) toAddr = toAddresses[i] - err = k.subUnlockedCoins(ctx, fromAddr, coinAmt, true) // only sub this coin - if err != nil { - return err - } - - err = k.addCoins(ctx, toAddr, coinAmt) + err := k.sendCoin(ctx, fromAddr, toAddr, coin) if err != nil { return err } - - // Create account if recipient does not exist. - // - // NOTE: This should ultimately be removed in favor a more flexible approach - // such as delegated fee messages. - accExists := k.ak.HasAccount(ctx, toAddr) - if !accExists { - defer telemetry.IncrCounter(1, "new", "account") - k.ak.SetAccount(ctx, k.ak.NewAccountWithAddress(ctx, toAddr)) - } - - events = events.AppendEvent(sdk.NewEvent( - types.EventTypeTransfer, - sdk.NewAttribute(types.AttributeKeyRecipient, toAddr.String()), - sdk.NewAttribute(types.AttributeKeySender, fromAddrString), - sdk.NewAttribute(sdk.AttributeKeyAmount, coinAmt.String()), - )) } - events = events.AppendEvent(sdk.NewEvent( + + sdkCtx := sdk.UnwrapSDKContext(ctx) + sdkCtx.EventManager().EmitEvent(sdk.NewEvent( sdk.EventTypeMessage, sdk.NewAttribute(types.AttributeKeySender, fromAddrString), )) - sdkCtx.EventManager().EmitEvents(events) return nil } +func (k BaseSendKeeper) sendCoin(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coin) error { + err := k.subUnlockedCoins(ctx, fromAddr, sdk.NewCoins(amt), true) // only sub this coin + if err != nil { + return err + } + + err = k.addCoins(ctx, toAddr, sdk.NewCoins(amt)) + if err != nil { + return err + } + + // Create account if recipient does not exist. + // + // NOTE: This should ultimately be removed in favor a more flexible approach + // such as delegated fee messages. + accExists := k.ak.HasAccount(ctx, toAddr) + if !accExists { + defer telemetry.IncrCounter(1, "new", "account") + k.ak.SetAccount(ctx, k.ak.NewAccountWithAddress(ctx, toAddr)) + } + + sdkCtx := sdk.UnwrapSDKContext(ctx) + sdkCtx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypeTransfer, + sdk.NewAttribute(types.AttributeKeyRecipient, toAddr.String()), + sdk.NewAttribute(types.AttributeKeySender, fromAddr.String()), + sdk.NewAttribute(sdk.AttributeKeyAmount, amt.String()), + ), + ) + return nil +} + // validateCoinsBeforeSend is checks extracted from subUnlockedCoins to be run before sendRestrictionFn inside SendCoins func (k BaseSendKeeper) validateCoinsBeforeSend(ctx context.Context, addr sdk.AccAddress, amt sdk.Coins) error { if !amt.IsValid() { From b2463e2a2594a727049c7953268eaff2541807b1 Mon Sep 17 00:00:00 2001 From: kakysha Date: Wed, 12 Jun 2024 16:13:02 +0300 Subject: [PATCH 3/5] fix: revert "do not fail fast" in bank's sends since it should be handled by SendRestrictionFn by not returning error when necessary --- x/bank/keeper/send.go | 74 +++++++------------------------------------ 1 file changed, 12 insertions(+), 62 deletions(-) diff --git a/x/bank/keeper/send.go b/x/bank/keeper/send.go index 5119b6b0121d..d1ed4dea5479 100644 --- a/x/bank/keeper/send.go +++ b/x/bank/keeper/send.go @@ -157,45 +157,19 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input, return err } - outAddresses := make([][]sdk.AccAddress, len(outputs)) - isSuccess := false - - for i, out := range outputs { - var ( - outAddress, newOutAddress sdk.AccAddress - ) - outAddress, err = k.ak.AddressCodec().StringToBytes(out.Address) + for _, out := range outputs { + outAddress, err := k.ak.AddressCodec().StringToBytes(out.Address) if err != nil { return err } - outAddresses[i] = make([]sdk.AccAddress, len(out.Coins)) // out address per denom - for j, coin := range out.Coins { - newOutAddress, err = k.sendRestriction.apply(ctx, inAddress, outAddress, coin) + for _, coin := range out.Coins { + newOutAddress, err := k.sendRestriction.apply(ctx, inAddress, outAddress, coin) if err != nil { - continue - } - - outAddresses[i][j] = newOutAddress - - isSuccess = true - } - } - - if !isSuccess { - return err // returning last err from sendRestrictionFn (does it matter which one?) - } - - for i, out := range outputs { - for j, coin := range out.Coins { - // skip restricted coin - if outAddresses[i][j] == nil { - continue + return err } - toAddr := outAddresses[i][j] - - err := k.sendCoin(ctx, inAddress, toAddr, coin) + err = k.sendCoin(ctx, inAddress, newOutAddress, coin) if err != nil { return err } @@ -220,45 +194,21 @@ func (k BaseSendKeeper) SendCoins(ctx context.Context, fromAddr, toAddr sdk.AccA return err } - var ( - err error - isSuccess bool // if at least one send succeedes, we proceed - toAddresses = make([]sdk.AccAddress, len(amt)) // out address per denom - ) - // bech32 encoding is expensive! Only do it once for fromAddr - fromAddrString := fromAddr.String() - - for i, coin := range amt { - toAddr, err = k.sendRestriction.apply(ctx, fromAddr, toAddr, coin) - + for _, coin := range amt { + newToAddr, err := k.sendRestriction.apply(ctx, fromAddr, toAddr, coin) if err != nil { - continue - } - - toAddresses[i] = toAddr - - isSuccess = true - } - - if !isSuccess { - return err // returning last err from sendRestrictionFn (does it matter which one?) - } - - for i, coin := range amt { - // skip restricted coin - if toAddresses[i] == nil { - continue + return err } - toAddr = toAddresses[i] - - err := k.sendCoin(ctx, fromAddr, toAddr, coin) + err = k.sendCoin(ctx, fromAddr, newToAddr, coin) if err != nil { return err } } sdkCtx := sdk.UnwrapSDKContext(ctx) + // bech32 encoding is expensive! Only do it once for fromAddr + fromAddrString := fromAddr.String() sdkCtx.EventManager().EmitEvent(sdk.NewEvent( sdk.EventTypeMessage, sdk.NewAttribute(types.AttributeKeySender, fromAddrString), From 5cd942fa42f449f6bee6208e952073d73a823ebf Mon Sep 17 00:00:00 2001 From: kakysha Date: Wed, 12 Jun 2024 18:26:03 +0300 Subject: [PATCH 4/5] feat: set DoNotFailFast inside context in consensus code so permissions module fails do not panic --- baseapp/baseapp.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 3cc88aa7dfc1..b13b6e35d0a0 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -43,6 +43,8 @@ type ( // particular, if a module changed the substore key name (or removed a substore) // between two versions of the software. StoreLoader func(ms storetypes.CommitMultiStore) error + + contextKeyT string ) const ( @@ -54,6 +56,8 @@ const ( execModeVoteExtension // Extend or verify a pre-commit vote execModeVerifyVoteExtension // Verify a vote extension execModeFinalize // Finalize a block proposal + + DoNotFailFastContextKey contextKeyT = "DoNotFailFast" ) var _ servertypes.ABCI = (*BaseApp)(nil) @@ -708,7 +712,7 @@ func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context func (app *BaseApp) preBlock(req *abci.RequestFinalizeBlock) error { if app.preBlocker != nil { - ctx := app.finalizeBlockState.Context() + ctx := app.finalizeBlockState.Context().WithValue(DoNotFailFastContextKey, struct{}{}) rsp, err := app.preBlocker(ctx, req) if err != nil { return err @@ -733,7 +737,8 @@ func (app *BaseApp) beginBlock(req *abci.RequestFinalizeBlock) (sdk.BeginBlock, ) if app.beginBlocker != nil { - resp, err = app.beginBlocker(app.finalizeBlockState.Context()) + ctx := app.finalizeBlockState.Context().WithValue(DoNotFailFastContextKey, struct{}{}) + resp, err = app.beginBlocker(ctx) if err != nil { return resp, err } @@ -746,7 +751,6 @@ func (app *BaseApp) beginBlock(req *abci.RequestFinalizeBlock) (sdk.BeginBlock, ) } - ctx := app.finalizeBlockState.ctx app.AddStreamEvents(ctx.BlockHeight(), ctx.BlockTime(), resp.Events, true) resp.Events = sdk.MarkEventsToIndex(resp.Events, app.indexEvents) @@ -797,11 +801,12 @@ func (app *BaseApp) deliverTx(tx []byte) *abci.ExecTxResult { // endBlock is an application-defined function that is called after transactions // have been processed in FinalizeBlock. -func (app *BaseApp) endBlock(ctx context.Context) (sdk.EndBlock, error) { +func (app *BaseApp) endBlock(_ context.Context) (sdk.EndBlock, error) { var endblock sdk.EndBlock if app.endBlocker != nil { - eb, err := app.endBlocker(app.finalizeBlockState.Context()) + ctx := app.finalizeBlockState.Context().WithValue(DoNotFailFastContextKey, struct{}{}) + eb, err := app.endBlocker(ctx) if err != nil { return endblock, err } @@ -814,7 +819,6 @@ func (app *BaseApp) endBlock(ctx context.Context) (sdk.EndBlock, error) { ) } - ctx := app.finalizeBlockState.ctx app.AddStreamEvents(ctx.BlockHeight(), ctx.BlockTime(), eb.Events, true) eb.Events = sdk.MarkEventsToIndex(eb.Events, app.indexEvents) From b0d018abeff98e441490f10f14c4b9876be7c565 Mon Sep 17 00:00:00 2001 From: kakysha Date: Wed, 12 Jun 2024 19:21:31 +0300 Subject: [PATCH 5/5] fix: enable sends fast fail in gov proposals execution --- baseapp/baseapp.go | 8 ++++---- x/gov/abci.go | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index b13b6e35d0a0..927d90f0304d 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -57,7 +57,7 @@ const ( execModeVerifyVoteExtension // Verify a vote extension execModeFinalize // Finalize a block proposal - DoNotFailFastContextKey contextKeyT = "DoNotFailFast" + DoNotFailFastSendContextKey contextKeyT = "DoNotFailFast" ) var _ servertypes.ABCI = (*BaseApp)(nil) @@ -712,7 +712,7 @@ func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context func (app *BaseApp) preBlock(req *abci.RequestFinalizeBlock) error { if app.preBlocker != nil { - ctx := app.finalizeBlockState.Context().WithValue(DoNotFailFastContextKey, struct{}{}) + ctx := app.finalizeBlockState.Context().WithValue(DoNotFailFastSendContextKey, struct{}{}) rsp, err := app.preBlocker(ctx, req) if err != nil { return err @@ -737,7 +737,7 @@ func (app *BaseApp) beginBlock(req *abci.RequestFinalizeBlock) (sdk.BeginBlock, ) if app.beginBlocker != nil { - ctx := app.finalizeBlockState.Context().WithValue(DoNotFailFastContextKey, struct{}{}) + ctx := app.finalizeBlockState.Context().WithValue(DoNotFailFastSendContextKey, struct{}{}) resp, err = app.beginBlocker(ctx) if err != nil { return resp, err @@ -805,7 +805,7 @@ func (app *BaseApp) endBlock(_ context.Context) (sdk.EndBlock, error) { var endblock sdk.EndBlock if app.endBlocker != nil { - ctx := app.finalizeBlockState.Context().WithValue(DoNotFailFastContextKey, struct{}{}) + ctx := app.finalizeBlockState.Context().WithValue(DoNotFailFastSendContextKey, struct{}{}) eb, err := app.endBlocker(ctx) if err != nil { return endblock, err diff --git a/x/gov/abci.go b/x/gov/abci.go index 56ab8641bd10..e21337f18292 100644 --- a/x/gov/abci.go +++ b/x/gov/abci.go @@ -158,7 +158,8 @@ func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) error { // Messages may mutate state thus we use a cached context. If one of // the handlers fails, no state mutation is written and the error // message is logged. - cacheCtx, writeCache := ctx.CacheContext() + execCtx := ctx.WithValue(baseapp.DoNotFailFastSendContextKey, nil) // enable fail fast during msg handling + cacheCtx, writeCache := execCtx.CacheContext() messages, err := proposal.GetMsgs() if err != nil { proposal.Status = v1.StatusFailed