search

cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.27k stars 3.63k forks source link

Remove blocked address list from bank keeper #12825

Open colin-axner opened 2 years ago

colin-axner commented 2 years ago

Thanks @alexanderbez for the suggestion!

Summary

Remove blocked addresses from the bank keeper.

Problem Definition

Blocked addresses were initially introduced in SimApp as a way to prevent the simulation from using module account addresses to send, receive, delegate, etc which would cause the strict invariants to break.

Of course, if it breaks in simulations, it'll break in production if the same action occurs. This was fixed by adding blocked addresses to the bank keeper.

Over time, blocking addresses at the bank keeper level has proved extremely problematic. Requiring the app.go file to declare the blocked addresses could easily result in an incorrect declaration and it was pretty obvious that the initial patch fix was generating compounding problems.

At the developer level, usage of the bank keeper becomes problematic as only certain send functions check if the receiver is blocked and I believe non check if the sender is blocked. Any sort of functionality which requires refunding will not want to allow sends from a blocked address, for example in ics29. The main functions SendCoins does no blocked address check, rightfully so, as blocked addresses may still send coins around, but this is confusing for developers relying on the bank API

What does the blocked address list solve?

It has always been intended to block module account addresses from receiving funds so as to not break invariants. Additionally it has begun to be used to block regular addresses as well.

If invariants are weakend to ensure that the module account balance >= expected balance, then module accounts could begin receiving tokens again and user addresses could be blocked from sending messages at the ante handler layer.

Invariants can be weakened if there is no valid reason why the module account balance must equal the expected balance. This would only be the case (from my understanding) if the module with the module account relied on the tracked (expected) balance instead of the actual balance. It seems to me, receiving extra tokens should not affect the functionality of a module, they should just be extra tokens that will likely become unused. Thus modules should always be relying on the actual token balance within the module account

From what I can tell, the pr that introduced module accounts #4255, did not provide a reason for why the invariants couldn't be weakened. Within the SDK, module account invariants are used for at least:

For staking, I see the invariant checks the bonded/notbonded tracked values against the bank keepers actual bonded/notbonded pool and that GetBondedPool and GetNotBondedPool is used only for this invariant and queries. Thus from what I can tell, it is okay to weaken the invariant.

A similar story is true for distribution which ensures the outstanding rewards + community pool are equal to the actual amount

The governance invariant has already been weakened

Proposal

Pre-requirement:

Proposed Changes:

If desirable, replace blocked address functionality with an ante handler which has a list of blocked addresses specified by the app.go config, which discards any messages signed by a blocked address. Note, module accounts will no longer be blocked in this instance.

Other references

alexanderbez commented 2 years ago

@marbar3778 would love to get this into 0.47

tac0turtle commented 2 years ago

is this a short term fix or a long term fix? if its a large problem now then we can get it in, but we need to evaluate the scope of 0.47 cause it may be larger than we would want with the possibility of IS and liquidate staking needing to be upstreamed.

alexanderbez commented 2 years ago

Not sure I understand what long vs short term means here? The notion of a deny list was added solely to address invariants and simulations. They're not needed. In the case an app really needs a true deny list, they can add it as an AnteHandler decorator.

aaronc commented 2 years ago

The AnteDecorator alternative feels kind of hacky. I propose that if there are addresses which shouldn't receive funds - either because that would break invariants or actual module logic - there should be a separate type of "bank account" which doesn't have an sdk.AccAddress. Maybe we could call it a module escrow. It would just be referenced by the module name in bank.Keeper and not addressable in any transaction. I think most existing module accounts fit this pattern, should be blocked and already use keeper methods which use module account names. We could just change the data for these "accounts" to be in a separate balance store in bank and get rid of the blocked accounts.

alexanderbez commented 2 years ago

The AnteDecorator alternative feels kind of hacky.

Why? I think it feels completely natural.

I propose that if there are addresses which shouldn't receive funds - either because that would break invariants or actual module logic - there should be a separate type of "bank account" which doesn't have an sdk.AccAddress

That would be an interesting approach that could work as well.

There are varying solutions here with varying degrees of complexities and UX 'niceness'.

aaronc commented 2 years ago

The AnteDecorator alternative feels kind of hacky.

Why? I think it feels completely natural.

Because it requires ante decorators to be aware of module logic. For me it has the same "shouldn't need to know that here" smell as putting it in app.go

colin-axner commented 2 years ago

Thanks for the feedback. It seems module accounts are being used in two ways currently.

In IBC, we are seeing demand for the second user-flow. I think it'd be nice to separate these types of "module accounts". I'm very open to whatever solution is landed on, but it would be great to consider how long each solution would take to get implemented. It might be less troublesome to explicitly create a new "module escrow" type rather than migrate the existing accounts to be escrows

Is there a way we could remove the blocked address list before creating the ability for a "module escrow" or is there strong preference to deliver these changes in the same major release?

With regards to the ante decorator, my idea was more if folks wanted to maintain the ability to block regular addresses (not module addresses), like I see osmosis doing, though one could argue the SDK doesn't need to maintain this ante decorator

alexanderbez commented 2 years ago

Because it requires ante decorators to be aware of module logic.

No it doesn't. It just need to be aware of the message types and the deny list. If you need fancy address filtering, then a simple deny list as it stands today wouldn't serve that need either.

alexanderbez commented 2 years ago

Is there a way we could remove the blocked address list before creating the ability for a "module escrow" or is there strong preference to deliver these changes in the same major release?

I would rather us get rid of the deny list altogether.