feat: deterministic map iteration

[bruce-wayne2]

Description

We should ensure that events emitted are in a deterministic order on any node.

Sorry, the previous PR-12693 was closed due to wrong operation.

Close: #12693

---

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)

[codecov]

Codecov Report

Merging #12781 (265d0c8) into main (4fe7797) will decrease coverage by 0.17%.
The diff coverage is 82.35%.

❗ Current head 265d0c8 differs from pull request most recent head 7747c80. Consider uploading reports for the commit 7747c80 to get more accurate results

@@            Coverage Diff             @@
##             main   #12781      +/-   ##
==========================================
- Coverage   55.87%   55.70%   -0.18%     
==========================================
  Files         646      646              
  Lines       54895    54859      -36     
==========================================
- Hits        30675    30561     -114     
- Misses      21762    21845      +83     
+ Partials     2458     2453       -5     

Impacted Files	Coverage Δ
x/distribution/keeper/params.go	75.00% <ø> (-3.58%)	⬇️
x/distribution/module.go	64.28% <ø> (ø)
types/dec_coin.go	98.41% <50.00%> (-0.79%)	⬇️
crypto/keyring/keyring.go	61.95% <100.00%> (ø)
types/events.go	84.65% <100.00%> (+0.35%)	⬆️
x/distribution/abci.go	100.00% <100.00%> (ø)
x/distribution/keeper/allocation.go	97.10% <100.00%> (+10.43%)	⬆️
x/genutil/gentx.go	0.00% <0.00%> (-85.49%)	⬇️
x/genutil/module.go	9.52% <0.00%> (-57.15%)	⬇️
x/genutil/genesis.go	0.00% <0.00%> (-40.00%)	⬇️
... and 5 more

[robert-zaremba]

lgtm. Someone from the core team should approve it or reject it to close the controversy.

[peterbourgon]

Again, events are by definition non-deterministic. Applications can not and must not rely on determinism in event delivery.

[sunnya97]

@peterbourgon Why are events by definition non-deterministic? Like why isn't it a design goal to make them deterministic? What's a situation in which we would want non-deterministic events?

[peterbourgon]

Oh! Sorry if I'm confusing things, here. When I say events are by definition non-deterministic, I don't mean that events have been defined to be non-deterministic, as if deterministic events were somehow an alternative. I mean is literally impossible, in a distsys theory sense, for a node to deliver deterministic events, assuming common definitions of "deterministic" and "events", and assuming a node should not allow its e.g. RPC or Websocket or whatever clients to impact consensus.

edit: When Merkelized state is maintained in a node, and clients access that state with (essentially) synchronous RPCs, then it's clear that the source of truth is the node, and that clients just have a view of that state, and the system is sound. But if a client assumes that Merkelized state is accurately modeled by a stream of events, they're wrong: that stream of events is fundamentally unreliable, in the CAP sense. The node, which is responsible for consensus, factually cannot make authoritative claims about the state "seen" by an event consumer, because there is factually no way to know that an event sent by a server has been observed by a client. I mean, that's not strictly true: you can do it, but the protocol you need for it is either peer-to-peer and session-oriented, e.g. TCP, where determinism requires either unbounded resources or allows clients to halt your server; or a consensus protocol, which is infeasibly expensive and would essentially replicate the state API.

[robert-zaremba]

There are many benefits for having deterministic events:

• offline balance tracking
• realyers (notable go IBC relayer v1 was based on event tracking)
• tooling

There are may discussions to put events into the App state and make the part of the consensus.

Like already discussed in the previous PR (#12693), I really don't see any reason to block this improvement.

[peterbourgon]

I agree that it would be enormously valuable. The problem is that it isn't possible, without sacrificing essential properties of the system.

edit: You're of course free to disregard my warnings and give it a shot! If you do I bet it'll pass tests and work most of the time. If and when there are problems, you know how to reach me 😉

[robert-zaremba]

@peterbourgon we know that events are not deterministic. We were are discussion various features around events, and, imho, every improvement is good - as long as it won't impact visibly the chain performance.

[peterbourgon]

@robert-zaremba

we know that events are not deterministic

Is that true?

If I recall correctly, this issue stemmed from a consensus failure in a downstream project — cosmwasm? — which relied on deterministic order of events and event attributes. Events aren't deterministic, so that assumption is invalid, and the app is broken. The correct fix would have been to fix this bug in the app. Instead, AFAIK, Cosmos was updated to satisfy these incorrect/unspecified assumptions.

Maybe I misunderstood — maybe they know their app is broken and they're working on a fix, but it'll take some time? Could be. Haven't seen anything indicating that so far, though. (Link me if you know otherwise!)

every improvement is good - as long as it won't impact visibly the chain performance

Is this change an improvement? I dunno, I actually see it as a regression: per above, it allows broken apps to continue being broken, I think. As an amusing counter-example, consider Go itself! Initially for k, v := range map was implemented so that order was deterministic, even though this was (deliberately) not guaranteed in the language spec. After a few releases they actually changed the implementation to explicitly randomize the order, because they observed a bunch of programs that were, incorrectly, relying on this unspecified property of the implementation.

[robert-zaremba]

There are over 2 years of discussions in that topic. Initially I wanted to Merkelize events in Tendermint block. But that won't happen.

[robert-zaremba]

BTW, many apps use events. You can track events using your own node.

[robert-zaremba]

Also I think your assumption is wrong. An app can use events. And we want to use them in reliable way. Hence the PR and all discussions.

[peterbourgon]

BTW, many apps use events. You can track events using your own node.

I use events! They're super handy! It's totally fine to use events, as long as you don't assume they will be reliably and deterministically delivered (over the network), and as long as you don't treat them as equivalent to the Merkelized block/state data returned by queries.

I'm sure there are plenty of apps that do use events with these invalid assumptions, and I'm sure that they run in production or whatever without errors for long periods of time. That doesn't mean they aren't broken.

[sunnya97]

@peterbourgon sorry I don't understand what you're talking about

Events are emitted based on state machine execution of finalized txs. Why would those not be deterministic, from the perspective of the chain?

Like yes, obviously we don't know that every node received a copy of the latest head of the chain... But whenever they do, they will still be emitting the same events, as long as the events are based solely on the content of the chain?

[peterbourgon]

@sunnya97

@peterbourgon sorry I don't understand what you're talking about

Events are emitted based on state machine execution of finalized txs. Why would those not be deterministic, from the perspective of the chain?

Like yes, obviously we don't know that every node received a copy of the latest head of the chain... But whenever they do, they will still be emitting the same events, as long as the events are based solely on the content of the chain?

I totally agree that the node can easily produce events that are deterministic relative to the consensus state of the chain. But I'm talking about determinism from the perspective of event consumers, i.e. out-of-process clients receiving events over a Websocket or whatever. If consumers are going to treat events as reliable and deterministic and equivalent to consensus state, then it's their perspective that matters, not the server's.

edit: I can write up an example demonstrating all of the issues at play, if this still isn't clear. Just let me know.

[robert-zaremba]

@sunnya97 , events are note validated by the consensus. So, if you keep your own tendermint then you know legit events.

And this discussion addresses the issue of reliably consume events. I don't understand why we are blocking it. I'm going to approve it, but since there is a controversy, someone from the core team has to approve it as well #12781 (review)

[tac0turtle]

@bruce-wayne2 should we backport this to 0.45.and 0.46

[bruce-wayne2]

@bruce-wayne2 should we backport this to 0.45.and 0.46

0.45 does not support go1.18, do I need to modify the implementation?

[alexanderbez]

I say we merge this and backport to 0.45 and 0.46 and we separately update 0.45 to update to Go 1.19.