EPIC: make guarantees around query responses

[tac0turtle]

Summary

As mentioned in the issue from cosmwasm there is a need to have data response guarantees around queries and messages. This issue focuses on the former, queries.

Problem Definition

Within minor releases responses of queries could change, potentially causing issues to downstream users.

Proposal

• Add proto annotations for deterministic queries feat: Add proto annotation for deterministic queries #13174
• For the following modules do:
  • Write test vectors and regression tests for queries to provide guarantees around queries
  • Manual audit of query code to make sure there is no potential source of non-determinism
  • test vectors and regression: x/auth queries #13191
  • write regression & test vectors for x/bank queries #13192
  • write regression & test vectors for x/staking queries  #13193
  • Remove module_query_safe for x/auth's bech32 grpc queries #13625
• Add documentation around deterministic queries #13464
• Add deterministic test for ModuleAccountByName query #13704

This could be done in coordination with auto generated queries

[aaronc]

I believe there should be some annotation in the .proto files to indicate which queries are deterministic and can be used inside the state machine.

There may always be some cases where people want to use queries in some non-deterministic or very inefficient way. Those queries shouldn't be exposed to the state machine and modules like cosmwasm.

Probably by default we should assume all queries are unsafe, and one by one start marking the safe ones (which have proper regression tests, gas consumption, etc.) with an annotation.

[amaury1093]

It's also probably safer to disable all historical queries, since they depend on each node's pruning settings.

[tac0turtle]

It's also probably safer to disable all historical queries, since they depend on each node's pruning settings.

could you elaborate

[amaury1093]

If your state machine code makes a historical query for height say N-1000, then:

• if the validator node prunes state older than 1000 blocks, than the query will error
• if the validtor node doesn't prune, then the query will succeed

causing a consensus error.

My safe solution above is to only allow state machine queries to query the latest state.

[alexanderbez]

I'm not sure I understand @AmauryM. AFAIK, CW would not make any historical queries, but rather assumes the latest/current state.

I like @aaronc's proposal of a Proto annotation. This way devs/clients can inspect docs and know what queries are safe to use.

So I say we:

1. Introduce and add annotations
2. Write tests for determinisms for said queries (i.e. loop N times and ensure output is canonical each time)

[amaury1093]

I creatd a first PR to make sure we agree on the proto annotation, and which queries will have the guarantees on determinism. #13174

[amaury1093]

Write tests for determinisms for said queries (i.e. loop N times and ensure output is canonical each time)

How about using property-based testing with rapid, which the SDK already uses? So basically something like:

// Check GetBalance determinism
rapid.Check(func(t *testing.T) {
  addr := AddressGenerator().Draw(t) // where AddressGenerator returns a rapid.Generator[sdk.AccAddress]
  amt := rapid.Uint64().Draw(t)
  bankKeeper.SetBalance(ctx, addr, sdk.NewCoin(amt, "stake"))
  for i := 0; i < 1000; i++ {
    // make sure bankKeeper.GetBalance always returns the same response
  }
})

So there are 1000*N iterations, where N = our rapid's configuration. This way we don't hardcode the response.

Do we also agree these tests belong inside test/integration ? I.e not suited to be tested using mocks.

[alexanderbez]

I've never seen or used Rapid, but the approach seems reasonable to me.

[tac0turtle]

with rapid how do you verify that the response didnt change in minor releases?

[amaury1093]

with rapid how do you verify that the response didnt change in minor releases?

Good point. So maybe we do one or three hardcoded responses (to check backwards-determinism) plus rapid (to check the property "query is deterministic"). I feel both are useful here since they test different things.

[amaury1093]

FYI, the way we currently test determinism/regression across minor releases is weak. We basically test queries against hardcoded values. E.g. if we fund acct1 with balance2, then the query always return the same balance, see example.

With the above method, we might catch changes in the query response. But we wouldn't catch stuff like:

• server-side logic change (e.g. additional server-side validation of incoming request on the query handler)
• gas change

These are not trivial to catch though. Are people fine, for this epic, to do the following:

1. keep the current hardcoded tests for regression
2. add clear documentation for module developers that they need to be careful about those state-machine breaking changes on each PR, when they add module_query_safe proto annotation, it can easily be a footgun.

Also happy to hear other ideas on how to test state-machine breaking changes in queries.

[alexanderbez]

@AmauryM I'm a bit lost as to how your points relate to non-determinism re queries:

server-side logic change (e.g. additional server-side validation of incoming request on the query handler)

How is validation breaking determinism? If some query previous didn't error on input but with a new change now does, is this what you mean? If so, tests should easily catch this (e.g. Error vs NoError assertions).

gas change

Why is this difficult? Can't we read the gas consumed off of the context?

[amaury1093]

How is validation breaking determinism? If some query previous didn't error on input but with a new change now does, is this what you mean?

Yes. Imagine you (or any module developer in the future) create a PR on a deterministic query which:

• adds a new field in the proto request struct
• does some validation with this new field

Then our existing deterministic tests won't fail. Even though this is a state-machine breaking change. We would need to write new tests.

gas change

So you're proposing to hardcode the gas consumed by each query inside the test itself? Not sure how flakey this would be with our test setup, but we can try.

[alexanderbez]

Then our existing deterministic tests won't fail. Even though this is a state-machine breaking change. We would need to write new tests.

Yeah, makes sense. I suppose there isn't much we can do -- we just have to ensure we right good tests for new fields.

So you're proposing to hardcode the gas consumed by each query inside the test itself? Not sure how flakey this would be with our test setup, but we can try.

Yes, exactly! With queries, the gas should not be flakey since we're not simulating any state! In fact, we can improve these tests by also ensuring the gas consumed never changes per iteration.

[amaury1093]

This Epic is complete 🎉.

As a summary, we added the (cosmos.query.v1.module_query_safe) = true protobuf annotation to Auth,bank, staking grpc queries that are allowed to be called from the state machine. Here's some documentation too.

[tac0turtle]

amazing, thank you!!! Nice job!!

Should we open a new epic to slowly migrate other modules as well?

[amaury1093]

Not sure about all modules, there are still caveats like #13041 (comment), so a consensus error probability is never 0.

Are there queries/modules that people particularly want? Let's do those first.

[tac0turtle]

@ValarDragon @ethanfrey @assafmo do you have queries you need to be deterministic from the core sdk modules

[ethanfrey]

Bank, Staking and Auth is a huge step forward 💪
Thank you for getting this in 🚀

I don't have any more pressing needs. I think gov would be interesting to some users, but I think it is also in a process of change and not in a place to produce such guarantees.

Maybe adding such deterministic query checks for x/distribution, which is often used with staking?

NB: We just merged support to let chains configure their own such whitelist. Will play together well with this work CosmWasm/wasmd#1069

[assafmo]

We went over all of these queries and found them to be deterministic: https://github.com/scrtlabs/SecretNetwork/blob/4397f93b2/x/compute/internal/keeper/query_plugins.go#L163-L199

Actually we didn't find a query in the standard modules that isn't deterministic, but we excluded the ones with iterations (E.g. /cosmos.auth.v1beta1.Query/Accounts) to prevent spamming nodes with huge queries.

[amaury1093]

We went over all of these queries and found them to be deterministic

Thanks for the list! @assafmo How did you check they were deterministic?

[assafmo]

By eye, I went over the code and made sure there's just storage access and no maps serializations