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

R4R: Runtime-assertable invariants #2807

Merged
merged 4 commits into from
Nov 15, 2018

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Nov 14, 2018

Closes #2663

This PR adds code to Gaia to assert an invariant set at the end of processing every block (right before returning abci.ResponseEndBlock). Currently, these invariants are the following:

  • Non-negative balance invariant, which asserts that no account has a negative balance (likely can remove this after the int => uint change @alexanderbez is working on)
  • Validator accum invariant, which asserts that the sum of validator accums is equal to the total accum. O(n) where n is the number of validator distribution infos, which I believe is capped at MaxValidators.
  • Supply invariant, which asserts that the total supply of tokens in accounts, unbonding delegations, validators, and fee pools is equal to the total supply tracked in the stake pool. O(a + d + v + i), where a is the number of accounts, d is the number of unbonding delegations, v is the number of validators (not just bonded, all candidates), and i is the number of validator distribution infos.
  • Positive power invariant, which asserts that all stored validators have positive power. O(v) where v is the number of validators.

These invariants will obviously become too expensive in the long run to run on each block, but I think this may be a nice bit of defense-in-depth in the short run. This is written so removing the invariant checks will required a hard fork (which might end up being the hard fork to enable transfers, or might be a later fork).

Up for debate:

  • Any near-term DoS concerns? Do we need to add high gas costs to particular transactions (creating an account, creating a validator) to mitigate them?
  • Should disabling the invariants require a hard-fork, or should it be a parameter which could be changed with a ParamChangeProposal?

To-do:

  • Make sure calling at EndBlock is correct
  • A bit of benchmarking
  • Should we exclude any invariants?

Standard checklist:

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

@codecov
Copy link

codecov bot commented Nov 14, 2018

Codecov Report

Merging #2807 into develop will increase coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2807      +/-   ##
===========================================
+ Coverage    56.73%   56.75%   +0.02%     
===========================================
  Files          131      131              
  Lines         8554     8554              
===========================================
+ Hits          4853     4855       +2     
+ Misses        3368     3366       -2     
  Partials       333      333

@cwgoes cwgoes changed the title WIP: Runtime-assertable invariants R4R: Runtime-assertable invariants Nov 14, 2018
Copy link
Member

@zmanian zmanian left a comment

Choose a reason for hiding this comment

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

Love the code re-use from the simulator.

Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

I was able to pull down this branch and run a testnet, as well as the simulator successfully.

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

LGTM. Changes are clean and minimal 👌

I would be nice if we could revert this somehow w/o a hard fork, although it's not directly clear to me why it would cause one? So my vote would be for a parameter proposal. In either case, this can be done post GoS/pre-launch.

@cwgoes
Copy link
Contributor Author

cwgoes commented Nov 15, 2018

It would be nice if we could revert this somehow w/o a hard fork, although it's not directly clear to me why it would cause one? So my vote would be for a parameter proposal. In either case, this can be done post GoS/pre-launch.

It doesn't require a hard fork in the sense that the state transition machine is incompatible (it would be compatible), but rather that if some nodes dropped the invariants and others did not, if any were ever violated the network would diverge, since the nodes continuing to check the invariants would panic and halt.

@alexanderbez
Copy link
Contributor

Mhmmm yes, good point. Although I'd hope none would be violated. So it sounds more like a soft fork.

Ultimately, validators could still run this if they wanted to, no?

@jackzampolin jackzampolin merged commit cf6b7ef into develop Nov 15, 2018
@cwgoes cwgoes deleted the cwgoes/runtime-assertable-invariants branch January 17, 2019 13:09
chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this pull request Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants