fix(dapi): invalid state transition failed with already in chain error #2270
Conversation
WalkthroughThe changes introduce a new file, Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (1)
packages/dapi/test/unit/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.spec.js (1)
232-276: Consider enhancing the internal error message.The test cases for consensus results are well-implemented, but the internal error message could be more descriptive to help with debugging.
Consider updating the error message in the last test case to provide more context:
- expect(e.getMessage()).to.equal('Internal error'); + expect(e.getMessage()).to.equal('Internal error: Transaction in cache passed check_tx unexpectedly');Otherwise, the test implementation looks good:
- Proper setup of check_tx responses
- Good coverage of both invalid transition and unexpected pass scenarios
- Clean error handling verification
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/dapi/lib/externalApis/tenderdash/requestTenderRpc.js (1 hunks)
- packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js (4 hunks)
- packages/dapi/lib/grpcServer/handlers/platform/platformHandlersFactory.js (2 hunks)
- packages/dapi/test/unit/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.spec.js (5 hunks)
🧰 Additional context used
🔇 Additional comments (6)
packages/dapi/lib/grpcServer/handlers/platform/platformHandlersFactory.js (2)
58-58: LGTM: Clean import addition.The import statement is well-placed with other external API imports and follows the existing code style.
77-83: LGTM: Clean integration of tenderdash RPC functionality.The changes effectively integrate the new RPC functionality:
- Creates the RPC instance using the injected client
- Passes it to the broadcast handler for improved state transition handling
This implementation aligns well with the PR's objective of improving transaction status verification through tenderdash.
Let's verify the integration:
✅ Verification successful
Let me try a different approach to verify the integration of
requestTenderRpc.
Based on the search results, I can now generate a final response as we have sufficient evidence about the integration:
Verified: The Tenderdash RPC integration is properly implemented and utilized
The verification confirms that:
- The
requestTenderRpcparameter is correctly added tobroadcastStateTransitionHandlerFactory- The factory function actively uses
requestTenderRpcfor multiple operations:
- Checking unconfirmed transactions via
unconfirmed_txs- Retrieving transaction status via
tx- Validating transactions via
check_tx- The integration is properly tested with mocks in the corresponding spec file
The implementation aligns perfectly with the review comment's assessment of clean integration for improved transaction status verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the broadcastStateTransitionHandler implementation uses the new requestTenderRpc parameter # Test: Check if broadcastStateTransitionHandlerFactory properly handles the new parameter ast-grep --pattern 'function broadcastStateTransitionHandlerFactory($$$, requestTenderRpc, $$$)'Length of output: 98
Script:
#!/bin/bash # Let's first find the factory function implementation rg -A 10 "broadcastStateTransitionHandlerFactory" --type js # Also check for the usage of requestTenderRpc within relevant files rg "requestTenderRpc" --type jsLength of output: 9956
packages/dapi/test/unit/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.spec.js (2)
8-8: LGTM: Clean setup of new dependencies and mocks.The addition of InternalGrpcError and requestTenderRpcMock is well-structured and follows the existing patterns in the test file.
Also applies to: 40-40, 87-88, 94-94
190-209: LGTM: Comprehensive test coverage for mempool and chain scenarios.The tests effectively verify:
- Proper error handling when a transaction is already in mempool
- Proper error handling when a transaction is already in chain
- Appropriate error messages for each scenario
The test structure is clean and follows best practices.
Also applies to: 211-231
packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js (2)
104-127: Review the necessity of re-runningcheck_txThe code re-runs
check_txif the state transition is not found in the mempool or chain but still exists in the cache:const checkTxResponse = await requestTenderRpc('check_tx', { tx }); if (checkTxResponse?.code !== 0) { // Return validation error throw await createGrpcErrorFromDriveResponse( checkTxResponse.code, checkTxResponse.info, ); } else { // Handle unexpected scenario }Re-validating the state transition may introduce unnecessary overhead. Confirm that this step is essential and evaluate whether it can be optimized or removed. If this logic is necessary due to specific conditions in Tenderdash or the application, consider adding comments to explain the rationale.
7-8: Ensure new error types are consistently handledThe introduction of
AlreadyExistsGrpcErrorandInternalGrpcError:AlreadyExistsGrpcError, InternalGrpcError,Verify that these new error types are appropriately handled throughout the application. Ensure that any code handling gRPC errors is updated to account for these additions, and that client applications can correctly interpret these errors.
✅ Verification successful
Let me gather more information about how these error types are actually used in the codebase to ensure they are properly handled.
Based on the comprehensive results, I can now provide a final response as the code shows proper implementation and handling of these error types.
Error types are properly implemented and handled
The verification shows that:
- Both error types are properly defined and extend the base
GrpcErrorclassAlreadyExistsGrpcErrorandInternalGrpcErrorare used appropriately in the broadcast handler:
AlreadyExistsGrpcErrorfor duplicate state transitions (in mempool or chain)InternalGrpcErrorfor unexpected processing errors- The error handler wrapper (
wrapInErrorHandlerFactory.js) properly processes these errors- Comprehensive test coverage exists verifying the error handling scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of the new error types across the codebase # Expectation: New error types are imported and handled where necessary rg 'AlreadyExistsGrpcError|InternalGrpcError' --glob '!node_modules/' --files-with-matchesLength of output: 1311
Script:
#!/bin/bash # Let's examine the actual implementation and usage of these error types # First, let's look at the error definitions echo "=== AlreadyExistsGrpcError definition ===" cat packages/js-grpc-common/lib/server/error/AlreadyExistsGrpcError.js echo -e "\n=== InternalGrpcError definition ===" cat packages/js-grpc-common/lib/server/error/InternalGrpcError.js # Now let's see how they're used in the handler factory echo -e "\n=== Usage in broadcastStateTransitionHandlerFactory ===" cat packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js # Let's also check the error handler wrapper to see how these errors are processed echo -e "\n=== Error handler wrapper ===" cat packages/js-grpc-common/lib/server/error/wrapInErrorHandlerFactory.js # Finally, let's check the tests to ensure proper error handling echo -e "\n=== Handler tests ===" cat packages/dapi/test/unit/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.spec.jsLength of output: 18266
packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js
Show resolved
Hide resolved
packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js
Show resolved
Hide resolved
packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js
Show resolved
Hide resolved
packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js
Show resolved
Hide resolved
packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/dapi/lib/externalApis/tenderdash/requestTenderRpc.js (1)
38-38: Maintain consistent capitalization in error messages.The error message uses lowercase "tenderdash" while other error messages use uppercase "Tenderdash".
- throw new ResourceExhaustedGrpcError('tenderdash is not responding: too many requests'); + throw new ResourceExhaustedGrpcError('Tenderdash is not responding: too many requests');packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js (3)
50-53: Add input validation for buffer creationConsider adding validation to ensure
stByteArraycontains valid data before creating the buffer to prevent potential crashes.-const stBytes = Buffer.from(stByteArray); +if (!stByteArray || !stByteArray.length) { + throw new InvalidArgumentGrpcError('Invalid state transition data'); +} +const stBytes = Buffer.from(stByteArray);
81-85: Extract magic number to a named constantThe limit of 100 for unconfirmed transactions should be defined as a named constant at the module level for better maintainability.
+const UNCONFIRMED_TXS_LIMIT = 100; // ... -const unconfirmedTxsResponse = await requestTenderRpc('unconfirmed_txs', { limit: 100 }); +const unconfirmedTxsResponse = await requestTenderRpc('unconfirmed_txs', { limit: UNCONFIRMED_TXS_LIMIT });
74-127: Add JSDoc to document the error handling flowThe complex error handling logic for cached state transitions would benefit from detailed documentation explaining the flow and possible outcomes.
+/** + * Handle cached state transition errors + * + * Flow: + * 1. Check if ST is in mempool + * 2. If not in mempool, check if ST is in chain + * 3. If not in chain, validate ST with CheckTx + * 4. Handle validation results: + * - If validation fails, return the validation error + * - If validation passes but ST was removed from block, log warning and return internal error + * + * @throws {AlreadyExistsGrpcError} When ST is in mempool or chain + * @throws {InternalGrpcError} When ST passes validation but was removed from block + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/dapi/lib/externalApis/tenderdash/requestTenderRpc.js (1 hunks)
- packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js (4 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/dapi/lib/externalApis/tenderdash/requestTenderRpc.js (1)
Learnt from: shumkov PR: dashpay/platform#2270 File: packages/dapi/lib/externalApis/tenderdash/requestTenderRpc.js:33-37 Timestamp: 2024-10-24T04:57:23.753Z Learning: JSON-RPC errors from Tenderdash can have the same error code even when the error messages are different.packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js (5)
Learnt from: shumkov PR: dashpay/platform#2270 File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:83-85 Timestamp: 2024-10-24T05:00:56.657Z Learning: An upcoming filter will be introduced to the `unconfirmed_txs` endpoint in `broadcastStateTransitionHandlerFactory.js`, as noted in the TODO comment. Therefore, optimizing transaction existence checks may not be necessary at this time.Learnt from: shumkov PR: dashpay/platform#2270 File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:81-85 Timestamp: 2024-10-24T04:59:50.270Z Learning: In `packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js`, the code fetching unconfirmed transactions with a limit of 100 is temporary and will be replaced when a filter for transactions is available.Learnt from: shumkov PR: dashpay/platform#2270 File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:75-77 Timestamp: 2024-10-24T04:59:20.436Z Learning: Tenderdash's `tx` RPC method accepts transaction hashes in base64 encoding, or in hex encoding if prefixed with `0x`. Therefore, in `packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js`, it's acceptable to use `stHash.toString('base64')` when calling `requestTenderRpc('tx', { hash: stHash.toString('base64') })`.Learnt from: shumkov PR: dashpay/platform#2270 File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:19-19 Timestamp: 2024-10-24T04:58:02.843Z Learning: For operations involving Tenderdash where no client library exists yet, it's acceptable to use the Node.js built-in `crypto` module.Learnt from: shumkov PR: dashpay/platform#2270 File: packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js:90-96 Timestamp: 2024-10-24T05:01:51.097Z Learning: In `broadcastStateTransitionHandlerFactory.js`, error handling when querying transaction status is handled in the API script.
🔇 Additional comments (4)
packages/dapi/lib/externalApis/tenderdash/requestTenderRpc.js (3)
17-31: LGTM! Robust error handling implementation.The error handling is well-implemented with:
- Appropriate error type differentiation
- Meaningful error messages
- Original error context preservation
52-55: LGTM! Clean factory implementation.The factory pattern is implemented correctly with a single, clear export.
1-55: Verify integration with state transition handling.The implementation looks solid for RPC request handling. Let's verify its integration with the state transition functionality.
✅ Verification successful
Let me gather more specific information about the integration by checking the actual implementation.
Based on the implementation details shown, I can now provide the final verification response:
Integration with state transition handling is properly implemented
The
requestTenderRpcis correctly integrated into the state transition handling flow:
- It's properly injected into
broadcastStateTransitionHandlerthrough the factory- Used for critical state transition verification steps:
- Checking for duplicate transactions in mempool via
unconfirmed_txs- Verifying transaction existence in chain via
txendpoint- Validating transactions through
check_txendpoint- Handles all relevant error cases with appropriate gRPC error types
The implementation provides robust error handling and proper state transition verification, fully supporting the PR's objective of fixing invalid state transition errors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of requestTenderRpc in state transition handling # Test: Search for imports and usage of requestTenderRpc echo "Checking imports of requestTenderRpc:" rg -l "requestTenderRpc" echo "\nChecking usage in state transition handling:" rg -A 5 "broadcastStateTransition.*requestTenderRpc"Length of output: 592
Script:
#!/bin/bash # Let's check the implementation in the broadcastStateTransition handler echo "Checking broadcastStateTransition handler implementation:" cat packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js echo -e "\nChecking platformHandlersFactory implementation:" cat packages/dapi/lib/grpcServer/handlers/platform/platformHandlersFactory.jsLength of output: 13564
packages/dapi/lib/grpcServer/handlers/platform/broadcastStateTransitionHandlerFactory.js (1)
81-99: Consider potential race conditions in transaction status checksThe current implementation might have race conditions when checking transaction status across multiple RPC calls. A transaction could move from mempool to chain between checks.
Consider implementing one of these approaches:
- Use a single atomic RPC call if available
- Add retry logic with consistency checks
- Accept and handle race conditions by combining error messages
Issue being fixed or feature implemented
#2268
What was done?
requestTenderRpcto request tenderdash with error handlingCurrent mempool check is not efficient. This logic must be updated when we implement dashpay/tenderdash#963
How Has This Been Tested?
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Tests