Improve release frequency for signing keys #842
Conversation
|
""" WalkthroughThe changes refactor the keystore's key release mechanism from a block height-based synchronous process to an asynchronous, block ID-driven approach. The keystore now queries transaction results per block to determine which signing keys to release. This update propagates through the keystore implementation, its usage, and all related tests and mocks. Changes
Sequence Diagram(s)sequenceDiagram
participant API_Server
participant Bootstrap
participant KeyStore
participant FlowClient
API_Server->>Bootstrap: StartAPIServer(ctx)
Bootstrap->>KeyStore: New(ctx, keys, client, config, logger)
Note over KeyStore: Starts processLockedKeys goroutine
API_Server->>KeyStore: NotifyBlock(blockHeader)
KeyStore->>KeyStore: enqueue blockHeader on blockChan
KeyStore->>FlowClient: GetTransactionResultsByBlockID(ctx, blockID)
FlowClient-->>KeyStore: []*TransactionResult
KeyStore->>KeyStore: releasekeys(blockHeight, txResults)
KeyStore->>API_Server: keys released for block
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes identified related to the objectives from the linked issues. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsservices/requester/keystore/key_store.go (10)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (9)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| close(k.done) | ||
| return | ||
| case blockID := <-k.blockChan: | ||
| // TODO: If calling `k.client.GetTransactionResultsByBlockID` for each |
There was a problem hiding this comment.
This is something we need to consider. If calling GetTransactionResultsByBlockID for each new block, is a bit aggressive for ANs, we might consider the approach with the available keys ratio.
There was a problem hiding this comment.
I think it should be OK for each gw to send a single request per block since it's generally low impact. Some of these requests are served by ENs, but it shouldn't be hard to add caching on ANs if it becomes a problem.
this does highlight that we need a tx streaming endpoint.
There was a problem hiding this comment.
how about making these new calls opt-in. that way most GWs would not start polling each block, but the few infra providers that need it could turn it on.
Looking over the implementation, we could probably just skip the lookup if it's not enabled and pass an empty slice
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
services/requester/keystore/key_store.go (1)
143-154: Consider implementing the suggested optimization.The TODO comment raises a valid concern about AN load. The suggested optimization could reduce unnecessary API calls.
Would you like me to implement the available keys ratio optimization to reduce load on Access Nodes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bootstrap/bootstrap.go(1 hunks)services/ingestion/event_subscriber.go(1 hunks)services/ingestion/event_subscriber_test.go(5 hunks)services/requester/keystore/key_store.go(5 hunks)services/requester/keystore/key_store_test.go(6 hunks)services/testutils/mock_client.go(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
services/ingestion/event_subscriber.go (3)
Learnt from: peterargue
PR: onflow/flow-evm-gateway#617
File: api/stream.go:62-67
Timestamp: 2024-10-18T19:26:37.579Z
Learning: In the `flow-evm-gateway` project, within the Go file `api/stream.go`, the `prepareBlockResponse` method includes the Bloom filter as the field `LogsBloom` in the returned `Block` struct.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
services/testutils/mock_client.go (2)
Learnt from: peterargue
PR: onflow/flow-evm-gateway#617
File: api/stream.go:62-67
Timestamp: 2024-10-18T19:26:37.579Z
Learning: In the `flow-evm-gateway` project, within the Go file `api/stream.go`, the `prepareBlockResponse` method includes the Bloom filter as the field `LogsBloom` in the returned `Block` struct.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#615
File: bootstrap/bootstrap.go:167-197
Timestamp: 2024-10-17T18:04:04.165Z
Learning: In the `flow-evm-gateway` Go project, the validation ensuring that `startHeight` is less than or equal to `endHeight` is performed before the `StartTraceDownloader` method in `bootstrap/bootstrap.go`, so additional checks in this method are unnecessary.
bootstrap/bootstrap.go (2)
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
services/requester/keystore/key_store.go (4)
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:50-62
Timestamp: 2025-03-07T01:35:09.751Z
Learning: In the flow-evm-gateway codebase, panics are acceptable in scenarios where immediate detection of critical bugs is desired during development and testing, particularly for invariant violations that should never occur in a correctly functioning system (e.g., when a key is available but locked in the keystore implementation).
services/ingestion/event_subscriber_test.go (2)
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
services/requester/keystore/key_store_test.go (4)
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:50-62
Timestamp: 2025-03-07T01:35:09.751Z
Learning: In the flow-evm-gateway codebase, panics are acceptable in scenarios where immediate detection of critical bugs is desired during development and testing, particularly for invariant violations that should never occur in a correctly functioning system (e.g., when a key is available but locked in the keystore implementation).
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
🔇 Additional comments (18)
services/ingestion/event_subscriber.go (1)
179-179: LGTM! Correctly updated to use block ID.The change from
blockEvents.HeighttoblockEvents.BlockIDproperly aligns with the keystore refactoring to use block IDs instead of block heights for asynchronous key release processing.bootstrap/bootstrap.go (1)
246-246: LGTM! Correctly updated keystore initialization.The updated constructor signature properly provides the keystore with the required dependencies (context, client, logger) to support asynchronous key release processing based on transaction results.
services/testutils/mock_client.go (2)
21-21: LGTM! New mock function field added.The
GetTransactionResultsByBlockIDFuncfield correctly follows the established pattern for mock function fields in this client.
54-62: LGTM! Mock method implementation follows established pattern.The
GetTransactionResultsByBlockIDmethod correctly implements the standard mock pattern: delegate to the function field if set, otherwise fall back to the embedded client's method.services/ingestion/event_subscriber_test.go (5)
47-51: LGTM! Test updated for new keystore constructor.The changes correctly introduce shared context and logger variables and update the keystore initialization to use the new constructor signature requiring context, client, and logger.
91-95: LGTM! Consistent test updates.The test correctly uses shared context and logger variables and updates the keystore initialization to match the new constructor signature.
197-201: LGTM! Test properly updated.The keystore initialization correctly uses the new constructor signature with context, client, and logger dependencies.
264-268: LGTM! Test updates are consistent.The changes maintain consistency with other test functions by using shared variables and the updated keystore constructor.
330-334: LGTM! Final test function properly updated.The test correctly follows the established pattern with shared context and logger variables and updated keystore initialization.
services/requester/keystore/key_store_test.go (6)
32-32: LGTM! Keystore constructor updated correctly.The keystore initialization properly uses the new constructor signature with context, client, and logger dependencies.
74-97: LGTM! Mock client setup for testing expiration behavior.The mock client correctly implements the
GetTransactionResultsByBlockIDFuncto test key expiration behavior, returning empty results for the specific block ID and finalized transaction results for others.
113-113: LGTM! Updated to use block ID for notification.The change from block height to block ID (
blockID10) correctly aligns with the new keystore interface.
119-123: LGTM! Properly handles asynchronous processing.The change to use block ID for notification and the added sleep appropriately accounts for the asynchronous
processLockedKeysbackground goroutine processing time.
141-148: LGTM! Constructor updated with all required dependencies.The keystore initialization correctly provides context, account keys, client, and logger to support the new asynchronous processing capabilities.
194-194: LGTM! Final keystore constructor update.The concurrent use test properly uses the updated keystore constructor with all required dependencies.
services/requester/keystore/key_store.go (3)
24-34: LGTM! Well-structured additions for async processing.The new fields properly support the asynchronous block-based key release mechanism with appropriate shutdown signaling.
39-68: LGTM! Constructor properly handles the new async design.Good optimization to skip the background goroutine when no keys are present (index mode).
100-108: Add handling for full channel buffer.The current implementation silently drops block notifications when
blockChanis full. This could delay key releases during high load.Consider adding a default case that logs when blocks are dropped:
select { case <-k.done: k.logger.Warn().Msg( "received `NotifyBlock` while the server is shutting down", ) -default: +case k.blockChan <- blockID: + // Successfully queued block for processing +default: + k.logger.Warn(). + Str("blockID", blockID.String()). + Msg("block notification dropped - channel full") k.blockChan <- blockID }Alternatively, consider making this a blocking send to ensure all blocks are processed, or increase the buffer size if dropping is acceptable.
⛔ Skipped due to learnings
Learnt from: peterargue PR: onflow/flow-evm-gateway#772 File: services/requester/keystore/key_store.go:94-97 Timestamp: 2025-03-07T01:34:25.462Z Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.Learnt from: peterargue PR: onflow/flow-evm-gateway#617 File: api/stream.go:62-67 Timestamp: 2024-10-18T19:26:37.579Z Learning: In the `flow-evm-gateway` project, within the Go file `api/stream.go`, the `prepareBlockResponse` method includes the Bloom filter as the field `LogsBloom` in the returned `Block` struct.
m-Peter
force-pushed
the
mpeter/improve-signing-keys-release-frequency
branch
3 times, most recently
from
4ed4dd4 to
f1f3f8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/key_store_release_test.go (2)
90-90: Document or dynamically generate the test address.The hardcoded address lacks context. Consider adding a comment explaining its purpose or generate it dynamically.
- testAddr := common.HexToAddress("55253ed90B70b96C73092D8680915aaF50081194") + // Use a deterministic test address for balance verification + testAddr := common.HexToAddress("55253ed90B70b96C73092D8680915aaF50081194")Alternatively, generate it dynamically:
testKey, _ := crypto.GenerateKey() testAddr := crypto.PubkeyToAddress(testKey.PublicKey)
114-121: Improve assertion message and consider additional verification.The assertion message is grammatically awkward. Also, consider adding more explicit verification of the key release mechanism.
expectedBalance := int64(keyCount) * transferAmount assert.Eventually(t, func() bool { balance, err := rpcTester.getBalance(testAddr) require.NoError(t, err) return balance.Cmp(big.NewInt(expectedBalance)) == 0 - }, time.Second*15, time.Second*1, "all transactions were not executed") + }, time.Second*15, time.Second*1, "expected %d valid transactions to execute, resulting in balance of %d", keyCount, expectedBalance)Consider also logging transaction receipts to verify which transactions succeeded:
// After all transactions are submitted, verify receipts for valid nonces validNonces := []uint64{0, 1, 2, 3, 4} for _, nonce := range validNonces { // Verify transaction with this nonce was mined }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bootstrap/bootstrap.go(1 hunks)services/ingestion/event_subscriber.go(1 hunks)services/ingestion/event_subscriber_test.go(5 hunks)services/requester/keystore/key_store.go(5 hunks)services/requester/keystore/key_store_test.go(6 hunks)services/testutils/mock_client.go(2 hunks)tests/key_store_release_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- services/ingestion/event_subscriber.go
- services/testutils/mock_client.go
- bootstrap/bootstrap.go
- services/ingestion/event_subscriber_test.go
- services/requester/keystore/key_store_test.go
- services/requester/keystore/key_store.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
tests/key_store_release_test.go (4)
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:50-62
Timestamp: 2025-03-07T01:35:09.751Z
Learning: In the flow-evm-gateway codebase, panics are acceptable in scenarios where immediate detection of critical bugs is desired during development and testing, particularly for invariant violations that should never occur in a correctly functioning system (e.g., when a key is available but locked in the keystore implementation).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
m-Peter
force-pushed
the
mpeter/improve-signing-keys-release-frequency
branch
from
f1f3f8e to
eed7b42
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bootstrap/bootstrap.go(1 hunks)services/ingestion/event_subscriber.go(1 hunks)services/ingestion/event_subscriber_test.go(5 hunks)services/requester/keystore/key_store.go(5 hunks)services/requester/keystore/key_store_test.go(6 hunks)services/testutils/mock_client.go(2 hunks)tests/key_store_release_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- services/ingestion/event_subscriber.go
- services/testutils/mock_client.go
- bootstrap/bootstrap.go
- services/ingestion/event_subscriber_test.go
- services/requester/keystore/key_store_test.go
- services/requester/keystore/key_store.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
tests/key_store_release_test.go (5)
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:50-62
Timestamp: 2025-03-07T01:35:09.751Z
Learning: In the flow-evm-gateway codebase, panics are acceptable in scenarios where immediate detection of critical bugs is desired during development and testing, particularly for invariant violations that should never occur in a correctly functioning system (e.g., when a key is available but locked in the keystore implementation).
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/batch_tx_pool.go:0-0
Timestamp: 2025-06-19T11:36:25.478Z
Learning: In Go, when copying maps that contain slices (like `map[gethCommon.Address][]pooledEvmTx`), perform deep copies by iterating over the map and copying each slice individually using `make()` and `copy()` to avoid shared references that could lead to race conditions and data corruption.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (5)
tests/key_store_release_test.go (5)
1-21: LGTM! Clean imports and package structure.The imports are well-organized and include all necessary dependencies for the key store release test.
23-44: LGTM! Excellent improvement over hardcoded sleep.The readiness check using
require.Eventuallyproperly addresses the past review feedback about replacing hardcoded sleep with emulator readiness polling.
46-73: LGTM! Proper multi-key account setup for key rotation testing.The configuration and multi-key account creation are well-structured and appropriate for testing the key release mechanism with key rotation.
75-87: LGTM! Proper gateway startup without unnecessary delays.The ready channel is used correctly without additional sleep, properly addressing the past review feedback about removing unnecessary delays.
124-131: LGTM! Sound final assertion logic.The balance calculation and eventual assertion are correctly implemented, providing appropriate timeout and retry logic for transaction processing.
m-Peter
force-pushed
the
mpeter/improve-signing-keys-release-frequency
branch
from
eed7b42 to
4677a8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/key_store_release_test.go (3)
103-103: Fix typo in comment.There's a typo in the comment.
- // Finalliy, we assert that `testAddr` has the expected balance + // Finally, we assert that `testAddr` has the expected balance
94-105: Improve test documentation clarity.The comment explaining the nonce sequence logic could be clearer about which specific transactions are expected to succeed and fail.
Consider expanding the comment to explicitly state which nonces will succeed (0, 1, 2, 3, 4) and which will fail, making the test logic more transparent:
- // The first 6 nonces: [5, 4, 3, 2, 1, 1], are all invalid EVM - // transactions that will fail with `nonce too high`. - // Given that we created our COA above with a `keyCount := 5`, - // we assert that transaction submission still functions properly - // and that signing keys get released frequently, with each new - // Flow block. - // The rest 8 nonces: [0, 1, 5, 2, 5, 3, 5, 4], have 5 valid - // EVM transactions and in between there are 3 invalid EVM - // transactions. + // The first 6 nonces: [5, 4, 3, 2, 1, 1], are all invalid EVM + // transactions that will fail with `nonce too high` (fresh account starts at nonce 0). + // Given that we created our COA above with a `keyCount := 5`, + // we assert that transaction submission still functions properly + // and that signing keys get released frequently, with each new + // Flow block. + // The rest 8 nonces: [0, 1, 5, 2, 5, 3, 5, 4], contain 5 valid + // EVM transactions (nonces 0, 1, 2, 3, 4) and 3 invalid ones + // (the duplicate nonce 5 submissions).
23-130: Consider adding test timeout and resource cleanup.The test relies on eventually consistent behavior but lacks an overall timeout. Consider adding a test timeout context to prevent hanging tests.
func Test_KeyStoreSigningKeysRelease(t *testing.T) { + // Set overall test timeout + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + srv, err := startEmulator(true) require.NoError(t, err) - - ctx, cancel := context.WithCancel(context.Background()) defer func() { - cancel() srv.Stop() }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bootstrap/bootstrap.go(1 hunks)services/ingestion/event_subscriber.go(1 hunks)services/ingestion/event_subscriber_test.go(5 hunks)services/requester/keystore/key_store.go(5 hunks)services/requester/keystore/key_store_test.go(6 hunks)services/testutils/mock_client.go(2 hunks)tests/key_store_release_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- bootstrap/bootstrap.go
- services/ingestion/event_subscriber.go
- services/requester/keystore/key_store_test.go
- services/testutils/mock_client.go
- services/ingestion/event_subscriber_test.go
- services/requester/keystore/key_store.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
tests/key_store_release_test.go (8)
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:50-62
Timestamp: 2025-03-07T01:35:09.751Z
Learning: In the flow-evm-gateway codebase, panics are acceptable in scenarios where immediate detection of critical bugs is desired during development and testing, particularly for invariant violations that should never occur in a correctly functioning system (e.g., when a key is available but locked in the keystore implementation).
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/batch_tx_pool.go:0-0
Timestamp: 2025-06-19T11:36:25.478Z
Learning: In Go, when copying maps that contain slices (like `map[gethCommon.Address][]pooledEvmTx`), perform deep copies by iterating over the map and copying each slice individually using `make()` and `copy()` to avoid shared references that could lead to race conditions and data corruption.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#615
File: bootstrap/bootstrap.go:167-197
Timestamp: 2024-10-17T18:04:04.165Z
Learning: In the `flow-evm-gateway` Go project, the validation ensuring that `startHeight` is less than or equal to `endHeight` is performed before the `StartTraceDownloader` method in `bootstrap/bootstrap.go`, so additional checks in this method are unnecessary.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#682
File: services/ingestion/event_subscriber.go:85-90
Timestamp: 2025-01-24T20:15:10.908Z
Learning: When calculating blocks to catch up in Flow, initialize the counter to 0 and only set it when the latest chain height is greater than the current height (latestOnChainHeight > currentHeight). There's no need to handle the else case as 0 blocks to catch up is the correct default when we're at the same height or ahead.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#617
File: api/stream.go:62-67
Timestamp: 2024-10-18T19:26:37.579Z
Learning: In the `flow-evm-gateway` project, within the Go file `api/stream.go`, the `prepareBlockResponse` method includes the Bloom filter as the field `LogsBloom` in the returned `Block` struct.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (2)
tests/key_store_release_test.go (2)
123-129: Verify balance calculation logic.The test expects exactly 5 successful transactions based on the nonce sequence. Ensure this calculation aligns with the actual transaction success criteria.
The balance calculation correctly expects 5 valid transactions (
keyCount * transferAmount) based on the nonce sequence where nonces 0, 1, 2, 3, 4 should succeed sequentially.
52-53: Fix undefined variable references.The variables
sc.FungibleToken.Addressandsc.FlowToken.Addressare referenced butscis not defined in the imports or anywhere in the file. This will cause compilation errors.Please ensure these variables are properly imported or defined. They likely should reference Flow service contracts.
⛔ Skipped due to learnings
Learnt from: m-Peter PR: onflow/flow-evm-gateway#831 File: services/requester/pool.go:202-214 Timestamp: 2025-06-17T10:29:35.941Z Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.Learnt from: m-Peter PR: onflow/flow-evm-gateway#831 File: services/requester/pool.go:202-214 Timestamp: 2025-06-17T10:29:35.941Z Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.Learnt from: m-Peter PR: onflow/flow-evm-gateway#831 File: tests/tx_batching_test.go:43-50 Timestamp: 2025-06-17T10:21:14.312Z Learning: In Go, package-level variables declared in one file are accessible to all other files within the same package. When reviewing Go code, check other files in the same package before flagging variables as undefined. The `sc` variable in the `tests` package is declared in `tests/helpers.go` and used across test files.Learnt from: peterargue PR: onflow/flow-evm-gateway#772 File: services/requester/keystore/key_store.go:94-97 Timestamp: 2025-03-07T01:34:25.462Z Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.Learnt from: peterargue PR: onflow/flow-evm-gateway#772 File: services/requester/keystore/key_store.go:50-62 Timestamp: 2025-03-07T01:35:09.751Z Learning: In the flow-evm-gateway codebase, panics are acceptable in scenarios where immediate detection of critical bugs is desired during development and testing, particularly for invariant violations that should never occur in a correctly functioning system (e.g., when a key is available but locked in the keystore implementation).
m-Peter
force-pushed
the
mpeter/improve-signing-keys-release-frequency
branch
3 times, most recently
from
5eb750d to
5d72c1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/key_store_release_test.go (2)
65-65: Define missing test account variable.The variable
eoaTestAccountis referenced but not defined in this file. This will cause a compilation error.Please define
eoaTestAccountin this test file or import it from the appropriate package where it's declared.
91-91: Define missing private key variable.The variable
eoaTestPrivateKeyis referenced but not defined in this file. This will cause a compilation error.Please define
eoaTestPrivateKeyin this test file or import it from the appropriate package where it's declared.
🧹 Nitpick comments (1)
tests/key_store_release_test.go (1)
89-89: Replace hardcoded sleep with proper readiness check.The 3-second sleep after bootstrap completion could make tests flaky. Consider implementing a proper readiness check for the EVM gateway.
You could poll for the gateway's readiness by checking if it responds to RPC calls:
- time.Sleep(3 * time.Second) // some time for EVM GW to startup + // Poll for EVM gateway readiness + require.Eventually(t, func() bool { + _, err := rpcTester.getBalance(common.HexToAddress("0x0000000000000000000000000000000000000000")) + return err == nil + }, 10*time.Second, 500*time.Millisecond, "EVM gateway failed to start")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bootstrap/bootstrap.go(1 hunks)services/ingestion/event_subscriber.go(1 hunks)services/ingestion/event_subscriber_test.go(5 hunks)services/requester/keystore/key_store.go(5 hunks)services/requester/keystore/key_store_test.go(6 hunks)services/testutils/mock_client.go(2 hunks)tests/key_store_release_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- services/ingestion/event_subscriber.go
- services/testutils/mock_client.go
- services/requester/keystore/key_store_test.go
- bootstrap/bootstrap.go
- services/ingestion/event_subscriber_test.go
- services/requester/keystore/key_store.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
tests/key_store_release_test.go (9)
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:50-62
Timestamp: 2025-03-07T01:35:09.751Z
Learning: In the flow-evm-gateway codebase, panics are acceptable in scenarios where immediate detection of critical bugs is desired during development and testing, particularly for invariant violations that should never occur in a correctly functioning system (e.g., when a key is available but locked in the keystore implementation).
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/batch_tx_pool.go:0-0
Timestamp: 2025-06-19T11:36:25.478Z
Learning: In Go, when copying maps that contain slices (like `map[gethCommon.Address][]pooledEvmTx`), perform deep copies by iterating over the map and copying each slice individually using `make()` and `copy()` to avoid shared references that could lead to race conditions and data corruption.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#615
File: bootstrap/bootstrap.go:167-197
Timestamp: 2024-10-17T18:04:04.165Z
Learning: In the `flow-evm-gateway` Go project, the validation ensuring that `startHeight` is less than or equal to `endHeight` is performed before the `StartTraceDownloader` method in `bootstrap/bootstrap.go`, so additional checks in this method are unnecessary.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#682
File: services/ingestion/event_subscriber.go:85-90
Timestamp: 2025-01-24T20:15:10.908Z
Learning: When calculating blocks to catch up in Flow, initialize the counter to 0 and only set it when the latest chain height is greater than the current height (latestOnChainHeight > currentHeight). There's no need to handle the else case as 0 blocks to catch up is the correct default when we're at the same height or ahead.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#617
File: api/stream.go:62-67
Timestamp: 2024-10-18T19:26:37.579Z
Learning: In the `flow-evm-gateway` project, within the Go file `api/stream.go`, the `prepareBlockResponse` method includes the Bloom filter as the field `LogsBloom` in the returned `Block` struct.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: tests/tx_batching_test.go:43-50
Timestamp: 2025-06-17T10:21:14.312Z
Learning: In Go, package-level variables declared in one file are accessible to all other files within the same package. When reviewing Go code, check other files in the same package before flagging variables as undefined. The `sc` variable in the `tests` package is declared in `tests/helpers.go` and used across test files.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (1)
tests/key_store_release_test.go (1)
94-133: Excellent test design for key release validation.The test logic is well-structured and comprehensive:
- Uses a clever mix of valid/invalid nonces to test the asynchronous key release mechanism
- Properly handles expected errors while still testing key release
- The final balance assertion correctly validates only successful transactions
- Good use of
assert.Eventuallyfor async validationThis effectively tests the new block ID-driven key release mechanism mentioned in the PR objectives.
m-Peter
force-pushed
the
mpeter/improve-signing-keys-release-frequency
branch
from
5d72c1f to
fb95c78
Compare
| k.keyMu.Lock() | ||
| defer k.keyMu.Unlock() | ||
|
|
||
| for _, txResult := range txResults { |
There was a problem hiding this comment.
Do we care about the value of txResult.Status here?
Given that we received the transaction results for a given block ID, with: GetTransactionResultsByBlockID, isn't it safe to release the account keys associated with the blocks' transactions, regardless of transaction result status?
There was a problem hiding this comment.
there are some exceptions that I can think of where simply unlocking the key would be problematic because the transaction failed in a way where sequence number did not increase:
- transaction had an incorrect proposer signature
- transaction had an incorrect sequence number
- incorrect reference block
It might be easiest to simply re-fetch the account key if there was an issue, but not sure if that is feasible performance wise
The alternative is to differentiate the special error cases where the sequence number did not increase...
There was a problem hiding this comment.
Good call-out @janezpodhostnik . Let me go case by case:
- I can't think of a scenario that would make this case manifest. Unless the node operator did something very wrong with the COA setup. But anyway, that would mean that no transactions would go through at all, it won't be a transient behavior. The EVM GW node will be non-functional upon bootstrap.
- This case I have seen happen every now and then, so good call-out. However, for every Flow transaction, we do fetch the COA account each time, and we use that sequence number, we don't keep the sequence number of each key in-memory. See 1 & 2.
- I think this is also something highly unlikely to happen. For every Flow transaction, we do fetch the latest sealed block, and we use it as the reference block. See 1 & 2 .
That being said, what do you think if we change the logic to unlock the keys for which their transaction result status is TransactionStatusSealed ? This way, for the cases that you mentioned, the respective locked account keys, will only come into availability after ~600 blocks have passed, just like we do now.
The Flow transactions that fail with nonce too low / nonce too high, such as this one: https://www.flowscan.io/tx/7ed8540dabf96edc462c5d7e96925f42abc5ef16b4a32c49bdf7d1fdcc1b9b0f?tab=script, are the majority of the transactions that cause account keys to be locked for ~600 blocks. But they get sealed, so we could take advantage of that.
There was a problem hiding this comment.
... we do fetch the COA account each time, and we use that sequence number ...
Since we do that I think its OK to just unlock the key here no matter the result. I thought we keep track of the sequence number, so we don't have to query it all the time.
m-Peter
force-pushed
the
mpeter/improve-signing-keys-release-frequency
branch
2 times, most recently
from
fdb49f8 to
06cea2d
Compare
m-Peter
force-pushed
the
mpeter/improve-signing-keys-release-frequency
branch
from
06cea2d to
0ba50b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/key_store_release_test.go (1)
86-88: Remove unnecessary sleep after ready signal.The ready channel already signals when bootstrap is complete. The additional 3-second sleep is unnecessary and slows down tests.
<-ready - -time.Sleep(3 * time.Second) // some time for EVM GW to startup
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
bootstrap/bootstrap.go(1 hunks)services/ingestion/event_subscriber.go(1 hunks)services/ingestion/event_subscriber_test.go(5 hunks)services/requester/keystore/key_store.go(4 hunks)services/requester/keystore/key_store_test.go(6 hunks)services/testutils/mock_client.go(2 hunks)tests/key_store_release_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- services/ingestion/event_subscriber.go
- bootstrap/bootstrap.go
- services/ingestion/event_subscriber_test.go
- services/testutils/mock_client.go
- services/requester/keystore/key_store_test.go
- services/requester/keystore/key_store.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
tests/key_store_release_test.go (9)
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:50-62
Timestamp: 2025-03-07T01:35:09.751Z
Learning: In the flow-evm-gateway codebase, panics are acceptable in scenarios where immediate detection of critical bugs is desired during development and testing, particularly for invariant violations that should never occur in a correctly functioning system (e.g., when a key is available but locked in the keystore implementation).
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/batch_tx_pool.go:0-0
Timestamp: 2025-06-19T11:36:25.478Z
Learning: In Go, when copying maps that contain slices (like `map[gethCommon.Address][]pooledEvmTx`), perform deep copies by iterating over the map and copying each slice individually using `make()` and `copy()` to avoid shared references that could lead to race conditions and data corruption.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#615
File: bootstrap/bootstrap.go:167-197
Timestamp: 2024-10-17T18:04:04.165Z
Learning: In the `flow-evm-gateway` Go project, the validation ensuring that `startHeight` is less than or equal to `endHeight` is performed before the `StartTraceDownloader` method in `bootstrap/bootstrap.go`, so additional checks in this method are unnecessary.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#682
File: services/ingestion/event_subscriber.go:85-90
Timestamp: 2025-01-24T20:15:10.908Z
Learning: When calculating blocks to catch up in Flow, initialize the counter to 0 and only set it when the latest chain height is greater than the current height (latestOnChainHeight > currentHeight). There's no need to handle the else case as 0 blocks to catch up is the correct default when we're at the same height or ahead.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#617
File: api/stream.go:62-67
Timestamp: 2024-10-18T19:26:37.579Z
Learning: In the `flow-evm-gateway` project, within the Go file `api/stream.go`, the `prepareBlockResponse` method includes the Bloom filter as the field `LogsBloom` in the returned `Block` struct.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: tests/tx_batching_test.go:43-50
Timestamp: 2025-06-17T10:21:14.312Z
Learning: In Go, package-level variables declared in one file are accessible to all other files within the same package. When reviewing Go code, check other files in the same package before flagging variables as undefined. The `sc` variable in the `tests` package is declared in `tests/helpers.go` and used across test files.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (5)
tests/key_store_release_test.go (5)
22-44: Well-structured test setup with proper readiness checks.The test correctly uses context cancellation, proper cleanup patterns, and polls for emulator readiness instead of using hardcoded sleeps. This ensures reliable test execution.
45-72: Effective multi-key account setup for testing key rotation.The test properly creates a multi-key account with 5 keys to test the key rotation mechanism. The configuration setup correctly uses the created account as the COA (Coinbase of Authority).
90-94: ECDSA key setup is correct.The conversion from hex to ECDSA private key is handled properly. The test address setup is appropriate for the test scenario.
95-123: Comprehensive test logic for key release mechanism.The test effectively exercises the new key release mechanism by:
- Testing both valid and invalid nonces to ensure keys are released regardless of transaction success
- Validating that the system can handle key rotation with the configured 5-key setup
- Properly handling expected "nonce too high" errors while logging for debugging
This aligns well with the PR objective of improving signing key release frequency.
125-132: Well-designed final assertion with proper async handling.The test correctly uses
Eventuallyto wait for all valid transactions to be processed, with appropriate timeout and polling intervals. The expected balance calculation properly accounts for the 5 valid transactions out of the 14 submitted.
janezpodhostnik
left a comment
janezpodhostnik
left a comment
There was a problem hiding this comment.
Looks good, but we maybe want to look into transaction errors where the sequence number is not increased.
| k.keyMu.Lock() | ||
| defer k.keyMu.Unlock() | ||
|
|
||
| for _, txResult := range txResults { |
There was a problem hiding this comment.
there are some exceptions that I can think of where simply unlocking the key would be problematic because the transaction failed in a way where sequence number did not increase:
- transaction had an incorrect proposer signature
- transaction had an incorrect sequence number
- incorrect reference block
It might be easiest to simply re-fetch the account key if there was an issue, but not sure if that is feasible performance wise
The alternative is to differentiate the special error cases where the sequence number did not increase...
| // Optimization to avoid AN calls when no signing keys have | ||
| // been used. For example, when back-filling the EVM GW state, | ||
| // we don't care about releasing signing keys. | ||
| if k.AvailableKeys() == k.size { |
There was a problem hiding this comment.
maybe introduce a function to make it clearer
| if k.AvailableKeys() == k.size { | |
| if !k.HasKeysInUse() { |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
services/requester/keystore/key_store.go (1)
156-167: Consider implementing the TODO optimization for production environments.The commented-out optimization using available keys ratio could be valuable for reducing Access Node load in production. Based on past review comments, there's concern about the aggressiveness of calling
GetTransactionResultsByBlockIDfor each block.Consider implementing the available keys ratio optimization as a configurable feature to balance between key release frequency and Access Node load.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/requester/keystore/key_store.go(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
services/requester/keystore/key_store.go (8)
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:94-97
Timestamp: 2025-03-07T01:34:25.462Z
Learning: The `release` method in KeyStore must use blocking channel sends to ensure no keys are ever dropped from the pool, even if this means potentially blocking until the channel has space.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#615
File: bootstrap/bootstrap.go:167-197
Timestamp: 2024-10-17T18:04:04.165Z
Learning: In the `flow-evm-gateway` Go project, the validation ensuring that `startHeight` is less than or equal to `endHeight` is performed before the `StartTraceDownloader` method in `bootstrap/bootstrap.go`, so additional checks in this method are unnecessary.
Learnt from: m-Peter
PR: onflow/flow-evm-gateway#831
File: services/requester/batch_tx_pool.go:0-0
Timestamp: 2025-06-19T11:36:25.478Z
Learning: In Go, when copying maps that contain slices (like `map[gethCommon.Address][]pooledEvmTx`), perform deep copies by iterating over the map and copying each slice individually using `make()` and `copy()` to avoid shared references that could lead to race conditions and data corruption.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#615
File: services/traces/engine.go:175-178
Timestamp: 2024-10-17T18:05:27.084Z
Learning: In the trace backfill process, returning upon encountering an error when retrieving a block or its Cadence ID is intentional because future blocks are unlikely to exist if these blocks don't.
Learnt from: peterargue
PR: onflow/flow-evm-gateway#772
File: services/requester/keystore/key_store.go:50-62
Timestamp: 2025-03-07T01:35:09.751Z
Learning: In the flow-evm-gateway codebase, panics are acceptable in scenarios where immediate detection of critical bugs is desired during development and testing, particularly for invariant violations that should never occur in a correctly functioning system (e.g., when a key is available but locked in the keystore implementation).
Learnt from: peterargue
PR: onflow/flow-evm-gateway#682
File: services/ingestion/event_subscriber.go:85-90
Timestamp: 2025-01-24T20:15:10.908Z
Learning: When calculating blocks to catch up in Flow, initialize the counter to 0 and only set it when the latest chain height is greater than the current height (latestOnChainHeight > currentHeight). There's no need to handle the else case as 0 blocks to catch up is the correct default when we're at the same height or ahead.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (11)
services/requester/keystore/key_store.go (11)
4-4: Import additions look good.The new imports (context, access, zerolog) are necessary for the enhanced functionality and properly support the architectural changes.
Also applies to: 9-9, 11-11
24-24: Well-designed struct fields for asynchronous processing.The addition of
client,blockChan,logger, anddonefields properly supports the new asynchronous architecture. The buffered channel size of 100 forblockChanis reasonable for handling block notifications without blocking.Also applies to: 29-34
39-44: Constructor signature change aligns with new requirements.The updated constructor properly accepts the necessary dependencies (context, client, logger) for the enhanced functionality.
65-70: Conditional goroutine launch is well-reasoned.The check for
ks.size > 0before launching theprocessLockedKeysgoroutine is appropriate for index-mode deployments where transaction submission is not allowed. The comment clearly explains the rationale.
80-83: HasKeysInUse method provides useful optimization.This method enables the optimization in
processLockedKeysto skip unnecessary Access Node calls when no keys are in use, which is particularly beneficial during state back-filling operations.
109-118: Robust shutdown handling in NotifyBlock.The implementation properly handles shutdown scenarios by checking the
donechannel and logging warnings if called during shutdown. This prevents potential blocking on theblockChanduring node shutdown.
172-174: Excellent optimization to reduce unnecessary Access Node calls.The
HasKeysInUse()check effectively prevents unnecessary calls to the Access Node when no keys are locked, which is particularly beneficial during back-filling operations.
176-183: Proper error handling and context usage.The implementation correctly uses the provided context instead of
context.Background()and includes appropriate error logging instead of silently ignoring errors. This addresses previous review feedback.
194-197: Safety check prevents potential panics.The empty slice check at the beginning of
releasekeysprevents potential panics when accessingtxResults[0]later in the method. This addresses previous review feedback.
202-211: Transaction status consideration.The current implementation unlocks keys for all transaction results regardless of their status. Based on past review discussion, this approach may unlock keys for transactions that failed in ways where the sequence number didn't increase (e.g., incorrect sequence number, invalid proposer signature).
Please verify that unlocking keys regardless of transaction status aligns with the intended behavior, particularly for failed transactions where the sequence number may not have increased.
207-211: Expiration-based key release maintains backward compatibility.The logic to unlock keys that have been locked beyond the expiration threshold preserves the existing behavior while adding the new transaction result-based unlocking. This ensures keys aren't permanently locked due to processing failures.
| ) | ||
| return | ||
| default: | ||
| k.blockChan <- blockID |
There was a problem hiding this comment.
this also needs a select otherwise it could block forever
| close(k.done) | ||
| return | ||
| case blockID := <-k.blockChan: | ||
| // TODO: If calling `k.client.GetTransactionResultsByBlockID` for each |
There was a problem hiding this comment.
I think it should be OK for each gw to send a single request per block since it's generally low impact. Some of these requests are served by ENs, but it shouldn't be hard to add caching on ANs if it becomes a problem.
this does highlight that we need a tx streaming endpoint.
| close(k.done) | ||
| return | ||
| case blockID := <-k.blockChan: | ||
| // TODO: If calling `k.client.GetTransactionResultsByBlockID` for each |
There was a problem hiding this comment.
how about making these new calls opt-in. that way most GWs would not start polling each block, but the few infra providers that need it could turn it on.
Looking over the implementation, we could probably just skip the lookup if it's not enabled and pass an empty slice
| continue | ||
| } | ||
|
|
||
| txResults, err := k.client.GetTransactionResultsByBlockID(ctx, blockID) |
There was a problem hiding this comment.
Compare to the previous height based approach, this blockID based approach would query AN for each block, wouldn't it be slower than height based approach?
My understanding is that if GW is indexing each block, then most keys would be released by NotifyTransaction method, only those tx that didn't get included in a block will need to be released by NotifyHeight/NotifyBlock. Since GW is indexing each block, why would it need to query AN for the transaction results. Note, AN could also fall behind, which will introduce more edge cases here.
There was a problem hiding this comment.
Compare to the previous height based approach, this blockID based approach would query AN for each block, wouldn't it be slower than height based approach?
Not really, querying ANs for each new Flow block increases the capacity of the available signing keys. And this is done outside the ingestion process, in a dedicated goroutine.
My understanding is that if GW is indexing each block, then most keys would be released by NotifyTransaction method, only those tx that didn't get included in a block will need to be released by NotifyHeight/NotifyBlock. Since GW is indexing each block, why would it need to query AN for the transaction results. Note, AN could also fall behind, which will introduce more edge cases here.
We make use of the Event Streaming API, and we subscribe only to the EVM-related events:
EVM.TransactionExecutedEVM.BlockExecuted
so NotifyTransaction will release the account keys for all valid EVM transactions, because they emit EVM.TransactionExecuted events.
But invalid EVM transactions do not emit EVM.TransactionExecuted events. The same goes for Flow transactions that expired, or had some sequence number issues. These get released after 600 Flow blocks, because they didn't even get included in a Flow block.
| type KeyLock interface { | ||
| NotifyTransaction(txID flowsdk.Identifier) | ||
| NotifyBlock(blockHeight uint64) | ||
| NotifyBlock(blockID flowsdk.Identifier) |
There was a problem hiding this comment.
Can we add comments to these two methods for scenarios they will be used?
And since this PR is to address the swam of invalid transactions (nonce too lower), I understand it takes time for NotifyBlockByHeight to release them, but why can't GW release them through NotifyTransaction? I think those invalid transactions are all included in the blocks, therefore will be seen and indexed by GW locally, no?
There was a problem hiding this comment.
Good idea 👍 I added some descriptive comments in 7f0145f .
I think those invalid transactions are all included in the blocks, therefore will be seen and indexed by GW locally, no?
You are right, they are generally included in Flow blocks. But we use the Event Streaming API in our event ingestion, and we only subscribe to these events:
EVM.TransactionExecutedEVM.BlockExecuted
Invalid EVM transactions do not emit EVM.TransactionExecuted events. That's why we resort to the gRPC API, GetTransactionResultsByBlockID.
There was a problem hiding this comment.
I understand invalid transaction will not trigger EVM.TransactionExecuted events, because they don't change EVM state, it can be ignored.
But isn't it nicer to be able to recognize invalid txs in GW? So that we can take corresponding actions if they are sent from us. And also reduce the load to ANs, and therefore further improve the release frequency.
When we send the tx, we know both the EVM tx ID and flow tx ID. When GW follows the chain, does it know the flow tx ID of each block? If so, we can release key with that. When we query GetTransactionResultsByBlockID, we kinda fetching tx id for each block, since we only take the tx id, and throw away the actual transaction results, which means we technically only need GetTransactionIDsByBlockID, no?
Or is it possible for An to add a new event such as EVM.TransactionRejected events for invalid transactions? Then we don't even need to fetch tx IDs for each block.
There was a problem hiding this comment.
Or is it possible for An to add a new event such as EVM.TransactionRejected events for invalid transactions? Then we don't even need to fetch tx IDs for each block.
Cannot emit event there as flow transaction panics. But maybe transaction can be modified accordingly.
There was a problem hiding this comment.
Yeah, the flow tx panics when nonce is low, so the flow tx is a failed tx, which doesn't emit any event. The question is if possible we can change this flow tx to be succeeded, and emit a EVM.TransactionRejected instead. Technically, the flow tx succeeded, just that its internal EVM tx got rejected (not failed, since we didn't even run this tx).
IMO, changing the flow tx from failed to succeed, and letting EVM to emit EVM.TransactionRejected event would be a better approach. Thoughts?
There was a problem hiding this comment.
We had discussed the possibility of using an EVM event dedicated for this (such as EVM.TransactionRejected), but it has certain drawbacks:
- Events are stored on-chain
- Trading bots are spamming 5-10 such transactions per Flow block, so about 5-10 such EVM events will be emitted per block. And that only reflects the activity as of today.
- Events in general are supposed to give insights about on-chain state changes. In this case, we're introducing an event for a technicality, which doesn't provide any actual insight to devs/users.
In general, I think we're better off using an off-chain approach for the issue at hand.
There was a problem hiding this comment.
I am not sure even if we need to query transactionIDs actually. We query account for each send, can't we just use sequenceID from there.
For example: I locked Index:0, which has seq 42. When I query account if Index:0 has seq 43, it should be unlocked etc.
There was a problem hiding this comment.
In general, I think we're better off using an off-chain approach for the issue at hand.
I agree with this. I'm also worried about including failure events because that would allow EVM users to cause events to be emitted while paying no gas.
There was a problem hiding this comment.
I am not sure even if we need to query transactionIDs actually. We query account for each send, can't we just use sequenceID from there.
For example: I locked Index:0, which has seq 42. When I query account if Index:0 has seq 43, it should be unlocked etc.
The problem is that we can't differentiate between a tx that had an invalid evm transaction and failed from one that was delayed and may eventually get executed. currently, we wait for tx expiration. this new system would allow us to detect failures much quicker.
There was a problem hiding this comment.
@peterargue But shouldn't delayed keep the sequence number but others increase?
Only issue maybe can be some AN falling back somehow I guess. But as seq never decrease shouldn't be that much of a deal breaker.
m-Peter
force-pushed
the
mpeter/improve-signing-keys-release-frequency
branch
from
318b1bb to
87ecd69
Compare
| k.logger.Warn().Msg( | ||
| "received `NotifyBlock` while the server is shutting down", | ||
| ) | ||
| case k.blockChan <- blockHeader: |
There was a problem hiding this comment.
this could block ingestion if processLockedKeys is slow. would it be better to just drop block notifications in that case and rely on the normal height based expirations?
There was a problem hiding this comment.
That was a very neat catch 💯 It's not that processLockedKeys is actually very slow, but in certain cases it can't keep up with the block notifications coming through the event ingestion.
For example, if a EVM GW node is say 10,000 blocks behind, then it will receive quite a lot of EVM events through the Event Streaming API. This will cause quite a lot of block notifications too for the KeyStore. But since k.blockChan only has a capacity of 200 (it was 100 and I doubled it), then naturally it can't keep up with the incoming number of block notifications. This has the unexpected side-effect of slowing down/blocking the event ingestion.
On the other side, if a EVM GW node has caught up, and is only a few blocks behind, I believe the event ingestion will not experience any delay. We'll be receiving events every ~0.8 seconds (block production), and the capacity of k.blockChan should be enough to handle this block notification rate.
I have applied your suggestion in 28a04f2. I added a default case for the existing select, which basically releases keys only through the normal height-based expiration logic.
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
| } | ||
|
|
||
| for txID, key := range k.usedKeys { | ||
| if blockHeight-key.lastLockedBlock.Load() >= accountKeyBlockExpiration { |
There was a problem hiding this comment.
To avoid overflow
| if blockHeight-key.lastLockedBlock.Load() >= accountKeyBlockExpiration { | |
| if blockHeight >= accountKeyBlockExpiration + key.lastLockedBlock.Load() { |
There was a problem hiding this comment.
That's a good point 👍 I'll fix it in a follow-up PR.
|
|
||
| txResults := []*flowsdk.TransactionResult{} | ||
| var err error | ||
| if k.config.COATxLookupEnabled { |
There was a problem hiding this comment.
requesting AN for each block is a heavy, maybe we could it when 50% of the key has not been released? I mean if only 1 key is used, we don't have to query AN, and most likely NotifyTransaction would work, or even if it was used by an invalid tx, we still have 99% keys left. Maybe we only need to enable lookup when 50% keys are used.
So instead of using a bool here, we could use a percentage as usedKeyPercentageThresholdForEnableCOATxLookup, which can cover both case.
There was a problem hiding this comment.
That's a good idea. I used to have a TODO comment for exactly what you just described, just with a different ration. See c738a82#diff-4d0c2f3e8fbdcb80962681f8bf741810b15391ed5d0f2d20163e1f8edbd9a78bL175-L187 .
// TODO: If calling
k.client.GetTransactionResultsByBlockIDfor each
// new block, seems to be problematic for ANs, we can take an approach
// like the one below:
// If the available keys ratio is >= 60% of the total signing keys,
// then we can skip checking the transaction result statuses.
// The signing keys from invalid EVM transactions, will eventually
// come again into availability, afteraccountKeyBlockExpiration
// blocks have passed.
availablekeysRatio := float64(k.AvailableKeys()) / float64(k.size)
if availablekeysRatio >= 0.6 {
continue
}| defer k.keyMu.Unlock() | ||
|
|
||
| for _, txResult := range txResults { | ||
| k.unsafeUnlockKey(txResult.TransactionID) |
There was a problem hiding this comment.
We only care about the TransactionID field of the transaction result.
It would be nice that we can just query GetTransactionIDsByBlockID, as it's much more lightweight. however, there is no such API from AN, but AN does have such storage API .
It's called flow.LightTransactionResult, which only contains transaction ID.
@peterargue is it worth to add this AN API? seems like a good fit for devin AI, no?
But one thing to note is GetTransactionIDsByBlockID might not be able to return a correct order, because it has no collection info. each light transaction result is stored with its Transaction Index, but multiple txs from different collections might have the same tx Index. So we might want to make it explicit as something like GetUnsortedTransactionIDsByBlockID. For our usecase here, unsorted is fine.
Thoughts?
There was a problem hiding this comment.
Just checked LookupTransactionResultsByBlockIDUsingIndex also does not sort transaction results.
There was a problem hiding this comment.
That's a good point. It would be much lighter if we could just fetch the transaction ID. From the current endpoint, most fields should be rather small, except for Events, which in certain cases could be a bit bulky. But since there's no other option, we can just stick with GetTransactionResultsByBlockID for now.
But one thing to note is GetTransactionIDsByBlockID might not be able to return a correct order, because it has no collection info. each light transaction result is stored with its Transaction Index, but multiple txs from different collections might have the same tx Index. So we might want to make it explicit as something like GetUnsortedTransactionIDsByBlockID. For our usecase here, unsorted is fine.
We do not really care for sorting, we simply need the transaction ID for releasing the corresponding account key.
Closes: #841
Description
Introduce a background process on keystore to improve release frequency for signing keys, by checking tx result status on each new block.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Tests