Add modules for deterministic total ordering derivation from partial ord #1335

ValarDragon · 2022-04-23T18:40:46Z

Component of #1329

What is the purpose of the change

The intention of this logic is to allow us to specify partial orderings when orderings are needed (e.g. beginblock, endblock, upgrade orders),and then have an automatic derivation of the relevant total ordering.

The problem faced right now is the important relations are obscured in app.go at the moment, and its a lot of tedious logic to verify, for an area that can have subtle errors slip through. The aim of this change is to remedy this.

Brief change log

Does not alter the state machine, I suggest we do that in a second PR where we can verify the important parts of the ordering carefully
Adds a DAG data structure, with deterministic topological sort. Uses the DAG + topological sort to derive a total ordering. https://en.wikipedia.org/wiki/Topological_sorting#Relation_to_partial_orders

Testing and Verifying

This change added tests and can be verified as follows:

dag_test.go provides unit tests for AddFirst and TopologicalSort
partialord_test.go demonstrates every exposed API, what it will look for the intended use case in app module ordering, and that the sorted result obeys intended properties.
If you want to verify the logic, the dag uses adjacency lists to store edges, and sorts using Kahn's algorithm (The algo is straightforward). The Dag is not intended to be externally used, its solely for the partial ord package, hence the internal package syntax employed.

I mainly wrote this for fun, its not really a priority if people don't have bandwidth to review. If you have a couple minutes, would appreciate feedback on if others agree partialord_test.go is a better API than what exists right now (

osmosis/app/modules.go

Lines 225 to 254 in ed10116

    
           var orderEndBlockers = []string{ 
        
           	lockuptypes.ModuleName, 
        
           	crisistypes.ModuleName, 
        
           	govtypes.ModuleName, 
        
           	stakingtypes.ModuleName, 
        
           	capabilitytypes.ModuleName, 
        
           	authtypes.ModuleName, 
        
           	banktypes.ModuleName, 
        
           	distrtypes.ModuleName, 
        
           	slashingtypes.ModuleName, 
        
           	minttypes.ModuleName, 
        
           	genutiltypes.ModuleName, 
        
           	evidencetypes.ModuleName, 
        
           	authz.ModuleName, 
        
           	paramstypes.ModuleName, 
        
           	upgradetypes.ModuleName, 
        
           	vestingtypes.ModuleName, 
        
           	ibchost.ModuleName, 
        
           	ibctransfertypes.ModuleName, 
        
           	gammtypes.ModuleName, 
        
           	incentivestypes.ModuleName, 
        
           	lockuptypes.ModuleName, 
        
           	poolincentivestypes.ModuleName, 
        
           	superfluidtypes.ModuleName, 
        
           	bech32ibctypes.ModuleName, 
        
           	txfeestypes.ModuleName, 
        
           	// Note: epochs' endblock should be "real" end of epochs, we keep epochs endblock at the end 
        
           	epochstypes.ModuleName, 
        
           	wasm.ModuleName, 
        
           }

) and if we should switch to it, pending later logic agreements.

This PR intentionally does not change app.go, and just shows the API via a test, so that we can do a moreful check of all orderings independently of logic / agreeing on the approach.

Documentation

Does this introduce a new feature or user-facing behavior changes? no
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? yes
How is the feature or change documented? Both modules are intended to be documented by their module.go's and function godocs.

The intention of this logic is to allow us to specify partial orderings when orderings are needed (e.g. beginblock, endblock, upgrade orders), and then have an automatic derivation of the relevant total ordering. partialord_test.go gives a sense of how the API would look if we went with this.

ValarDragon · 2022-04-23T18:42:16Z

osmoutils/partialord/partialord_test.go

+	beginBlockOrd := partialord.NewPartialOrdering(moduleNames)
+	beginBlockOrd.FirstElements("upgrades", "epochs", "capabilities")
+	beginBlockOrd.After("ibctransfers", "ibc")
+	beginBlockOrd.After("bech32ibc", "ibctransfers")
+	beginBlockOrd.Before("mint", "distribution")
+	// This is purely just to test last functionality, doesn't make sense in context
+	beginBlockOrd.LastElements("auth", "authz", "wasm")


This should be what the app.go ordering logic ends up looking like.

I think in practice, we will just use Before syntax, and not mix After and Before

Should we then avoid exposing the unused API?

I don't think so, what makes sense depends on the context of a given application

mixing before and after would be quite weird for the reader though

If my understanding of the inverse relationship between Before and After is right (i.e. beginBlockOrd.After("ibctransfers", "ibc") is the same as beginBlockOrd.Before("ibc", "ibctransfers"))

I'm still not quite sure, why do we need to expose both? In what context do we need both if we can substitute one for the other by switching the order of arguments?

Similar reason to why int libraries expose > and <=

codecov-commenter · 2022-04-23T18:45:52Z

Codecov Report

Merging #1335 (dea3eb5) into main (a33c3c2) will increase coverage by 0.29%.
The diff coverage is 73.20%.

@@            Coverage Diff             @@
##             main    #1335      +/-   ##
==========================================
+ Coverage   19.53%   19.82%   +0.29%     
==========================================
  Files         200      202       +2     
  Lines       27532    27685     +153     
==========================================
+ Hits         5377     5489     +112     
- Misses      21153    21175      +22     
- Partials     1002     1021      +19

Impacted Files	Coverage Δ
osmoutils/partialord/internal/dag/dag.go	`72.86% <72.86%> (ø)`
osmoutils/partialord/partialord.go	`75.00% <75.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44099c5...dea3eb5. Read the comment docs.

ValarDragon · 2022-04-23T18:53:46Z

I did a brief survey of other dag implementations before implementing this one, and I couldn't find any that followed either of:

Being maintained & publicly exposed
Being deterministic

Folks like hashicorp maintain an unexposed dag internally for dependency tracking things, but they don't have the same determinism requirements we have in the SDK. They put it in an internal package, so we couldn't use it even if we wanted. Seemed excessive to copy that rather than just writing a short file here imo, especially given how simple it is for this use case, and the ease of custom helpers like AddFirstElements and AddLastElements.

p0mvn

I think this is an awesome design, was really interesting to review this. Great work! I really like how much more readable and less prone to user input errors the new API will make it in app.go

Most of my comments are readability-related nits. However, I think that we should add more tests:

Test for AddLast in dag_test.to
Tests in partialord_test.go related to the sequence of calls of the API
- Aside from helping to spot bugs, if any, these tests should be helpful to understand the usage of the API

osmoutils/partialord/internal/dag/dag.go

p0mvn · 2022-04-24T03:47:28Z

osmoutils/partialord/partialord_test.go

+	beginBlockOrd := partialord.NewPartialOrdering(moduleNames)
+	beginBlockOrd.FirstElements("upgrades", "epochs", "capabilities")
+	beginBlockOrd.After("ibctransfers", "ibc")
+	beginBlockOrd.After("bech32ibc", "ibctransfers")
+	beginBlockOrd.Before("mint", "distribution")
+	// This is purely just to test last functionality, doesn't make sense in context
+	beginBlockOrd.LastElements("auth", "authz", "wasm")


Should we then avoid exposing the unused API?

osmoutils/partialord/partialord_test.go

osmoutils/partialord/internal/dag/dag_test.go

osmoutils/partialord/internal/dag/dag.go

ValarDragon · 2022-04-24T04:33:54Z

Thanks for the detailed review!

Co-authored-by: Roman <[email protected]>

mattverse

LGTM! This is an awesome implementation, and I love how we could existing problems of manually listing orders of Enblockers using dag and total ordering!

General question I have would be what were the possible places in this implementation that could have caused non determinism and how do we ensure "deterministic ordering"?

mattverse · 2022-04-24T10:18:24Z

osmoutils/partialord/internal/dag/dag.go

+	directedEdgeList := make([]map[int]int8, len(dag.nodeNameToId))
+	for i := 0; i < len(dag.nodeNameToId); i++ {
+		originalEdgeList := dag.directedEdgeList[i]
+		directedEdgeList[i] = make(map[int]int8, len(originalEdgeList))
+		for k, v := range originalEdgeList {
+			directedEdgeList[i][k] = v
+		}
+	}
+	// we re-use nodeNameToId and idToNodeNames as these are fixed at dag creation.


What's the reason we iterate to copy Dag instead of directly using dag.directedEdgeList? Is that to ensure we deep copy edge list?

Yeah, the topological sort algorithm used basically removes edges from every node marked as 'done', So we have to remove these edges from a copy.

More space efficient things can technically be done, but should never be important at the relevant sizes here.

osmoutils/partialord/internal/dag/dag.go

Co-authored-by: Matt, Park <[email protected]>

…s into dev/partial_ordering

p0mvn

LGTM

ValarDragon · 2022-04-25T16:54:25Z

Thanks for the really thorough reviews here 🙏

ValarDragon commented Apr 23, 2022

View reviewed changes

ValarDragon added 2 commits April 23, 2022 13:55

Update changelog

1e4a82f

Add safety sorting of input provided to partialord

b53df15

p0mvn requested changes Apr 24, 2022

View reviewed changes

ValarDragon and others added 5 commits April 23, 2022 23:37

Apply suggestions from code review

29770dc

Co-authored-by: Roman <[email protected]>

Address many lints

5676ee7

Add godocs and update error messages

a2b412e

Add requested tests

88e04b9

Fix linter warning

24b8fd3

mattverse approved these changes Apr 24, 2022

View reviewed changes

ValarDragon and others added 3 commits April 24, 2022 10:56

Update osmoutils/partialord/internal/dag/dag.go

d45e7f0

Co-authored-by: Matt, Park <[email protected]>

Dag -> DAG

c9947b4

Merge branch 'dev/partial_ordering' of github.com:osmosis-labs/osmosi…

dea3eb5

…s into dev/partial_ordering

p0mvn approved these changes Apr 25, 2022

View reviewed changes

ValarDragon merged commit a572cb3 into main Apr 25, 2022

ValarDragon deleted the dev/partial_ordering branch April 25, 2022 16:54

ValarDragon mentioned this pull request May 4, 2022

Make a partial ordering struct #1329

Closed

github-actions bot mentioned this pull request Mar 15, 2024

Broken Links Detected #7752

Closed

github-actions bot mentioned this pull request May 1, 2024

Broken Links Detected #8183

Closed

github-actions bot mentioned this pull request Oct 15, 2024

Broken Links Detected notional-labs/osmosis#75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add modules for deterministic total ordering derivation from partial ord #1335

Add modules for deterministic total ordering derivation from partial ord #1335

ValarDragon commented Apr 23, 2022 •

edited

Loading

ValarDragon Apr 23, 2022 •

edited

Loading

p0mvn Apr 24, 2022

ValarDragon Apr 24, 2022 •

edited

Loading

p0mvn Apr 24, 2022

ValarDragon Apr 25, 2022

codecov-commenter commented Apr 23, 2022 •

edited

Loading

ValarDragon commented Apr 23, 2022 •

edited

Loading

p0mvn left a comment

p0mvn Apr 24, 2022

ValarDragon commented Apr 24, 2022

mattverse left a comment

mattverse Apr 24, 2022

ValarDragon Apr 24, 2022

p0mvn left a comment

ValarDragon commented Apr 25, 2022

	var orderEndBlockers = []string{
	lockuptypes.ModuleName,
	crisistypes.ModuleName,
	govtypes.ModuleName,
	stakingtypes.ModuleName,
	capabilitytypes.ModuleName,
	authtypes.ModuleName,
	banktypes.ModuleName,
	distrtypes.ModuleName,
	slashingtypes.ModuleName,
	minttypes.ModuleName,
	genutiltypes.ModuleName,
	evidencetypes.ModuleName,
	authz.ModuleName,
	paramstypes.ModuleName,
	upgradetypes.ModuleName,
	vestingtypes.ModuleName,
	ibchost.ModuleName,
	ibctransfertypes.ModuleName,
	gammtypes.ModuleName,
	incentivestypes.ModuleName,
	lockuptypes.ModuleName,
	poolincentivestypes.ModuleName,
	superfluidtypes.ModuleName,
	bech32ibctypes.ModuleName,
	txfeestypes.ModuleName,
	// Note: epochs' endblock should be "real" end of epochs, we keep epochs endblock at the end
	epochstypes.ModuleName,
	wasm.ModuleName,
	}

Add modules for deterministic total ordering derivation from partial ord #1335

Add modules for deterministic total ordering derivation from partial ord #1335

Conversation

ValarDragon commented Apr 23, 2022 • edited Loading

What is the purpose of the change

Brief change log

Testing and Verifying

Documentation

ValarDragon Apr 23, 2022 • edited Loading

Choose a reason for hiding this comment

p0mvn Apr 24, 2022

Choose a reason for hiding this comment

ValarDragon Apr 24, 2022 • edited Loading

Choose a reason for hiding this comment

p0mvn Apr 24, 2022

Choose a reason for hiding this comment

ValarDragon Apr 25, 2022

Choose a reason for hiding this comment

codecov-commenter commented Apr 23, 2022 • edited Loading

Codecov Report

ValarDragon commented Apr 23, 2022 • edited Loading

p0mvn left a comment

Choose a reason for hiding this comment

p0mvn Apr 24, 2022

Choose a reason for hiding this comment

ValarDragon commented Apr 24, 2022

mattverse left a comment

Choose a reason for hiding this comment

mattverse Apr 24, 2022

Choose a reason for hiding this comment

ValarDragon Apr 24, 2022

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

ValarDragon commented Apr 25, 2022

ValarDragon commented Apr 23, 2022 •

edited

Loading

ValarDragon Apr 23, 2022 •

edited

Loading

ValarDragon Apr 24, 2022 •

edited

Loading

codecov-commenter commented Apr 23, 2022 •

edited

Loading

ValarDragon commented Apr 23, 2022 •

edited

Loading