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.

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

Allow injectable restrictions on bank transfers #568

Merged
merged 21 commits into from
May 12, 2023

Conversation

SpicyLemon
Copy link

Description

This PR:

  • Provides a mechanism for injecting functions that can restrict bank transfers.
  • Limits MsgMultiSend and InputOutputCoins to a single input.
  • Removes the bank module's SetQuarantineKeeper and SetSanctionKeeper functions.
  • Updates the sanction and quarantine modules to inject a restriction function.
  • Moves the bank module's MintingRestrictionFn type from the keeper package to types.
  • Removes SendCoinsBypassQuarantine.
  • Adds context helpers to the sanction and quarantine modules to allow bypassing those checks as needed.
  • Removes the bank keeper's SetSendRestrictionsFunc method; use the new AppendSendRestriction or PrependSendRestriction instead.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

…e, sanction, and other send restriction func stuff.
…nd make it private so that it can't be confused with a SendRestrictionFn.
…f with the contexts to reduce reduncancy, e.g. it was quarantine.HasQuarantineBypass and is now quarantine.HasBypass. Also add some unit tests on them.
…idn't really change anything or make anything easier.
@SpicyLemon SpicyLemon marked this pull request as ready for review May 5, 2023 19:54
nullpointer0x00
nullpointer0x00 previously approved these changes May 8, 2023
@SpicyLemon
Copy link
Author

Holding off on merging this.

The related provenance PR is failing sims, e.g. https://github.com/provenance-io/provenance/actions/runs/4897195504/jobs/8745004575?pr=1506

--- FAIL: TestAppImportExport (25.36s)
  panic: invariant broken: quarantine: Funds-Holder-Balance invariant
  quarantine funds holder account cosmos1cttxw40x6z6z77j5rp7qyr6vsrmhjcs2z8vvs7 balance insufficient, have: 119242885123397423nhash,2[602](https://github.com/provenance-io/provenance/actions/runs/4897195504/jobs/8745004575?pr=1506#step:8:603)2159497373707stake, need: 119242891558877803nhash,26022159497373707stake, 119242885123397423nhash is less than 119242891558877803nhash: insufficient funds
  
  	CRITICAL please submit the following transaction:
  		 tx crisis invariant-broken quarantine Funds-Holder-Balance [recovered]
  	panic: invariant broken: quarantine: Funds-Holder-Balance invariant
  quarantine funds holder account cosmos1cttxw40x6z6z77j5rp7qyr6vsrmhjcs2z8vvs7 balance insufficient, have: 119242885123397423nhash,26022159497373707stake, need: 119242891558877803nhash,26022159497373707stake, 119242885123397423nhash is less than 119242891558877803nhash: insufficient funds
  
  	CRITICAL please submit the following transaction:
  		 tx crisis invariant-broken quarantine Funds-Holder-Balance [recovered]
  	panic: invariant broken: quarantine: Funds-Holder-Balance invariant
  quarantine funds holder account cosmos1cttxw40x6z6z77j5rp7qyr6vsrmhjcs2z8vvs7 balance insufficient, have: 119242885123397423nhash,26022159497373707stake, need: 119242891558877803nhash,26022159497373707stake, 119242885123397423nhash is less than 119242891558877803nhash: insufficient funds
  
  	CRITICAL please submit the following transaction:
  		 tx crisis invariant-broken quarantine Funds-Holder-Balance
  
  goroutine 31 [running]:
  testing.tRunner.func1.2({0x22cfb40, 0xc0016e60e0})
  	/opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1389 +0x24e
  testing.tRunner.func1()
  	/opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1392 +0x39f
  panic({0x22cfb40, 0xc0016e60e0})
  	/opt/hostedtoolcache/go/1.18.10/x64/src/runtime/panic.go:838 +0x207
  github.com/cosmos/cosmos-sdk/x/simulation.SimulateFromSeed.func2()
  	/home/runner/go/pkg/mod/github.com/provenance-io/[email protected]/x/simulation/simulate.go:146 +0xf1
  panic({0x22cfb40, 0xc0016e60e0})
  	/opt/hostedtoolcache/go/1.18.10/x64/src/runtime/panic.go:844 +0x258
  github.com/cosmos/cosmos-sdk/x/crisis/keeper.Keeper.AssertInvariants({{0xc000c4b900, 0xe, 0x10}, {{0x3305c60, 0xc000be5cb0}, 0xc000010b18, {0x32d69f8, 0xc000c62470}, {0x32d6a48, 0xc000c625b0}, ...}, ...}, ...)
  	/home/runner/go/pkg/mod/github.com/provenance-io/[email protected]/x/crisis/keeper/keeper.go:82 +0x645
  github.com/cosmos/cosmos-sdk/x/crisis.EndBlocker({{0x32f4878, 0xc000130000}, {0x3306750, 0xc001f87f00}, {{0x0, 0x0}, {0x265c33e, 0xe}, 0x18, {0x0, ...}, ...}, ...}, ...)
  	/home/runner/go/pkg/mod/github.com/provenance-io/[email protected]/x/crisis/abci.go:20 +0x1fe
  github.com/cosmos/cosmos-sdk/x/crisis.AppModule.EndBlock(...)
  	/home/runner/go/pkg/mod/github.com/provenance-io/[email protected]/x/crisis/module.go:161
  github.com/cosmos/cosmos-sdk/types/module.(*Manager).EndBlock(_, {{0x32f4878, 0xc000130000}, {0x3306750, 0xc001f87f00}, {{0x0, 0x0}, {0x265c33e, 0xe}, 0x18, ...}, ...}, ...)
  	/home/runner/go/pkg/mod/github.com/provenance-io/[email protected]/types/module/module.go:505 +0x546
  github.com/provenance-io/provenance/app.(*App).EndBlocker(_, {{0x32f4878, 0xc000130000}, {0x3306750, 0xc001f87f00}, {{0x0, 0x0}, {0x265c33e, 0xe}, 0x18, ...}, ...}, ...)
  	/home/runner/work/provenance/provenance/app/app.go:913 +0x79
  github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).EndBlock(0xc0000010e0, {0xc000130000?})
  	/home/runner/go/pkg/mod/github.com/provenance-io/[email protected]/baseapp/abci.go:212 +0x25d
  github.com/cosmos/cosmos-sdk/x/simulation.SimulateFromSeed({0x330a890?, 0xc0001f3860?}, {0x32d2[620](https://github.com/provenance-io/provenance/actions/runs/4897195504/jobs/8745004575?pr=1506#step:8:621)?, _}, _, _, _, {0xc000f06000, 0x48, 0x60}, ...)
  	/home/runner/go/pkg/mod/github.com/provenance-io/[email protected]/x/simulation/simulate.go:188 +0x191a
  github.com/provenance-io/provenance/app.TestAppImportExport(0xc0001f3860)
  	/home/runner/work/provenance/provenance/app/sim_test.go:191 +0x60f
  testing.tRunner(0xc0001f3860, 0x2de72a0)
  	/opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1439 +0x102
  created by testing.(*T).Run
  	/opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1486 +0x35f

@SpicyLemon
Copy link
Author

Found the issue, just haven't fixed it yet.
The sim is failing at the end of block 23 when it runs invariants.
I fired it up locally in my IDE so I could more easily play with some of the parameters. I found that, if I cranked the period value down to 1, so that the invariant checks happen every block, it still failed on block 23. I also added a boatload of logging and output. I couldn't find any msgs that might involve the amount missing from the quarantine funds holder account: 6435480380nhash.

However, I did find that, during the block 23 invariant check, cosmos1j8q6n66zteutllcufe08apmv04n2crlnvkdqpy had a QuarantineRecord of 6435480380nhash from cosmos1w6t0l7z0yerj49ehnqwqaayxqpe3u7e23edgma. I then found that, during the BeginBlock of block 23, this error is getting logged:

I[2023-05-08|19:04:22.003] BeginBlocker - Expiring reward program id:35654 title:"title" description:"description" distribute_from_address:"cosmos1j8q6n66zteutllcufe08apmv04n2crlnvkdqpy" total_reward_pool:<denom:"nhash" amount:"3217740190" > remaining_pool_balance:<denom:"nhash" amount:"3217740190" > claimed_amount:<denom:"nhash" amount:"0" > max_reward_by_address:<denom:"nhash" amount:"665" > minimum_rollover_amount:<denom:"nhash" amount:"100000000000" > claim_period_seconds:58715 program_start_time:<seconds:7935779021 > expected_program_end_time:<seconds:7935837736 > program_end_time_max:<seconds:7935837736 > claim_period_end_time:<seconds:7935837736 > actual_program_end_time:<seconds:7935843546 > claim_periods:1 current_claim_period:1 state:STATE_FINISHED expiration_offset:86715 qualifying_actions:<vote:<minimum_actions:48 maximum_actions:6155803 minimum_delegation_amount:<denom:"nhash" amount:"48" > > >
E[2023-05-08|19:04:22.003] Failed to refund remaining balance. spendable balance 0nhash is smaller than 3217740190nhash: insufficient funds [cosmos/cosmos-sdk/x/bank/keeper/send.go:268]
E[2023-05-08|19:04:22.003] cannot expire reward program because of error spendable balance 0nhash is smaller than 3217740190nhash: insufficient funds [cosmos/cosmos-sdk/x/bank/keeper/send.go:268]

I'm fairly certain that's the culprit. Here's what I think is happening:

  1. Reward program begin block tries to do a send.
  2. The send restrictions are run (including quarantine) and the amount being sent is quarantined.
  3. The rest of the send logic tries and fails to happen.
  4. The error is logged and the reward program is left in place.

The problem is that, most sends happen inside a transaction. If an error is encountered, the state is rolled back. But in that BeginBlock, it's not in a transaction, and the state isn't being rolled back. So a quarantine record is being created for an amount that didn't actually end up being transferred.

I'm pretty sure the best solution is to fix the reward program begin blocker to properly roll back what it might have partially done. I'm going to look into doing that tomorrow. I'll probably do a quick audit of other BeginBlockers to see if they might be in the same boat here.

@nullpointer0x00 nullpointer0x00 dismissed their stale review May 9, 2023 14:06

There was an issue found. So, holding off on review until fixed.

@SpicyLemon
Copy link
Author

SpicyLemon commented May 11, 2023

Found the problem, and it was specific to the rewards module (in the provenance repo). The fix is to use a cache context for its begin/end block stuff so things aren't partially done.

There wasn't anything that needed to be addressed in here.

Copy link
Member

@iramiller iramiller left a comment

Choose a reason for hiding this comment

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

We might want to come up with a method for overriding the spendable coins count when the user is sanctioned. This would be a separate effort though.

@SpicyLemon SpicyLemon merged commit 3c79779 into main-pio May 12, 2023
SpicyLemon added a commit that referenced this pull request May 12, 2023
* Add SendRestriction stuff to the bank keeper and remove the quarantine, sanction, and other send restriction func stuff.

* Move the restrictions into types so other modules can use them for expected keepers.

* Update the sanction module to provide a send restriction.

* Add a way to bypass the sanction restriction with a special context.

* Fix some unit tests.

* Update the quarantine module to use a SendRestriction.

* Move the SendRestriction wrapper struct thingy back into the keeper and make it private so that it can't be confused with a SendRestrictionFn.

* Limit the MsgMultiSend simulation operation to a single input.

* Fix the sanction send restriction to use the from address instead of to address (as it should have been).

* Fix all the unit tests that broke from these changes.

* Remove the quarantine and sanction expected keepers and mocks from the bank module.

* Remove the module names from the names of the funcs doing bypass stuff with the contexts to reduce reduncancy, e.g. it was quarantine.HasQuarantineBypass and is now quarantine.HasBypass. Also add some unit tests on them.

* correct the import alias in the send restriction function files.

* Add some tests on the quarantine SendRestrictionFn.

* Add unit tests on the sanction SendRestrictionFn.

* Update changelog.

* Update a comment to be a bit more specific.

* Add unit test on WithMintCoinsRestriction to make sure it doesn't update the existing keeper.

* Add unit tests on the send restriction functions via InputOutputCoins.

* Undo changes to x/sanction/simulation/operations_test.go since they didn't really change anything or make anything easier.

* When there isn't enough spendable balance, always include a coin amount in the error message.
iramiller added a commit that referenced this pull request May 12, 2023
…tions

Backport #568 to v0.46.x and mark v0.46.10-pio-4 in the changelog.
@SpicyLemon SpicyLemon deleted the dwedul/send-restrictions branch June 8, 2023 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants