Skip to content

Commit

Permalink
fix: condition for running invalid keys chek and tests
Browse files Browse the repository at this point in the history
  • Loading branch information
Amuhar committed Dec 25, 2023
1 parent 74bbee0 commit e4bddab
Show file tree
Hide file tree
Showing 4 changed files with 151 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,8 @@ export class StakingModuleGuardService {

if (
!lastContractsState ||
currentContractState.nonce !== lastContractsState.nonce
currentContractState.lastChangedBlockHash !==
lastContractsState.lastChangedBlockHash
) {
const invalidKeys = await this.getInvalidKeys(
stakingModuleData,
Expand Down Expand Up @@ -422,10 +423,8 @@ export class StakingModuleGuardService {
if (!firstState || !secondState) return false;
if (firstState.depositRoot !== secondState.depositRoot) return false;

// Check if the nonce values are different. A difference in nonce implies a state change.
if (firstState.nonce !== secondState.nonce) return false;
// If the nonce is unchanged, the state might still have changed.
// Therefore, we also need to compare the 'lastChangedBlockHash'.
// Therefore, we need to compare the 'lastChangedBlockHash' instead
// It's important to note that it's not possible for the nonce to be different
// while having the same 'lastChangedBlockHash'.
if (firstState.lastChangedBlockHash !== secondState.lastChangedBlockHash)
Expand Down
86 changes: 70 additions & 16 deletions src/guardian/staking-module-guard/staking-module-guard.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import {
vettedKeysWithoutDuplicates,
} from './keys.fixtures';
import { InconsistentLastChangedBlockHash } from 'common/custom-errors';
import { KeysValidationModule } from 'guardian/keys-validation/keys-validation.module';
import { KeysValidationService } from 'guardian/keys-validation/keys-validation.service';

jest.mock('../../transport/stomp/stomp.client');

Expand Down Expand Up @@ -52,6 +54,8 @@ describe('StakingModuleGuardService', () => {
let stakingModuleGuardService: StakingModuleGuardService;
let guardianMessageService: GuardianMessageService;
let stakingRouterService: StakingRouterService;
let keysValidationService: KeysValidationService;
let findInvalidKeys: jest.SpyInstance;

beforeEach(async () => {
const moduleRef = await Test.createTestingModule({
Expand All @@ -67,6 +71,7 @@ describe('StakingModuleGuardService', () => {
GuardianMessageModule,
RepositoryModule,
PrometheusModule,
KeysValidationModule,
],
}).compile();

Expand All @@ -75,6 +80,8 @@ describe('StakingModuleGuardService', () => {
stakingModuleGuardService = moduleRef.get(StakingModuleGuardService);
guardianMessageService = moduleRef.get(GuardianMessageService);
stakingRouterService = moduleRef.get(StakingRouterService);
keysValidationService = moduleRef.get(KeysValidationService);
findInvalidKeys = jest.spyOn(keysValidationService, 'findInvalidKeys');

jest.spyOn(loggerService, 'log').mockImplementation(() => undefined);
jest.spyOn(loggerService, 'warn').mockImplementation(() => undefined);
Expand Down Expand Up @@ -344,6 +351,69 @@ describe('StakingModuleGuardService', () => {
expect(mockSendMessageFromGuardian).toBeCalledTimes(1);
expect(mockSignDepositData).toBeCalledTimes(1);
});

it('should call invalid keys check if lastChangedBlockHash was changed', async () => {
const mockSendMessageFromGuardian = jest
.spyOn(guardianMessageService, 'sendMessageFromGuardian')
.mockImplementation(async () => undefined);

const mockIsSameContractsStates = jest.spyOn(
stakingModuleGuardService,
'isSameContractsStates',
);

const mockSignDepositData = jest
.spyOn(securityService, 'signDepositData')
.mockImplementation(async () => signature);

await stakingModuleGuardService.handleCorrectKeys(
{
...stakingModuleData,
lastChangedBlockHash: '0x1',
unusedKeys: [],
vettedUnusedKeys: [],
},
blockData,
);

expect(findInvalidKeys).toBeCalledTimes(1);

findInvalidKeys.mockClear();

await stakingModuleGuardService.handleCorrectKeys(
{
...stakingModuleData,
lastChangedBlockHash: '0x1',
unusedKeys: [],
vettedUnusedKeys: [],
},
blockData,
);

expect(findInvalidKeys).toBeCalledTimes(0);
findInvalidKeys.mockClear();

await stakingModuleGuardService.handleCorrectKeys(
{
...stakingModuleData,
lastChangedBlockHash: '0x2',
unusedKeys: [],
vettedUnusedKeys: [],
},
blockData,
);

expect(findInvalidKeys).toBeCalledTimes(1);

expect(mockIsSameContractsStates).toBeCalledTimes(3);
const { results } = mockIsSameContractsStates.mock;
expect(results[0].value).toBeFalsy();
expect(results[1].value).toBeTruthy();
expect(results[2].value).toBeFalsy();

expect(mockSendMessageFromGuardian).toBeCalledTimes(2);
expect(mockSignDepositData).toBeCalledTimes(2);
});
});

describe('excludeEligibleIntersections', () => {
Expand Down Expand Up @@ -511,22 +581,6 @@ describe('StakingModuleGuardService', () => {
expect(result).toBeFalsy();
});

it('should return false if nonce are different', () => {
// It's important to note that it's not possible for the nonce to be different
// while having the same 'lastChangedBlockHash'.
const state = {
depositRoot: '0x1',
nonce: 1,
blockNumber: 100,
lastChangedBlockHash: 'hash',
};
const result = stakingModuleGuardService.isSameContractsStates(state, {
...state,
nonce: 2,
});
expect(result).toBeFalsy();
});

it('should return false if lastChangedBlockHash are different', () => {
// It's important to note that it's not possible for the nonce to be different
// while having the same 'lastChangedBlockHash'.
Expand Down
54 changes: 54 additions & 0 deletions src/staking-router/staking-router-storage.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { Injectable, LoggerService, Inject } from '@nestjs/common';
import { WINSTON_MODULE_NEST_PROVIDER } from 'nest-winston';
import { Configuration } from 'common/config';
import { KeysApiService } from 'keys-api/keys-api.service';
import { StakingModuleData } from 'guardian';
import { getVettedUnusedKeys } from './vetted-keys';
import { RegistryOperator } from 'keys-api/interfaces/RegistryOperator';
import { RegistryKey } from 'keys-api/interfaces/RegistryKey';
import { SRModule } from 'keys-api/interfaces';
import { Meta } from 'keys-api/interfaces/Meta';
import { InconsistentLastChangedBlockHash } from 'common/custom-errors';
import { SROperatorListWithModule } from 'keys-api/interfaces/SROperatorListWithModule';
import { ELBlockSnapshot } from 'keys-api/interfaces/ELBlockSnapshot';

type UpdatedData = {
operatosByModules: SROperatorListWithModule;
unusedKeys:
meta: Meta;

}

@Injectable()
export class StakingRouterService {
constructor(
@Inject(WINSTON_MODULE_NEST_PROVIDER) protected logger: LoggerService,
protected readonly config: Configuration,
protected readonly keysApiService: KeysApiService,
) {}

protected stakingRouterCache: Record<number, StakingModuleData> = {};


async fetchData() {
// fetch operators and modules
const { data: operatorsByModules, meta } =
await this.keysApiService.getOperatorListWithModule();

const result = operatorsByModules.filter(({ module: stakingModule }) => {
const cacheLastChangedBlockHash =
this.stakingRouterCache[stakingModule.id].lastChangedBlockHash;

return cacheLastChangedBlockHash !== stakingModule.lastChangedBlockHash
? true
: false;
});

}

async fetchOperatorsAndModules() {

}


}
24 changes: 24 additions & 0 deletions test/manifest.e2e-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ import { GanacheProviderModule } from '../src/provider';

import { BlsService } from '../src/bls';
import { GuardianMessageService } from '../src/guardian/guardian-message';
import { KeyValidatorInterface } from '@lido-nestjs/key-validation';

// Mock rabbit straight away
jest.mock('../src/transport/stomp/stomp.client.ts');
Expand All @@ -99,6 +100,9 @@ describe('ganache e2e tests', () => {
let sendDepositMessage: jest.SpyInstance;
let sendPauseMessage: jest.SpyInstance;

let keyValidator: KeyValidatorInterface;
let validateKeys: jest.SpyInstance;

beforeEach(async () => {
server = makeServer(FORK_BLOCK, CHAIN_ID, UNLOCKED_ACCOUNTS);
await server.listen(GANACHE_PORT);
Expand Down Expand Up @@ -147,6 +151,7 @@ describe('ganache e2e tests', () => {
lidoService = moduleRef.get(LidoService);
depositService = moduleRef.get(DepositService);
guardianMessageService = moduleRef.get(GuardianMessageService);
keyValidator = moduleRef.get(KeyValidatorInterface);

// Initializing needed service instead of the whole app
blsService = moduleRef.get(BlsService);
Expand All @@ -165,6 +170,8 @@ describe('ganache e2e tests', () => {
sendPauseMessage = jest
.spyOn(guardianMessageService, 'sendPauseMessage')
.mockImplementation(() => Promise.resolve());

validateKeys = jest.spyOn(keyValidator, 'validateKeys');
});

describe('node checks', () => {
Expand Down Expand Up @@ -1196,6 +1203,18 @@ describe('ganache e2e tests', () => {

await guardianService.handleNewBlock();

expect(validateKeys).toBeCalledTimes(1);
expect(validateKeys).toBeCalledWith(
expect.arrayContaining([
expect.objectContaining({
key: toHexString(pk),
// just some random sign
depositSignature:
'0x8bf4401a354de243a3716ee2efc0bde1ded56a40e2943ac7c50290bec37e935d6170b21e7c0872f203199386143ef12612a1488a8e9f1cdf1229c382f29c326bcbf6ed6a87d8fbfe0df87dacec6632fc4709d9d338f4cf81e861d942c23bba1e',
}),
]),
);

expect(sendDepositMessage).toBeCalledTimes(0);
expect(sendPauseMessage).toBeCalledTimes(0);

Expand Down Expand Up @@ -1226,8 +1245,13 @@ describe('ganache e2e tests', () => {
// list of keys for /keys?used=false mock
mockedKeysApiUnusedKeys(keysApiService, [keyWithWrongSign], newMeta);

validateKeys.mockClear();

await guardianService.handleNewBlock();

expect(validateKeys).toBeCalledTimes(1);
expect(validateKeys).toBeCalledWith([]);

// should found invalid key and skip again
// on this iteration cache will be used
expect(sendDepositMessage).toBeCalledTimes(0);
Expand Down

0 comments on commit e4bddab

Please sign in to comment.