feat: Add `OPERATOR` tier #3316

victor-yanev · 2024-12-09T14:07:40Z

Description:

Instead of tracking the remaining total budget in-memory inside HbarLimitService (we would have to divide it by the number of Kubernetes instances, etc - it is creating unnecessary complexity in the configurations), we could create a new subscription tier (SubscriptionTier.OPERATOR) and add a pre-configured spending plan in the JSON configuration file which links the EVM address(es) of the relay operator account(s) to it.

In this way we can only set the total limit for the OPERATOR tier to HBAR_RATE_LIMIT_TINYBAR and it will be shared by all instances of the relay.

Related issue(s):

Fixes #3099

Notes for reviewer:

Checklist

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

Signed-off-by: Victor Yanev <[email protected]>

github-actions · 2024-12-09T14:33:04Z

Test Results

20 files - 1 268 suites - 47 46m 4s ⏱️ -29s
610 tests +3 603 ✅ + 5 4 💤 ±0 3 ❌ - 2
686 runs - 8 679 ✅ ± 0 4 💤 ±0 3 ❌ - 8

For more details on these failures, see this check.

Results for commit f71eaf6. ± Comparison against base commit c006489.

This pull request removes 1 and adds 4 tests. Note that renamed tests count towards both.

"before all" hook in "@precompile-calls Tests for eth_call with HTS" ‑ RPC Server Acceptance Tests Acceptance tests @precompile-calls Tests for eth_call with HTS "before all" hook in "@precompile-calls Tests for eth_call with HTS"

calls createFungibleToken with custom fees ‑ RPC Server Acceptance Tests Acceptance tests @precompile-calls Tests for eth_call with HTS Create HTS token via direct call to Hedera Token service calls createFungibleToken with custom fees
calls createFungibleToken ‑ RPC Server Acceptance Tests Acceptance tests @precompile-calls Tests for eth_call with HTS Create HTS token via direct call to Hedera Token service calls createFungibleToken
calls createNonFungibleToken with fees ‑ RPC Server Acceptance Tests Acceptance tests @precompile-calls Tests for eth_call with HTS Create HTS token via direct call to Hedera Token service calls createNonFungibleToken with fees
calls createNonFungibleToken ‑ RPC Server Acceptance Tests Acceptance tests @precompile-calls Tests for eth_call with HTS Create HTS token via direct call to Hedera Token service calls createNonFungibleToken

♻️ This comment has been updated with latest results.

…ient Signed-off-by: Victor Yanev <[email protected]>

Signed-off-by: Victor Yanev <[email protected]>

…nse` Signed-off-by: Victor Yanev <[email protected]>

Signed-off-by: Victor Yanev <[email protected]>

konstantinabl

lgtm! but please fix the failing tests in CI

Signed-off-by: Victor Yanev <[email protected]>

sonarcloud · 2024-12-10T15:39:37Z

Quality Gate failed

Failed conditions
4.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

codecov · 2024-12-10T15:59:39Z

Codecov Report

Attention: Patch coverage is 87.75510% with 6 lines in your changes missing coverage. Please review.

Project coverage is 85.00%. Comparing base (c006489) to head (f71eaf6).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...s/relay/src/lib/services/hbarLimitService/index.ts	89.47%	3 Missing and 1 partial ⚠️
packages/relay/src/lib/relay.ts	50.00%	0 Missing and 1 partial ⚠️
.../relay/src/lib/services/hapiService/hapiService.ts	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3316      +/-   ##
==========================================
+ Coverage   84.79%   85.00%   +0.21%     
==========================================
  Files          69       69              
  Lines        4637     4682      +45     
  Branches     1041     1048       +7     
==========================================
+ Hits         3932     3980      +48     
+ Misses        398      392       -6     
- Partials      307      310       +3

Flag	Coverage Δ
config-service	`98.14% <ø> (ø)`
relay	`79.72% <83.67%> (+0.98%)`	⬆️
server	`83.28% <ø> (ø)`
ws-server	`36.66% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ckages/config-service/src/services/globalConfig.ts	`100.00% <ø> (ø)`
...y/src/lib/db/types/hbarLimiter/subscriptionTier.ts	`100.00% <100.00%> (ø)`
packages/relay/src/lib/relay.ts	`91.54% <50.00%> (-1.31%)`	⬇️
.../relay/src/lib/services/hapiService/hapiService.ts	`79.78% <87.50%> (-0.65%)`	⬇️
...s/relay/src/lib/services/hbarLimitService/index.ts	`93.12% <89.47%> (+0.42%)`	⬆️

... and 6 files with indirect coverage changes

Nana-EC

Nice work.
Some suggestions and questions

Nana-EC · 2024-12-11T03:07:13Z

packages/relay/src/lib/relay.ts

      duration,
    );

    const hapiService = new HAPIService(logger, register, this.cacheService, this.eventEmitter, hbarLimitService);

    this.clientMain = hapiService.getMainClientInstance();
+    if (this.clientMain.operatorAccountId) {
+      hbarLimitService.setOperatorAddress(this.clientMain.operatorAccountId.toSolidityAddress());


Q: why is this needed when it's set in the hbarLimitService constructor?

It's complicated 😄, currently the configuration code in the acceptance tests is quite "spaghetti", I opened a draft PR which attempts to fix it - #3331

Nana-EC · 2024-12-11T03:16:02Z

packages/config-service/src/services/globalConfig.ts

@@ -283,43 +283,31 @@ export class GlobalConfig {
      envName: 'HBAR_RATE_LIMIT_BASIC',
      type: 'number',
      required: false,
-      defaultValue: null,
+      defaultValue: 1_120_000_000, // 11.2 hbar


nit: note the assumed duration for context

It's right below it:

Nana-EC · 2024-12-11T03:16:37Z

docs/configuration.md

@@ -111,7 +111,6 @@ Unless you need to set a non-default value, it is recommended to only populate o
 | `REDIS_RECONNECT_DELAY_MS`                  | "1000"                                | Sets the delay between reconnect retries from the Redis client in ms                                                                                                                                                                                                                                   |
 | `SEND_RAW_TRANSACTION_SIZE_LIMIT`           | "131072"                              | Sets the limit of the transaction size the relay accepts on eth_sendRawTransaction                                                                                                                                                                                                                     |
 | `GET_RECORD_DEFAULT_TO_CONSENSUS_NODE`      | "false"                               | Flag to set if get transaction record logic should first query the mirror node (false) or consensus node via the SDK (true).                                                                                                                                                                           |
-| `HBAR_RATE_LIMIT_WHITELIST`                 | [""]                                  | An array of EVM addresses that are allowed to bypass the HBAR rate limits                                                                                                                                                                                                                              |


Q: Was this ever used?
If so what version was this added in?

This is from the old hbar limiter and it's no longer used anywhere, so I removed it

Nana-EC · 2024-12-11T03:17:13Z

packages/relay/src/lib/db/types/hbarLimiter/subscriptionTier.ts

+  /**
+   * Relay Operators
+   */
+  OPERATOR = 'OPERATOR',


I think this requires an update to the design doc.
Did this already happen or will it be done in follow up?

Good point, I will update the design doc to reflect this

I suggest that we do it in a separate PR to not clutter this one even more with changes

quiet-node

Good work some questions!

quiet-node · 2024-12-11T17:02:30Z

packages/relay/src/lib/services/hbarLimitService/index.ts

+    const operatorId = ConfigService.get('OPERATOR_ID_MAIN');
+    const operatorKey = ConfigService.get('OPERATOR_KEY_MAIN');
+    if (operatorId) {
+      this.operatorAddress = AccountId.fromString(operatorId as string).toSolidityAddress();


I just wanted to confirm that toSolidityAddress() will return only the 20-byte address without the prefix 0x, correct? If that's the case should we add the 0x prefix for consistency?

Sure, we can do that

quiet-node · 2024-12-11T17:17:21Z

packages/relay/src/lib/services/hbarLimitService/index.ts

   * @private
   */
-  private reset: Date;
+  private operatorAddress?: string;


Since OPERATOR_ID or OPERATOR_KEY are mandatory config, should we make this as a mandatory variable as well?

Yes, we can but we would need to duplicate the really screwed up logic in hapiService.ts:

https://github.com/hashgraph/hedera-json-rpc-relay/blob/main/packages/relay/src/lib/services/hapiService/hapiService.ts#L334-L359

I opened another draft PR which fixes the spaghetti code in order to make it really work by only using the ConfigService

quiet-node · 2024-12-11T17:23:03Z

packages/relay/src/lib/services/hbarLimitService/index.ts

+      this.logger.trace(`${requestDetails.formattedRequestId} Creating operator spending plan...`);
+      operatorPlan = await this.createSpendingPlanForAddress(
+        this.operatorAddress!,
+        requestDetails,
+        SubscriptionTier.OPERATOR,


I understand that this aligns with other plans; however, since OPERATOR is a special plan and should have only one account plus should not be updated, was wondering why don't we create operatorPlan directly in the constructor to make it more official?

We would still need this logic, so that the operator plan is recreated in case the cache expires (after the limit duration).

Otherwise, if we don't have this on-demand creation, we will need custom logic to create it in intervals of time which could create race conditions and the operator plan might not yet be created for incoming requests.

quiet-node · 2024-12-11T17:27:42Z

packages/relay/src/lib/services/hbarLimitService/index.ts

+    } else if (operatorKey) {
+      this.operatorAddress = Utils.createPrivateKeyBasedOnFormat(operatorKey as string).publicKey.toEvmAddress();
+    }


hmm since the OPERATOR_ID is a mandatory config, I think this will never be reached.

Yeah, that's true but in some cases the OPERATOR_ID is set to an empty string (e.g., in the charts:install workflow)

quiet-node · 2024-12-11T17:31:15Z

packages/server/tests/acceptance/hbarLimiter.spec.ts

+        for (const plan of operatorPlans) {
+          await hbarSpendingPlanRepository.addToAmountSpent(plan.id, plan.amountSpent, requestDetails, mockTTL);
+        }
+
        // Note: Since the total HBAR budget is shared across the entire Relay instance by multiple test cases,
        //       and expense updates occur asynchronously, the wait below ensures that the HBAR amount has sufficient time
        //       to update properly after each test.


Hmm should we need a new acceptance test to see how new OPERATOR plan work in e2e flow?

Not really, the total limit tests already cover this

quiet-node · 2024-12-11T17:31:19Z

packages/server/tests/acceptance/hbarLimiter.spec.ts

+        for (const plan of operatorPlans) {
+          await hbarSpendingPlanRepository.addToAmountSpent(plan.id, plan.amountSpent, requestDetails, mockTTL);
+        }
+
        // Note: Since the total HBAR budget is shared across the entire Relay instance by multiple test cases,
        //       and expense updates occur asynchronously, the wait below ensures that the HBAR amount has sufficient time
        //       to update properly after each test.


Hmm should we need a new acceptance test to see how new OPERATOR plan work in e2e flow?

#3316 (comment)

The operator plan essentially replaces the in-memory tracking of the total limit that we used to do before

feat: Add OPERATOR tier

4238440

Signed-off-by: Victor Yanev <[email protected]>

victor-yanev added the enhancement New feature or request label Dec 9, 2024

victor-yanev self-assigned this Dec 9, 2024

victor-yanev added this to the 0.63.0 milestone Dec 9, 2024

victor-yanev added the Technical Debt Issue which resolves technical debt label Dec 9, 2024

victor-yanev added 3 commits December 9, 2024 16:12

fix: build error

937ad01

Signed-off-by: Victor Yanev <[email protected]>

fix: remove duplicated fetch of remaining budget

385d86c

Signed-off-by: Victor Yanev <[email protected]>

chore: improve readability

09d99eb

Signed-off-by: Victor Yanev <[email protected]>

victor-yanev added 5 commits December 10, 2024 11:56

fix: make sure that operator address is always up-to-date with the cl…

41fd3a5

…ient Signed-off-by: Victor Yanev <[email protected]>

chore: formatting

0344758

Signed-off-by: Victor Yanev <[email protected]>

chore: formatting

4f8d625

Signed-off-by: Victor Yanev <[email protected]>

chore: fix tests

0307f32

Signed-off-by: Victor Yanev <[email protected]>

fix: do not ignore error when EVM address is not provided to `addExpe…

44823cd

…nse` Signed-off-by: Victor Yanev <[email protected]>

victor-yanev marked this pull request as ready for review December 10, 2024 12:27

victor-yanev requested review from AlfredoG87, lukelee-sl and a team as code owners December 10, 2024 12:27

victor-yanev added 4 commits December 10, 2024 14:29

chore: revert unnecessary change

0f4a4e7

Signed-off-by: Victor Yanev <[email protected]>

chore: revert unnecessary change

006de0f

Signed-off-by: Victor Yanev <[email protected]>

chore: reduce code duplication in metricService.spec.ts

4dd94b6

Signed-off-by: Victor Yanev <[email protected]>

chore: reduce code duplication in metricService.spec.ts

1f4faf2

Signed-off-by: Victor Yanev <[email protected]>

konstantinabl previously approved these changes Dec 10, 2024

View reviewed changes

fix: error in charts:install workflow

ab5e636

Signed-off-by: Victor Yanev <[email protected]>

victor-yanev dismissed konstantinabl’s stale review via ab5e636 December 10, 2024 15:16

fix: hbarLimiter.spec.ts

d30cec3

Signed-off-by: Victor Yanev <[email protected]>

victor-yanev requested a review from Nana-EC as a code owner December 10, 2024 15:33

fix: remove unnecessary rest in hbarLimiter.spec.ts

f71eaf6

Signed-off-by: Victor Yanev <[email protected]>

victor-yanev requested a review from konstantinabl December 10, 2024 15:56

Nana-EC reviewed Dec 11, 2024

View reviewed changes

quiet-node requested changes Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `OPERATOR` tier #3316

feat: Add `OPERATOR` tier #3316

victor-yanev commented Dec 9, 2024 •

edited

Loading

github-actions bot commented Dec 9, 2024 •

edited

Loading

konstantinabl left a comment •

edited

Loading

sonarcloud bot commented Dec 10, 2024

codecov bot commented Dec 10, 2024

Nana-EC left a comment

Nana-EC Dec 11, 2024

victor-yanev Dec 12, 2024

Nana-EC Dec 11, 2024

victor-yanev Dec 12, 2024

Nana-EC Dec 11, 2024

victor-yanev Dec 12, 2024

Nana-EC Dec 11, 2024

victor-yanev Dec 12, 2024

victor-yanev Dec 12, 2024

quiet-node left a comment

quiet-node Dec 11, 2024

victor-yanev Dec 12, 2024

quiet-node Dec 11, 2024

victor-yanev Dec 12, 2024

victor-yanev Dec 12, 2024

quiet-node Dec 11, 2024

victor-yanev Dec 12, 2024

quiet-node Dec 11, 2024

victor-yanev Dec 12, 2024

quiet-node Dec 11, 2024

victor-yanev Dec 12, 2024

quiet-node Dec 11, 2024

victor-yanev Dec 12, 2024

feat: Add OPERATOR tier #3316

Are you sure you want to change the base?

feat: Add OPERATOR tier #3316

Conversation

victor-yanev commented Dec 9, 2024 • edited Loading

github-actions bot commented Dec 9, 2024 • edited Loading

Test Results

konstantinabl left a comment • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Dec 10, 2024

Quality Gate failed

codecov bot commented Dec 10, 2024

Codecov Report

Nana-EC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quiet-node left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: Add `OPERATOR` tier #3316

feat: Add `OPERATOR` tier #3316

victor-yanev commented Dec 9, 2024 •

edited

Loading

github-actions bot commented Dec 9, 2024 •

edited

Loading

konstantinabl left a comment •

edited

Loading