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.

Sign up for GitHub

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

Jump to bottom

[On-Chain] Claim/Proof on-chain events #629

Merged
merged 21 commits into from
Jun 29, 2024
Merged

[On-Chain] Claim/Proof on-chain events #629

merged 21 commits into from
Jun 29, 2024

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Jun 20, 2024 •
edited
Loading

Summary

  1. Refactor claim settle/expire event types to add num_relays and rename compute_units to num_compute_units.
  2. Add claim/proof event types and emit them on-chain.

Issue

Type of change

Select one or more:

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

Testing

Documentation changes (only if making doc changes)

  • make docusaurus_start; only needed if you make doc changes

Local Testing (only if making code changes)

  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • See quickstart guide for instructions

PR Testing (only if making code changes)

  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.
    • THIS IS VERY EXPENSIVE, so only do it after all the reviews are complete.
    • Optionally run make trigger_ci if you want to re-trigger tests without any code changes
    • If tests fail, try re-running failed tests only using the GitHub UI as shown here

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 added on-chain On-chain business logic code health Cleans up some code push-image CI related - pushes images to ghcr.io devnet-test-e2e labels Jun 20, 2024
@bryanchriswhite bryanchriswhite self-assigned this Jun 20, 2024
@github-actions GitHub Actions
Copy link

The image is going to be pushed after the next commit.

You can use make trigger_ci to push an empty commit.

If you also want to run E2E tests, please add devnet-test-e2e label.

@github-actions GitHub Actions
Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks.

You may need to run make trigger_ci to submit an empty commit that'll trigger the tests.

GCP workloads (requires changing the namespace to 629)
Grafana network dashboard for devnet-issue-{issue-id}

github-actions[bot]
github-actions bot reviewed Jun 20, 2024
View reviewed changes
x/proof/types/types.go Outdated Show resolved Hide resolved
This was linked to issues Jun 20, 2024
[Proof & Claim Windows] Publish on-chain events from Claim & Proof Windows #580
Closed
[TODO] @bryanchriswhite's Iteration16 TODOs #586
Closed
@bryanchriswhite bryanchriswhite mentioned this pull request Jun 20, 2024
Closed
15 tasks
@bryanchriswhite bryanchriswhite force-pushed the issues/580/events/refactor+feat branch from dfe9c1f to 3de0683 Compare June 27, 2024 07:13
github-actions[bot]
github-actions bot reviewed Jun 27, 2024
View reviewed changes
proto/poktroll/tokenomics/event.proto Outdated Show resolved Hide resolved
x/proof/keeper/msg_server_create_claim_test.go Outdated Show resolved Hide resolved
x/tokenomics/types/event.pb.go Outdated Show resolved Hide resolved
api/poktroll/tokenomics/event.pulsar.go Outdated Show resolved Hide resolved
@bryanchriswhite bryanchriswhite force-pushed the issues/580/events/refactor+feat branch from 3de0683 to 7f8b902 Compare June 27, 2024 11:04
@bryanchriswhite bryanchriswhite force-pushed the issues/580/events/refactor+feat branch from 7f8b902 to 00ccddc Compare June 27, 2024 11:40
@bryanchriswhite bryanchriswhite force-pushed the issues/580/events/refactor+feat branch from 0131b17 to d48e73e Compare June 27, 2024 11:58
@bryanchriswhite bryanchriswhite marked this pull request as ready for review June 27, 2024 11:59
@bryanchriswhite
Merge remote-tracking branch 'pokt/main' into issues/580/events/refac…
a398ac4 
…tor+feat

* pokt/main:
  [Relayminer] off-chain claim/proof message distribution (#619)
@bryanchriswhite
chore: proto_regen
a0896c3
@bryanchriswhite
Merge remote-tracking branch 'pokt/main' into issues/580/events/refac…
438859e 
…tor+feat

* pokt/main:
  [Proof Module] on-chain claim/proof message distribution validation (#620)
  [AppGateServer] Refactor the AppGate server to use the new shannon-sdk implementation  (#636)
red-0ne
red-0ne reviewed Jun 28, 2024
View reviewed changes
Copy link
Contributor

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

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

Partial review
image
I have yet to look at the tests.

x/tokenomics/keeper/settle_pending_claims.go Show resolved Hide resolved
x/proof/keeper/msg_server_submit_proof.go Outdated Show resolved Hide resolved
proto/poktroll/proof/tx.proto
message MsgSubmitProofResponse {
poktroll.proof.Proof proof = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍
We should consider revisiting our Msg keepers to return whatever they created/updated

Copy link
Member

Choose a reason for hiding this comment

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

TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#663

@bryanchriswhite
chore: reveiew feedback improvements
f381a61
@bryanchriswhite
chore: review improvements
a4d3061
@bryanchriswhite
Merge remote-tracking branch 'pokt/main' into issues/580/events/refac…
1e51c1d 
…tor+feat

* pokt/main:
  [On-Chain] Refactor `grace_period_end_offset_blocks` as a `shared` module param (#628)
  [Docs] Add "Session Windows & On-Chain Parameters" section to claim/proof/lifecycle docs (#632)
red-0ne
red-0ne reviewed Jun 28, 2024
View reviewed changes
Copy link
Contributor

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

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

We have events, that's a great event!

Left a few comments, this is probably the final review.

x/proof/keeper/msg_server_create_claim.go
telemetry.ClaimComputeUnitsCounter(
types.ClaimProofStage_CLAIMED,
numComputeUnits,
err,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we meter numRelays too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a telemetry call, not an event emission. This particular telemetry call is specific to compute units.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm realizing that I misunderstood your message initially but I get it now. I'm not sure what the state of the code was at the time but now it looks like this:

			telemetry.ClaimCounter(types.ClaimProofStage_CLAIMED, 1, err)
			telemetry.ClaimComputeUnitsCounter(
				types.ClaimProofStage_CLAIMED,
				numComputeUnits,
				err,
			)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Did it again. 😅🙈

I've opened #643 to track this.

x/proof/keeper/msg_server_create_claim.go Outdated Show resolved Hide resolved
x/proof/keeper/msg_server_create_claim_test.go
event, err := cosmostypes.ParseTypedEvent(abci.Event(events[0]))
require.NoError(t, err)

claimCreatedEvent, ok := event.(*types.EventClaimCreated)
Copy link
Contributor

Choose a reason for hiding this comment

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

Smooth!

e2e/tests/session_steps_test.go Show resolved Hide resolved
@red-0ne red-0ne self-requested a review June 28, 2024 23:04
red-0ne
red-0ne approved these changes Jun 28, 2024
View reviewed changes
Copy link
Contributor

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

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

Nothing blocking here so preemptively approving.

Feel free to add TODOs if you think so.

@bryanchriswhite bryanchriswhite merged commit 0ed3518 into main Jun 29, 2024
10 checks passed
@bryanchriswhite bryanchriswhite deleted the issues/580/events/refactor+feat branch June 29, 2024 12:32
This was referenced Jul 2, 2024
Merged
Closed
Olshansk
Olshansk reviewed Jul 5, 2024
View reviewed changes
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.

Looks amazing!

x/proof/keeper/msg_server_submit_proof.go
return &types.MsgSubmitProofResponse{}, nil
numRelays, err = claim.GetNumRelays()
if err != nil {
return nil, status.Error(codes.Internal, types.ErrProofInvalidClaimRootHash.Wrap(err.Error()).Error())
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to reuse these errors?

Copy link
Contributor Author

@bryanchriswhite bryanchriswhite Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I believe this error is accurate. If claim.GetNumRelays() or claim.GetNumComputeUnits() fails, it's because the root is invalid.

proto/poktroll/proof/stage.proto

option go_package = "github.com/pokt-network/poktroll/x/proof/types";

enum ClaimProofStage {
Copy link
Member

Choose a reason for hiding this comment

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

Love this

proto/poktroll/proof/tx.proto
message MsgSubmitProofResponse {
poktroll.proof.Proof proof = 1;
Copy link
Member

Choose a reason for hiding this comment

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

TODO?

e2e/tests/session_steps_test.go
@@ -297,3 +250,71 @@ func (s *suite) waitForNewBlockEvent(
s.Log("Success; message detected before timeout.")
}
}

Copy link
Member

Choose a reason for hiding this comment

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

These helpers LGTM. Did you take some time to make sure we reuse them elsewhere? (i.e. some of the work I did that should leverage them)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yessir! 🫡 My observation was that the spaghetti code was strewn between the session, relay, and settlement scenarios.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Olshansk Olshansk Olshansk left review comments

@github-actions GitHub Actions GitHub Actions left review comments

@red-0ne red-0ne red-0ne approved these changes

Assignees

@bryanchriswhite bryanchriswhite

Labels
code health Cleans up some code devnet devnet-test-e2e on-chain On-chain business logic push-image CI related - pushes images to ghcr.io
Projects
Shannon
Status: ✅ Done
Milestone
Shannon Beta TestNet Launch
3 participants
@bryanchriswhite @red-0ne @Olshansk