Check guarded relayer on interceptor#7669
Conversation
| return nil | ||
| } | ||
|
|
||
| func (inTx *InterceptedTransaction) checkGuardedRelayer(tx *transaction.Transaction) error { |
There was a problem hiding this comment.
can you instead move the checks into func (txv *txValidator) CheckTxValidity(interceptedTx process.InterceptedTransactionHandler) error ?
this should already be called on the intercepted transactions, and has access to the accounts adapter, so no need to add extra dependencies
There was a problem hiding this comment.
indeed an easier approach.. pushed
There was a problem hiding this comment.
Pull request overview
This PR adds a guarded relayer check to the transaction interceptor for relayed transactions v3. The check prevents guarded accounts from being used as relayers, ensuring consistency between the interceptor validation phase and the transaction processing phase.
Changes:
- Added
AccountsAdapterfield toInterceptedTransactionand its factory - Implemented
checkGuardedRelayermethod to validate that relayers are not guarded accounts - Updated all call sites and test cases to pass the new
AccountsAdapterparameter
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| process/transaction/interceptedTransaction.go | Added AccountsAdapter field and checkGuardedRelayer method to validate relayers aren't guarded |
| process/interceptors/factory/interceptedTxDataFactory.go | Updated factory to accept and pass AccountsAdapter to intercepted transactions |
| process/interceptors/factory/argInterceptedDataFactory.go | Added AccountsAdapter field to factory arguments |
| process/factory/interceptorscontainer/shardInterceptorsContainerFactory.go | Added AccountsAdapter from args.Accounts to factory arguments |
| process/factory/interceptorscontainer/metaInterceptorsContainerFactory.go | Added AccountsAdapter from args.Accounts to factory arguments |
| update/factory/fullSyncInterceptors.go | Added AccountsAdapter from args.Accounts to factory arguments |
| node/node.go | Updated NewInterceptedTransaction call to include stateComponents.AccountsAdapter() |
| epochStart/bootstrap/syncEpochStartMeta.go | Added disabled.NewAccountsAdapter() for bootstrap scenario |
| integrationTests/testHeartbeatNode.go | Added AccountsStub to test setup |
| node/nodeTesting_test.go | Enhanced AccountsStub mock with GetExistingAccountCalled implementation |
| process/transaction/interceptedTransaction_test.go | Added test for nil AccountsAdapter, updated all test helpers, added guarded relayer test case |
| process/interceptors/factory/interceptedMetaHeaderDataFactory_test.go | Added AccountsStub to test mocks |
| process/interceptors/factory/interceptedEquivalentProofsFactory_test.go | Added AccountsStub to test mocks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| shardCoordinator.ComputeIdCalled = func(address []byte) uint32 { | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
The ComputeIdCalled function is assigned twice in this test helper function. The first assignment (lines 184-195) contains the actual logic to handle different addresses, but the second assignment (lines 201-203) unconditionally overwrites it to always return 0. This means the first assignment's logic for distinguishing between senderAddress, relayerAddress, guardedRelayerAddress, recvAddress, and other addresses is never executed. This could cause test failures or incorrect test behavior because the shard coordinator won't properly route addresses to their expected shards.
| shardCoordinator.ComputeIdCalled = func(address []byte) uint32 { | |
| return 0 | |
| } |
| shardCoordinator.SameShardCalled = func(firstAddress, secondAddress []byte) bool { | ||
| return string(firstAddress) == string(relayerAddress) && | ||
| string(secondAddress) == string(senderAddress) | ||
| string(secondAddress) == string(senderAddress) || | ||
| string(firstAddress) == string(guardedRelayerAddress) | ||
| } |
There was a problem hiding this comment.
The logic for SameShardCalled is incomplete and may not behave as intended. The condition should have parentheses to clarify operator precedence. Currently, it's evaluated as (string(firstAddress) == string(relayerAddress) && string(secondAddress) == string(senderAddress)) || (string(firstAddress) == string(guardedRelayerAddress)), which returns true if firstAddress is guardedRelayerAddress regardless of secondAddress. This is likely incorrect - it should probably check if both addresses are in the same shard, possibly using (firstAddress == relayerAddress && secondAddress == senderAddress) || (firstAddress == guardedRelayerAddress && secondAddress == senderAddress) or a similar symmetric check.
| relayerAcc, err := inTx.accountsAdapter.GetExistingAccount(tx.RelayerAddr) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The checkGuardedRelayer method uses GetExistingAccount which returns an error if the account doesn't exist. However, this creates an inconsistency with the transaction processor (shardProcess.go line 730-737) which uses getAccountFromAddress followed by LoadAccount. The processor creates a new account if it doesn't exist, and a new account won't be guarded, so the transaction would be accepted. In the interceptor, a non-existent relayer account will cause GetExistingAccount to return ErrAccNotFound, rejecting the transaction. This means the interceptor could reject valid transactions from new relayers that the processor would accept. Consider using LoadAccount instead or handling the ErrAccNotFound case specifically to allow non-existent accounts (which by definition cannot be guarded).
| relayerAcc, err := inTx.accountsAdapter.GetExistingAccount(tx.RelayerAddr) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| relayerAccount, ok := relayerAcc.(state.UserAccountHandler) | ||
| if !ok { | ||
| return process.ErrWrongTypeAssertion | ||
| } |
There was a problem hiding this comment.
The checkGuardedRelayer method doesn't handle the case where GetExistingAccount returns nil, nil (no error but nil account). The disabled accounts adapter used during bootstrap (epochStart/bootstrap/disabled/disabledAccountsAdapter.go) returns nil, nil for all account operations. If a relayed transaction v3 is processed during bootstrap when the relayer is in the same shard, this would cause line 314's type assertion to fail (nil cannot be asserted to UserAccountHandler) and return ErrWrongTypeAssertion. Consider adding a nil check after getting the account: if check.IfNil(relayerAcc) { return nil } before the type assertion.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?