ci: add workflow for reporting possible state-changing diffs #13774

elias-orijtech · 2022-11-05T00:02:47Z

This PR implements a tool for detecting PRs that may touch state code. Example PR and run: https://github.com/elias-orijtech/cosmos-sdk/actions/runs/3439195320/jobs/5736196835

Fixes #13518

CC @odeke-em

cmd/statediff/main.go

+		for _, pkg := range pkg.Imports {
+			addPkg(pkg)
+		}


cmd/statediff/main.go

+
+func main() {
+	flag.Parse()
+	*dir, _ = filepath.Abs(*dir)


codecov · 2022-11-05T00:14:28Z

Codecov Report

Merging #13774 (5e179fb) into main (2739f83) will increase coverage by 0.40%.
The diff coverage is n/a.

❗ Current head 5e179fb differs from pull request most recent head dd59995. Consider uploading reports for the commit dd59995 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13774      +/-   ##
==========================================
+ Coverage   56.25%   56.66%   +0.40%     
==========================================
  Files         667      636      -31     
  Lines       56576    54552    -2024     
==========================================
- Hits        31829    30910     -919     
+ Misses      22165    21136    -1029     
+ Partials     2582     2506      -76

Impacted Files	Coverage Δ
x/auth/keeper/genesis.go	`0.00% <0.00%> (-50.00%)`	⬇️
x/auth/keeper/account.go	`53.33% <0.00%> (-18.34%)`	⬇️
store/cache/cache.go	`79.62% <0.00%> (-16.67%)`	⬇️
store/mem/store.go	`82.35% <0.00%> (-11.77%)`	⬇️
store/types/store.go	`67.44% <0.00%> (-11.63%)`	⬇️
store/internal/proofs/create.go	`80.95% <0.00%> (-9.53%)`	⬇️
x/group/internal/math/dec.go	`41.53% <0.00%> (-7.00%)`	⬇️
x/gov/abci.go	`90.29% <0.00%> (-6.86%)`	⬇️
server/config/config.go	`69.87% <0.00%> (-2.65%)`	⬇️
x/auth/types/account.go	`76.83% <0.00%> (-2.50%)`	⬇️
... and 103 more

cmd/statediff/main.go

+		for _, pkg := range pkg.Imports {
+			addPkg(pkg)
+		}


cmd/statediff/main.go

+	for n := range rootFuncs {
+		missing = append(missing, n.typ+"."+n.fun)
+	}


cmd/statediff/main.go

elias-orijtech · 2022-11-10T18:04:23Z

This is now ready for review.

odeke-em

LGTM, thank you @elias-orijtech! Just one comment though.

Kindly cc-ing @ebuchman too

.github/workflows/statediff.yml

odeke-em · 2022-11-11T05:06:49Z

@elias-orijtech please update the commit messages to link to the issue to be fixed:

Fixes #13518

So that it gets auto closed and a related PR is linked.

tac0turtle · 2022-11-11T07:20:24Z

does this post a comment in the pr or only fail in ci?

elias-orijtech · 2022-11-11T21:50:26Z

does this post a comment in the pr or only fail in ci?

It didn't, but now it does :) See elias-orijtech#106 (comment) for an example comment it posts (once pr PR).

tac0turtle · 2022-11-12T21:02:22Z

.github/workflows/statediff.yml

+      - uses: actions/checkout@v3
+      - uses: orijtech/statediff@main
+        with:
+          roots: 'github.com/cosmos/cosmos-sdk/baseapp.BaseApp.DeliverTx,github.com/cosmos/cosmos-sdk/baseapp.BaseApp.BeginBlock,github.com/cosmos/cosmos-sdk/baseapp.BaseApp.EndBlock,github.com/cosmos/cosmos-sdk/baseapp.BaseApp.Commit'


could you explain what this does?

oh I read what this does. I don't think this solves the issue described above.

The linked issue says

"State machine relevant code should basically be anything reachable from BeginBlock/DeliverTx/EndBlock/Commit."

This tool implements the basic reachability analysis: function or method calls. With the GitHub and testing machinery in place, the tool can easily be expanded. For example, what makes code such as x/staking/keeper/msg_server.go state code? Called by a root function/method, implementing a certain interface, something else? If you come up with the rule, I can add it to the tool.

@ebuchman could you please chime in too? @elias-orijtech implemented checking code reachable from the desired roots. Please chime in with suggestions for the use cases if it doesn't fit what he implemented. Thanks @tac0turtle @alexanderbez for the co-reviews!

tac0turtle

If we make a state machine breaking change in x/staking/keeper/msg_server.go it wouldn't fire. The tool may be to simple for what is wanted.

could you touch on how this tool works?

elias-orijtech · 2022-11-14T14:54:29Z

If we make a state machine breaking change in x/staking/keeper/msg_server.go it wouldn't fire. The tool may be to simple for what is wanted.

For now, maybe, but tool can be expanded to detect more cases. I can change "Fixes #13518" to "Update #13518" to reflect that, if you like. This PR is mostly about getting the ball rolling with a MVP tool and all the machinery for integrating with GitHub.

could you touch on how this tool works?

There is a bit of explanation in https://github.com/orijtech/statediff. Basically, the tool detect whether a PR touches any line that belongs to the graph of function or methods called from the roots.

alexanderbez · 2022-11-14T16:03:45Z

So this job mainly just checks if code was modified/added/removed in an execution flow that is regarded as part of the state machine? If so, how exactly does it determine that? Can there be false negatives? What about false positives?

elias-orijtech · 2022-11-14T16:52:55Z

So this job mainly just checks if code was modified/added/removed in an execution flow that is regarded as part of the state machine? If so, how exactly does it determine that? Can there be false negatives? What about false positives?

False negatives can happen if a state function or method is unreachable through direct function or method calls from a root. For example, through an interface method call.

False positives can happen if a PR touches a state function or method, but the change is not actually state changing. For example a comment or a performance optimization.

alexanderbez · 2022-11-14T18:19:07Z

I see. Ok, then I don't have any strong objections to this. I'm happy to give an ACK. I'm not convinced how much it will really buy us, but at least we can experiment with it.

Let's keep this as a non-required job.

The workflow uses the new github.com/orijtech/statediff tool that builds a callgraph from a set of root methods and functions, and checks whether a patch touches it. Fixes cosmos#13518 Signed-off-by: Elias Naur <elias@orijtech.com>

elias-orijtech · 2022-11-24T21:11:29Z

If we make a state machine breaking change in x/staking/keeper/msg_server.go it wouldn't fire. The tool may be to simple for what is wanted.

Can you clarify what you mean by too simple? Just adding roots (say, github.com/cosmos/cosmos-sdk/x/staking/keeper.msgServer.Delegate) will trigger on any changes to Delegate or one of its callees.

I've tried a few implementations to catch changes to msg_server.go automatically, but to make productive progress can you say what makes code such as that in msg_server.go state-sensitive? For example "any code called by the gRPC endpoints", or "implementations of interface X/Y/Z...".

tac0turtle

lets merge it and see how it works

tac0turtle · 2022-11-27T11:49:35Z

Can you clarify what you mean by too simple? Just adding roots (say, github.com/cosmos/cosmos-sdk/x/staking/keeper.msgServer.Delegate) will trigger on any changes to Delegate or one of its callees.

I actually need to see it work, I could be wrong with my assumption.

I've tried a few implementations to catch changes to msg_server.go automatically, but to make productive progress can you say what makes code such as that in msg_server.go state-sensitive? For example "any code called by the gRPC endpoints", or "implementations of interface X/Y/Z...".

This is a good question and not something we have well defined I believe. There is the unspoken things we shouldn't touch, but its better to write them down

odeke-em · 2022-11-27T13:42:08Z

Thank you for the reviews @alexanderbez @tac0turtle plus merge and for the good work @elias-orijtech

elias-orijtech requested a review from a team as a code owner November 5, 2022 00:02

elias-orijtech marked this pull request as draft November 5, 2022 00:02

github-actions bot added the Type: CI label Nov 5, 2022

github-advanced-security bot found potential problems Nov 5, 2022

View reviewed changes

julienrbrt reviewed Nov 5, 2022

View reviewed changes

cmd/statediff/main.go Outdated Show resolved Hide resolved

elias-orijtech force-pushed the main branch from 49400b0 to 323bb0f Compare November 8, 2022 23:09

elias-orijtech changed the title ~~cmd/statediff: add tool for reporting possible state-changing diffs~~ ci: add workflow for reporting possible state-changing diffs Nov 10, 2022

elias-orijtech force-pushed the main branch 4 times, most recently from 86b83ab to 7d7a1f6 Compare November 10, 2022 18:04

elias-orijtech marked this pull request as ready for review November 10, 2022 18:04

odeke-em approved these changes Nov 10, 2022

View reviewed changes

.github/workflows/statediff.yml Outdated Show resolved Hide resolved

elias-orijtech force-pushed the main branch 2 times, most recently from 52a167f to d02a4f9 Compare November 10, 2022 22:03

elias-orijtech force-pushed the main branch from 5e3b3d9 to d59d040 Compare November 11, 2022 21:43

tac0turtle reviewed Nov 12, 2022

View reviewed changes

tac0turtle requested changes Nov 12, 2022

View reviewed changes

alexanderbez approved these changes Nov 14, 2022

View reviewed changes

amaury1093 self-assigned this Nov 16, 2022

elias-orijtech force-pushed the main branch from f049932 to e05613b Compare November 23, 2022 14:11

Merge branch 'main' into main

dd59995

tac0turtle approved these changes Nov 27, 2022

View reviewed changes

tac0turtle enabled auto-merge (squash) November 27, 2022 11:48

tac0turtle merged commit 60fdbd0 into cosmos:main Nov 27, 2022

odeke-em mentioned this pull request Nov 27, 2022

tool to check SDK diffs and highlight just state-machine relevant code #13518

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: add workflow for reporting possible state-changing diffs #13774

ci: add workflow for reporting possible state-changing diffs #13774

elias-orijtech commented Nov 5, 2022 •

edited

Loading

codecov bot commented Nov 5, 2022 •

edited

Loading

elias-orijtech commented Nov 10, 2022

odeke-em left a comment

odeke-em commented Nov 11, 2022

tac0turtle commented Nov 11, 2022

elias-orijtech commented Nov 11, 2022

tac0turtle Nov 12, 2022

tac0turtle Nov 12, 2022

elias-orijtech Nov 14, 2022

odeke-em Nov 27, 2022

tac0turtle left a comment •

edited

Loading

elias-orijtech commented Nov 14, 2022

alexanderbez commented Nov 14, 2022

elias-orijtech commented Nov 14, 2022

alexanderbez commented Nov 14, 2022

elias-orijtech commented Nov 24, 2022

tac0turtle left a comment

tac0turtle commented Nov 27, 2022

odeke-em commented Nov 27, 2022

ci: add workflow for reporting possible state-changing diffs #13774

ci: add workflow for reporting possible state-changing diffs #13774

Conversation

elias-orijtech commented Nov 5, 2022 • edited Loading

codecov bot commented Nov 5, 2022 • edited Loading

Codecov Report

elias-orijtech commented Nov 10, 2022

odeke-em left a comment

Choose a reason for hiding this comment

odeke-em commented Nov 11, 2022

tac0turtle commented Nov 11, 2022

elias-orijtech commented Nov 11, 2022

tac0turtle Nov 12, 2022

Choose a reason for hiding this comment

tac0turtle Nov 12, 2022

Choose a reason for hiding this comment

elias-orijtech Nov 14, 2022

Choose a reason for hiding this comment

odeke-em Nov 27, 2022

Choose a reason for hiding this comment

tac0turtle left a comment • edited Loading

Choose a reason for hiding this comment

elias-orijtech commented Nov 14, 2022

alexanderbez commented Nov 14, 2022

elias-orijtech commented Nov 14, 2022

alexanderbez commented Nov 14, 2022

elias-orijtech commented Nov 24, 2022

tac0turtle left a comment

Choose a reason for hiding this comment

tac0turtle commented Nov 27, 2022

odeke-em commented Nov 27, 2022

elias-orijtech commented Nov 5, 2022 •

edited

Loading

codecov bot commented Nov 5, 2022 •

edited

Loading

tac0turtle left a comment •

edited

Loading