feat: Add proto annotation for deterministic queries #13174

amaury1093 · 2022-09-07T13:06:25Z

Description

Add option (cosmos.query.v1.deterministic) = true; to auth, bank and staking queries.

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...

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)

…41-deterministic-proto

amaury1093 · 2022-09-07T13:09:47Z

I'll generate the code once I get a concept/API ack for namings.

amaury1093 · 2022-09-07T13:11:00Z

proto/cosmos/query/v1/query.proto

+  //
+  // When set to true, the query can safely be called from within the state
+  // machine, for example via ADR-033 calls or from CosmWasm contracts.
+  bool deterministic = 11110001;


fyi in #12972, @aaronc you proposed cosmos.query.v1.internal.

Yeah not quite sure what the right word is

We do need to communicate with the doc string (and possibly the name) that these queries need to do proper gas management. Determinism is necessary but not sufficient for ADR-033

That's a great point!

aaronc

Not sure if we should use deterministic, internal or some other name, but this seems like the right idea

codecov · 2022-09-07T15:23:08Z

Codecov Report

Merging #13174 (831b94b) into main (6ec25b6) will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #13174      +/-   ##
==========================================
- Coverage   53.73%   53.63%   -0.10%     
==========================================
  Files         652      647       -5     
  Lines       55349    55194     -155     
==========================================
- Hits        29740    29605     -135     
+ Misses      23227    23218       -9     
+ Partials     2382     2371      -11

Impacted Files	Coverage Δ
x/distribution/simulation/operations.go	`80.64% <0.00%> (-9.68%)`	⬇️
tx/textual/valuerenderer/dec.go
tx/textual/valuerenderer/timestamp.go
tx/textual/valuerenderer/valuerenderer.go
tx/textual/valuerenderer/bytes.go
tx/textual/valuerenderer/int.go
x/staking/simulation/operations.go	`75.91% <0.00%> (+1.37%)`	⬆️

alexanderbez

I didn't verify all the queries to make sure they are deterministic, so I trust you. But the annotation LGTM!

amaury1093 · 2022-09-07T15:47:10Z

I didn't verify all the queries to make sure they are deterministic

Yes, me neither TBH. But these are the ones @atheeshp and I will audit through manually, and for each of them, add relevant tests.

aaronc · 2022-09-12T13:33:48Z

proto/cosmos/bank/v1beta1/query.proto

    option (google.api.http).get = "/cosmos/bank/v1beta1/balances/{address}/by_denom";
  }

  // AllBalances queries the balance of all coins for a single account.
  rpc AllBalances(QueryAllBalancesRequest) returns (QueryAllBalancesResponse) {
+    option (cosmos.query.v1.deterministic) = true;


I would argue this is the type of query that we should be very cautious about exposing to the state machine. Without pagination it very gas intensive.

This is true. But high gas costs, which smart contract devs should be aware of, is technically orthogonal to determinism. How do we express this to developers?

Not sure.. the main point to communicate with the annotation should be "appropriate for queries from modules" which is both determinism + manageable gas.

I'm assuming this particular query does have proper gas tracking via the store so it would likely just run out of gas. But I do also question the usefulness of these expensive queries. I guess this query is less expensive than querying all holders of a denom (which is what I thought it was), but even there, there is a scenario where a module might want to airdrop to all holders of a coin so I guess that is a valid use case.

I suppose then cosmos.query.v1.deterministic is a misnomer

@AmauryM any thoughts?

ACK. Just leave it as-is and we can add warning comments saying gas could be high.

Thinking about this a bit more, I actually think we should rename.

The three of us have the understanding regarding gas consumption, but we can't assume that about future users who might not read the docstring carefully. I think it is safer to use something like module_query_safe which prompts users to think more carefully about what "safe" means and if they don't know they're likely to check the doc string. Most people think they know what determinism means and then mark a query as such without thinking about other potential issues.

I'm actually thinking of some concrete examples right now where I want to add queries around protobuf file descriptors which are probably deterministic but wouldn't track gas and I would want to mark them explicitly as module_query_safe = false.

which prompts users to think more carefully about what "safe" means and if they don't know they're likely to check the doc string

Or maybe the opposite might happen: module developer sees module_query_safe = true on a high-gas query, and assumes it's safe to use, and ends up with unexpected high gas consumption.

If I want the user to check doc strings, I would actully put unsafe_, or expert_. unsafe_ wouldn't work here though, it has a different meaning.

I'm not opposed to Bez's idea of having 2 annotations.

I just want to note again, that "high gas consumption" is relative to the chain. What one chain might consider high, another chain might deem as normal.

Makes sense. Clearly the bigger issue is computation without gas consumption

aaronc · 2022-09-12T13:33:59Z

proto/cosmos/bank/v1beta1/query.proto

    option (google.api.http).get = "/cosmos/bank/v1beta1/spendable_balances/{address}";
  }

  // TotalSupply queries the total supply of all coins.
  rpc TotalSupply(QueryTotalSupplyRequest) returns (QueryTotalSupplyResponse) {
+    option (cosmos.query.v1.deterministic) = true;


amaury1093 added 4 commits September 7, 2022 14:49

feat: Add proto annotation for deterministic queries

0968bc5

Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/130…

bb88195

…41-deterministic-proto

Add comment

715f2cb

Tweak comment

26d2bc8

amaury1093 requested a review from a team as a code owner September 7, 2022 13:06

amaury1093 mentioned this pull request Sep 7, 2022

EPIC: make guarantees around query responses #13041

Closed

8 tasks

amaury1093 commented Sep 7, 2022

View reviewed changes

aaronc approved these changes Sep 7, 2022

View reviewed changes

kocubinski self-assigned this Sep 7, 2022

kocubinski approved these changes Sep 7, 2022

View reviewed changes

protocgen

6632a3a

github-actions bot added C:x/auth C:x/bank C:x/staking labels Sep 7, 2022

alexanderbez approved these changes Sep 7, 2022

View reviewed changes

aaronc mentioned this pull request Sep 7, 2022

feat: add proto annotation for Msg services #13178

Merged

19 tasks

amaury1093 added 2 commits September 8, 2022 11:12

Add gas

2e95c03

Merge branch 'main' into am/13041-deterministic-proto

831b94b

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Sep 8, 2022

amaury1093 enabled auto-merge (squash) September 8, 2022 09:16

amaury1093 merged commit 279f3f1 into main Sep 8, 2022

amaury1093 deleted the am/13041-deterministic-proto branch September 8, 2022 13:33

aaronc reviewed Sep 12, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add proto annotation for deterministic queries #13174

feat: Add proto annotation for deterministic queries #13174

amaury1093 commented Sep 7, 2022

amaury1093 commented Sep 7, 2022

amaury1093 Sep 7, 2022

aaronc Sep 7, 2022

aaronc Sep 7, 2022

alexanderbez Sep 7, 2022

amaury1093 Sep 8, 2022

aaronc left a comment

codecov bot commented Sep 7, 2022 •

edited

Loading

alexanderbez left a comment

amaury1093 commented Sep 7, 2022

aaronc Sep 12, 2022

alexanderbez Sep 12, 2022

aaronc Sep 12, 2022 •

edited

Loading

alexanderbez Sep 12, 2022

aaronc Sep 12, 2022

alexanderbez Sep 13, 2022

aaronc Sep 14, 2022 •

edited

Loading

amaury1093 Sep 14, 2022 •

edited

Loading

alexanderbez Sep 15, 2022

aaronc Sep 15, 2022

aaronc Sep 12, 2022

feat: Add proto annotation for deterministic queries #13174

feat: Add proto annotation for deterministic queries #13174

Conversation

amaury1093 commented Sep 7, 2022

Description

Author Checklist

Reviewers Checklist

amaury1093 commented Sep 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 7, 2022 • edited Loading

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

amaury1093 commented Sep 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

amaury1093 Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 7, 2022 •

edited

Loading

aaronc Sep 12, 2022 •

edited

Loading

aaronc Sep 14, 2022 •

edited

Loading

amaury1093 Sep 14, 2022 •

edited

Loading