Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tooling] Add protocheck CLI tool #770

Merged
merged 5 commits into from
Aug 30, 2024
Merged

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Aug 29, 2024

Summary

Add protocheck CLI tool and unstable subcommand to resolve stable serialization issue related to map protobuf message fields:

go run ./tools/scripts/protocheck/cmd unstable -h:

Recursively list or fix all protobuf files which omit the 'stable_marshaler_all' option.

Usage:
  protocheck unstable [flags]

Flags:
  -f, --fix    If present, protocheck will add the 'gogoproto.stable_marshaler_all' option to files which were discovered to be unstable.
  -h, --help   help for unstable

Global Flags:
  -p, --file-pattern string   Set the pattern passed to filepath.Match(), used to include file names which match. (default "*.proto")
  -r, --root string           Set the path of the directory from which to start walking the filesystem tree in search of files matching --file-pattern. (default "./proto")

Issue

This mitigates non-deterministic serialization due to usage of map fields in protobuf message fields.

It does not address any non-deterministic serialization due to mis-usage of repeated protobuf message fields, which is a much more complex concern.

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Documentation: make docusaurus_start; only needed if you make doc changes
  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code
  • I create and reference any new tickets, if applicable
  • I have left TODOs throughout the codebase, if applicable

@bryanchriswhite bryanchriswhite self-assigned this Aug 29, 2024
tools/scripts/protocheck/cmd/root.go Outdated Show resolved Hide resolved
tools/scripts/protocheck/cmd/unstable.go Outdated Show resolved Hide resolved
@bryanchriswhite bryanchriswhite added bug Something isn't working on-chain On-chain business logic code health Cleans up some code tooling Tooling - CLI, scripts, helpers, off-chain, etc... p0 Top priority labels Aug 30, 2024
@bryanchriswhite bryanchriswhite linked an issue Aug 30, 2024 that may be closed by this pull request
11 tasks
@bryanchriswhite bryanchriswhite force-pushed the issues/761/tools/protocheck branch 2 times, most recently from 24b52bb to c8e195e Compare August 30, 2024 11:53
@bryanchriswhite bryanchriswhite marked this pull request as ready for review August 30, 2024 11:56
Copy link
Member

@okdas okdas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, one more thing I'd love this script to do though is to return non-zero exit code on make check_proto_unstable_marshalers.

Currently:

❯ make check_proto_unstable_marshalers
INF Recursively checking for files matching "*.proto" in "./proto"
INF Found 52 unstable marshaler proto files:
INF 	proto/poktroll/gateway/query.proto
... more
❯ echo $?
0

Why: that will allow us to add make check_proto_unstable_marshalers as a CI step which will act as a gate before we merge into the main branch.

@bryanchriswhite
Copy link
Contributor Author

Looks good, one more thing I'd love this script to do though is to return non-zero exit code on `make ...

Nice observation! 😎 That was an oversight on my part. 🙇 Will fix.

@bryanchriswhite bryanchriswhite merged commit 5108ba9 into main Aug 30, 2024
10 checks passed
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bryanchriswhite Thanks for building the tool!

Just a couple things. Per my comment in #772, I'm thinking we could:

  1. Make it part of CI (or at least a TODO)
  2. Document it here: https://dev.poktroll.com/develop/developer_guide/debug_tips

@bryanchriswhite bryanchriswhite deleted the issues/761/tools/protocheck branch August 30, 2024 20:26
bryanchriswhite added a commit that referenced this pull request Sep 5, 2024
…ke-transfer

* pokt/main:
  [Relay Mining] Use per-service difficulty in the relayer (#745)
  [Tilt] Enable rest service by default in Tilt (#793)
  [Docs] Minor doc updates across the board (#792)
  build(deps): bump webpack from 5.89.0 to 5.94.0 in /docusaurus (#775)
  [Docs] Tokenomics Documentation (#750)
  fix: ensure stable_marshaler_all option on all proto files (#772)
  build(deps): bump micromatch from 4.0.5 to 4.0.8 in /docusaurus (#774)
  [Tooling] Add `protocheck` CLI tool (#770)
  Update `adding_params.md` to use `ignite` (#764)
  [Docs] Cleanup & Deprecation (#766)
bryanchriswhite added a commit that referenced this pull request Sep 5, 2024
…actor/transfer-msg_period-param

* issues/657/feat/app-stake-transfer:
  chore: regenerate protobufs
  [Relay Mining] Use per-service difficulty in the relayer (#745)
  [Tilt] Enable rest service by default in Tilt (#793)
  [Docs] Minor doc updates across the board (#792)
  build(deps): bump webpack from 5.89.0 to 5.94.0 in /docusaurus (#775)
  [Docs] Tokenomics Documentation (#750)
  fix: ensure stable_marshaler_all option on all proto files (#772)
  build(deps): bump micromatch from 4.0.5 to 4.0.8 in /docusaurus (#774)
  [Tooling] Add `protocheck` CLI tool (#770)
  Update `adding_params.md` to use `ignite` (#764)
  [Docs] Cleanup & Deprecation (#766)
bryanchriswhite added a commit that referenced this pull request Sep 5, 2024
…ues/657/chore/app-transfer-period

* issues/657/refactor/transfer-msg_period-param:
  chore: regenerate protobufs
  chore: regenerate protobufs
  fix: failing test
  chore: regenerate protobufs
  Revert "refactor: rename param to application_transfer_and_unboding_period_sessions"
  [Relay Mining] Use per-service difficulty in the relayer (#745)
  [Tilt] Enable rest service by default in Tilt (#793)
  [Docs] Minor doc updates across the board (#792)
  build(deps): bump webpack from 5.89.0 to 5.94.0 in /docusaurus (#775)
  [Docs] Tokenomics Documentation (#750)
  fix: ensure stable_marshaler_all option on all proto files (#772)
  build(deps): bump micromatch from 4.0.5 to 4.0.8 in /docusaurus (#774)
  [Tooling] Add `protocheck` CLI tool (#770)
  Update `adding_params.md` to use `ignite` (#764)
  [Docs] Cleanup & Deprecation (#766)
okdas added a commit that referenced this pull request Sep 5, 2024
## Summary

Adds a stable marshaler gate on CI.

## Issue

- #761
- #770
okdas pushed a commit that referenced this pull request Nov 14, 2024
## Summary

Add `protocheck` CLI tool and `unstable` subcommand to resolve stable
serialization issue related to map protobuf message fields:

`go run ./tools/scripts/protocheck/cmd unstable -h`:
```
Recursively list or fix all protobuf files which omit the 'stable_marshaler_all' option.

Usage:
  protocheck unstable [flags]

Flags:
  -f, --fix    If present, protocheck will add the 'gogoproto.stable_marshaler_all' option to files which were discovered to be unstable.
  -h, --help   help for unstable

Global Flags:
  -p, --file-pattern string   Set the pattern passed to filepath.Match(), used to include file names which match. (default "*.proto")
  -r, --root string           Set the path of the directory from which to start walking the filesystem tree in search of files matching --file-pattern. (default "./proto")
```

## Issue

This mitigates non-deterministic serialization due to usage of map
fields in protobuf message fields.

It **does not** address any non-deterministic serialization due to
mis-usage of `repeated` protobuf message fields, which is a much more
complex concern.

- #761

## Type of change

Select one or more:

- [x] New feature, functionality or library
- [ ] Bug fix
- [ ] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [ ] **Unit Tests**: `make go_develop_and_test`
- [ ] **LocalNet E2E Tests**: `make test_e2e`
- [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [x] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [ ] I have left TODOs throughout the codebase, if applicable
okdas added a commit that referenced this pull request Nov 14, 2024
## Summary

Adds a stable marshaler gate on CI.

## Issue

- #761
- #770
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working code health Cleans up some code on-chain On-chain business logic p0 Top priority tooling Tooling - CLI, scripts, helpers, off-chain, etc...
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[Protobufs] Ensure deterministic serialization
3 participants