add sorting into AccumulateChanges and add a unit test for it #584
Conversation
jtremback
requested review from
mpoke,
sainoe,
danwt and
shaspitz
as code owners
| @@ -30,6 +31,16 @@ func AccumulateChanges(currentChanges, newChanges []abci.ValidatorUpdate) []abci | |||
| for _, update := range m { | |||
| out = append(out, update) | |||
| } | |||
|
|
|||
| // The list of tendermint updates should hash the same across all consensus nodes | |||
There was a problem hiding this comment.
Note that this is currently not a requirement of ABCI, but I do agree that it's safer to have determinism. :)
| }, | ||
| }, | ||
| { | ||
| name: "two changes", |
There was a problem hiding this comment.
Is this use case even possible?
There was a problem hiding this comment.
Yea, if it is accumulating updates from several blocks
There was a problem hiding this comment.
They will be overwritten here, but I don't see a problem with extra testing.
| @@ -30,6 +31,16 @@ func AccumulateChanges(currentChanges, newChanges []abci.ValidatorUpdate) []abci | |||
| for _, update := range m { | |||
| out = append(out, update) | |||
| } | |||
|
|
|||
| // The list of tendermint updates should hash the same across all consensus nodes | |||
There was a problem hiding this comment.
Note that this is currently not a requirement of ABCI, but I do agree that it's safer to have determinism. :)
|
I would add a comment saying that this is not strictly needed because the updates are never committed to the store at the end of the block, and Tendermint doesnt' care about the order. |
The only reason this doesn't persist past the end of the block is because we happen to be sending updates each block. In theory, the fact that we send updates each block was supposed to be temporary. So to work as intended the sort is needed. |
By sending you mean sending from the consumer app to the consumer TM instance, right? |
Description
This adds sorting to the AccumulateChanges function to stop relying on Go map iteration order.
Linked issues
Closes: #505
Type of change
Please delete options that are not relevant.
How was the feature tested?
Issues and further questions
Is not tested under actual map iteration nondeterminism because this is hard to trigger intentionally. Still, the code is simple enough that this probably is ok.
Other information
Checklist:
Please delete options that are not relevant.
#<issue>#<issue>make test)make proto-genwas run (for changes in.protofiles)make proto-lintwas run (for changes in.protofiles)IterateX(ctx sdk.Context, cb func(arg1, arg2) (stop bool)))