[Load Testing] gherkin load test follow-up #521

red-0ne · 2024-05-08T22:02:17Z

Summary

Add batched Proofs and batched Claims txs to support concurrent sessions
Add Claims and Proofs counting and assertions
Some fixes to further stabilize the tests
Fix most of the RelayMiner's go-routine leaks

Issue

[Testing] Load Testing Relays Round #1 #20

Type of change

Select one or more:

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

Co-authored-by: Redouane Lakrache <[email protected]>

Co-authored-by: Bryan White <[email protected]>

…roof-integration * pokt/feat/proof-validation: chore: Add godoc comments chore: Update keeper query clients in-code documentation chore: Address review change requests [PubKeyClient] Implement PubKeyClient for on/off-chain usage (#413)

… test/proof-integration

…roof-integration * pokt/feat/proof-validation: fix: test error msg assertion

(cherry picked from commit eab6d984e65a9a07014c9a9839e7b55d125147a1)

* pokt/main: [LocalNet] Fix/localnet regenesis (#424) [CI] Delete validator pod if stuck during E2E test (#415) Fixing reletive link in documentation [Docs] Add code review guidelines & developer tips (#426) [Quick change] Bump default gas limit (#431)

Co-authored-by: Daniel Olshansky <[email protected]> Co-authored-by: h5law <[email protected]>

* pokt/main: A couple comment improvements Update google drive video to youtube video

bryanchriswhite

This is 🔥

Just a few more small suggestions but otherwise, this is good to go IMO. 🙌

load-testing/tests/relays_stress_helpers_test.go

pkg/relayer/session/claim.go

pkg/relayer/session/proof.go

…rkin-load-with-batched-claims

Olshansk

I have a few blocking comments that need to be resolved but have to say that the comments are absolute 🔥.

Given the necessary complexity of this, it's the perfect balance!

pkg/relayer/session/session.go

Olshansk · 2024-05-15T04:30:26Z

pkg/relayer/session/session.go

- // waiting for block to be committed.
- if len(lateSessions) > 0 {
- sessionsToClaimsPublishCh <- lateSessions
+ // If there are any late sessions to be claimed, emit them first.


I don't understand this. I assumed late sessions are too late and shouldn't be claimed, yet here we are trying to claim them.

This makes me question the rest of the business logic in this function.

IsWithinGracePeriod is checking for sessions to claim with <= operator. Which means that it would catch sessions that were supposed to be claimed in previous block heights too.
These late sessions might have their create claim window closed and are no longer eligible to be claimed, but that's not always the case.
Once claim window closing is implemented, they will be filtered out downstream at the appropriate step.

Will put this explanation as a comment but not at this specific line.

load-testing/tests/relays_stress_helpers_test.go

pkg/relayer/session/proof.go

…h-batched-claims

bryanchriswhite

Preemptively approving; two last questions/suggustions but otherwise this LGTM! 💪

bryanchriswhite · 2024-05-16T13:48:00Z

pkg/relayer/proxy/server_builder.go

@@ -17,7 +17,7 @@ import (
 // attempting to retrieve the supplier's on-chain record.
 // This is useful for testing and development purposes, where the supplier
 // may not be staked before the relay miner starts.
-const supplierStakeWaitTime = 2
+const supplierStakeWaitTime = 1


Did you intend to leave this at 1?

Yes, as long as our block time is 2s, other wise the RelayMiner will react too late to its Supplier being freshly staked, missing by the way some RelayRequests that'll fire ASAP.

pkg/relayer/session/session.go

bryanchriswhite · 2024-05-16T15:31:00Z

@red-0ne maybe run make trigger_ci one time as it looks like CI hasn't run since changing bases.

Olshansk · 2024-05-16T18:30:10Z

load-testing/tests/relays_stress_test.go

- s.currentProofCount,
- "unexpected claims and proofs count",
- )
+ // TODO_TECHDEBT: The current counting mechanism for the expected claims and proofs


How much of a concern is this?

Can't tell if it's "hard to do" or a major issue?

Since we don't have full control from within the test over how Gateways distribute their relays or info about which Supplier served a given request.

Although it's possible to do it with some modifications on the AppGateServer, results and failed proofs would anyway be visible on grafana, so I don't believe it to be a big concern.

pkg/relayer/session/claim.go

pkg/relayer/session/session.go

Olshansk

Left a few last-minute questions. Nothing blocking. Love the cleanup into goFoo functions.

Merge it in!

github-actions · 2024-05-16T23:09:48Z

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. If you just created a pull request, you might need to push another commit to produce a container image DevNet can utilize to spin up infrastructure. You can use make trigger_ci to push an empty commit.

Olshansk · 2024-05-17T00:11:41Z

@red-0ne Everything is 🟢

red-0ne and others added 30 commits March 1, 2024 15:08

feat: Implement proof's smt and signature validations

61c8dc8

[Proof] refactor: ring client for on-chain use (#411)

6239e74

Co-authored-by: Redouane Lakrache <[email protected]>

refactor: make session comparaison more generic

835889e

chore: add comments

a01cb26

Merge branch 'main' into feat/proof-validation

8a91394

test: proof module integration - claim creation

6e381a0

refactor: session hydrator errors

fd80fc9

test: improve claim creation error assertions & add cases

763e4f0

cleanup: remove unused test helpers

64d96c7

[PubKeyClient] Implement PubKeyClient for on/off-chain usage (#413)

0b82243

Co-authored-by: Bryan White <[email protected]>

chore: Address review change requests

74e7a09

chore: Update keeper query clients in-code documentation

e4484be

chore: Add godoc comments

3f277e7

fixup: review feedback improvements

5ad9efb

fixup! Merge remote-tracking branch 'pokt/feat/proof-validation' into…

90d89ec

… test/proof-integration

fix: test error msg assertion

2d5af98

Merge remote-tracking branch 'pokt/feat/proof-validation' into test/p…

3f17fce

…roof-integration * pokt/feat/proof-validation: fix: test error msg assertion

fixup! fixup: review feedback improvements

2bd35a0

(cherry picked from commit eab6d984e65a9a07014c9a9839e7b55d125147a1)

refactor: relay min difficulty bits as gov param

422e82f

test: submit proof message success

fc0755f

test: submit proof message error

caea47c

chore: Address review change requests

4bf0b9f

chore: Add missing change requests

75db907

Merge remote-tracking branch 'origin/main' into feat/proof-validation

f7f28bf

chore: Remove pubkey client

3357fea

chore: Fix unit tests and ring client removal consideration

f90ca76

Offline review of 406

46c61dd

review: feedback improvement

b2a78c2

Co-authored-by: Daniel Olshansky <[email protected]> Co-authored-by: h5law <[email protected]>

Merge remote-tracking branch 'pokt/main' into test/gherkin-load

f2408fd

* pokt/main: A couple comment improvements Update google drive video to youtube video

red-0ne mentioned this pull request May 14, 2024

[Application] feat: Add past delegation proofs #518

Merged

14 tasks

fix: Account for initial actors to calculate test duration

7106889

bryanchriswhite requested changes May 14, 2024

View reviewed changes

red-0ne added 3 commits May 15, 2024 03:14

chore: Reduce block waiting log messages

7f37198

fix: Revert stress test feature values

fed66b2

Merge remote-tracking branch 'origin/test/gherkin-load' into test/ghe…

a88207d

…rkin-load-with-batched-claims

Olshansk requested changes May 15, 2024

View reviewed changes

bryanchriswhite mentioned this pull request May 15, 2024

[Load Testing] gherkin/go implementation of test plan #1 #432

Merged

14 tasks

red-0ne added 3 commits May 15, 2024 22:23

Merge remote-tracking branch 'origin/main' into test/gherkin-load-wit…

e78024c

…h-batched-claims

chore: Address reivew change requests

1735a7f

fix: do not send json-rpc relays with id 0

7c69ed8

red-0ne changed the base branch from test/gherkin-load to main May 16, 2024 08:11

red-0ne requested review from bryanchriswhite and Olshansk May 16, 2024 08:11

bryanchriswhite approved these changes May 16, 2024

View reviewed changes

Olshansk reviewed May 16, 2024

View reviewed changes

pkg/relayer/session/claim.go Show resolved Hide resolved

Olshansk reviewed May 16, 2024

View reviewed changes

pkg/relayer/session/session.go Outdated Show resolved Hide resolved

Olshansk approved these changes May 16, 2024

View reviewed changes

red-0ne added the devnet-test-e2e label May 16, 2024

github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels May 16, 2024

chore: Address final reivew change requests

470f4da

red-0ne merged commit d543bb3 into main May 17, 2024
10 checks passed

bryanchriswhite mentioned this pull request May 21, 2024

[Load Test] Document Results from Load Test Plan #1 #513

Closed

13 tasks

Olshansk deleted the test/gherkin-load-with-batched-claims branch May 29, 2024 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Load Testing] gherkin load test follow-up #521

[Load Testing] gherkin load test follow-up #521

red-0ne commented May 8, 2024 •

edited

Loading

bryanchriswhite left a comment

Olshansk left a comment

Olshansk May 15, 2024

red-0ne May 16, 2024

bryanchriswhite left a comment •

edited

Loading

bryanchriswhite May 16, 2024

Olshansk May 16, 2024

red-0ne May 16, 2024

bryanchriswhite commented May 16, 2024 •

edited

Loading

Olshansk May 16, 2024

red-0ne May 16, 2024

Olshansk left a comment

github-actions bot commented May 16, 2024

Olshansk commented May 17, 2024

[Load Testing] gherkin load test follow-up #521

[Load Testing] gherkin load test follow-up #521

Conversation

red-0ne commented May 8, 2024 • edited Loading

Summary

Issue

Type of change

Testing

Sanity Checklist

bryanchriswhite left a comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Olshansk May 15, 2024

Choose a reason for hiding this comment

red-0ne May 16, 2024

Choose a reason for hiding this comment

bryanchriswhite left a comment • edited Loading

Choose a reason for hiding this comment

bryanchriswhite May 16, 2024

Choose a reason for hiding this comment

Olshansk May 16, 2024

Choose a reason for hiding this comment

red-0ne May 16, 2024

Choose a reason for hiding this comment

bryanchriswhite commented May 16, 2024 • edited Loading

Olshansk May 16, 2024

Choose a reason for hiding this comment

red-0ne May 16, 2024

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

github-actions bot commented May 16, 2024

Olshansk commented May 17, 2024

red-0ne commented May 8, 2024 •

edited

Loading

bryanchriswhite left a comment •

edited

Loading

bryanchriswhite commented May 16, 2024 •

edited

Loading