Refactor pending blocks handling #1568
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Warning Rate Limit Exceeded@tzdybal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 42 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThis update introduces significant improvements to the handling of pending blocks and metadata management within a blockchain system. By enhancing the atomicity and persistence of block submissions and metadata storage, the system now ensures better data integrity and resilience against node restarts or crashes. The changes span across block management, testing enhancements, and store functionalities, aiming to optimize synchronization and submission processes to the Data Aggregator (DA). Changes
Assessment against linked issues
Related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
tzdybal
force-pushed
the
tzdybal/fix_pending_blocks
branch
3 times, most recently
from
ab5704e
to
a8e4179
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (11)
- Makefile (1 hunks)
- block/manager.go (4 hunks)
- block/manager_test.go (3 hunks)
- block/pending_blocks.go (1 hunks)
- block/pending_blocks_test.go (1 hunks)
- node/full_node_integration_test.go (2 hunks)
- node/full_node_test.go (2 hunks)
- store/store.go (3 hunks)
- store/store_test.go (2 hunks)
- store/types.go (1 hunks)
- test/mocks/DA.go (1 hunks)
Files skipped from review due to trivial changes (1)
- test/mocks/DA.go
Additional comments: 13
block/pending_blocks_test.go (1)
- 4-90: The modifications in the test file, including the introduction of context handling, the use of new
storemethods, and the addition of thenewPendingBlocksfunction, significantly improve the robustness and maintainability of the tests. These changes demonstrate good testing practices and effective use of the newly introduced store functionality.The use of context ensures that the tests are more reliable and can be managed more effectively, especially in asynchronous operations or those that might require cancellation or timeouts. The
newPendingBlocksfunction simplifies the setup process for creatingPendingBlocksinstances, making the test code cleaner and easier to understand.Overall, these changes enhance the quality and maintainability of the tests, aligning well with the PR's objectives of improving system reliability and safety.
Makefile (1)
- 86-86: The addition of a new mock generation command for the
DAinterface in the Makefile is a valuable enhancement, supporting the PR's objectives related to testing enhancements. This change facilitates the generation of mocks for theDAinterface, which is essential for thorough and effective testing of DA interactions.The use of
mockeryfor generating mocks is consistent with common practices in Go projects, and the command syntax aligns well with the established patterns in the Makefile. This addition underscores the importance of robust testing infrastructure in ensuring the reliability and safety of the system.block/pending_blocks.go (1)
- 4-111: The extensive changes in
block/pending_blocks.go, including the introduction of new imports, theLastSubmittedHeightKeyconstant, updates to thePendingBlocksstruct, and significant modifications to methods, significantly enhance the handling and persistence of pending blocks. These changes align well with the PR's objectives of improving system reliability and safety, especially in scenarios involving node restarts or unexpected failures.The use of context handling and error management in methods like
getPendingBlocksimproves the robustness and reliability of the pending blocks handling. The introduction of theLastSubmittedHeightKeyconstant for persisting the last submitted height is a clear and maintainable approach. Modifications to methods such assetLastSubmittedHeightandloadFromStoredemonstrate careful consideration of atomicity and error handling, which are crucial for the reliability of the system.These changes enhance the reliability, maintainability, and robustness of the pending blocks handling, making the system more resilient to failures and restarts. Minor refinements could be made for clarity and consistency, but overall, the modifications are well-conceived and effectively implemented.
store/store_test.go (1)
- 210-244: The addition of the
TestMetadatafunction instore/store_test.gois a valuable enhancement, thoroughly testing the new metadata operations (SetMetadataandGetMetadata) added to the store. This test verifies the functionality of setting and retrieving arbitrary metadata using an in-memory key-value store, ensuring that the metadata operations work as expected.The test is well-structured and covers basic scenarios, including setting and retrieving metadata and handling non-existent metadata keys. This coverage is crucial for verifying the correct behavior and error handling of the metadata operations.
The addition of this test enhances the robustness of the store's testing suite, ensuring that the new functionality is correctly implemented and behaves as expected. It might be beneficial to consider adding more test cases if there are specific edge cases or error conditions that are not covered by the current tests.
block/manager_test.go (1)
- 190-229: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [133-229]
The modifications in
block/manager_test.go, including the addition of thegetTempKVStorefunction and enhancements to theTestSubmitBlocksToDAfunction, significantly improve the testing of block submission to the DA layer. These changes address the need for temporary on-disk key-value stores in testing and demonstrate a comprehensive approach to verifying the functionality and reliability of block submission.The
getTempKVStorefunction is a practical solution for creating temporary on-disk stores, effectively addressing the limitations of the in-memory KV store for certain test scenarios. The modifications to theTestSubmitBlocksToDAfunction, including the handling of temporary on-disk stores, saving blocks, updating height, and checking metadata, enhance the test coverage and robustness.These changes contribute to the overall quality and reliability of the system by ensuring that block submission to the DA layer is thoroughly tested. It's important to ensure that temporary on-disk stores are properly cleaned up after tests to avoid potential resource leaks.
store/store.go (2)
- 25-25: The addition of the
metaPrefixconstant is a straightforward and necessary change for distinguishing metadata keys from other data types in the store. This is a good practice for namespace separation in key-value stores.- 256-258: The
getMetaKeyfunction correctly generates keys for metadata by combining themetaPrefixwith the user-provided key. This encapsulation of key generation logic is a good practice for maintainability and consistency.node/full_node_test.go (2)
- 6-20: The addition of new imports, including
crypto/sha256,errors,os,strconv, and others, is necessary for the new test functionality introduced in this file. These imports are correctly placed and follow Go's convention of grouping standard library imports separately from third-party packages.- 253-291: The modifications to the
createAggregatorWithPersistencefunction to include additional parameters and setup logic for theTestPendingBlockstest are appropriate. This function now supports creating a node with a persistent database and a configurable DA client, which is crucial for testing the persistence and submission behavior. Ensure that the error handling within this function is robust, especially around resource creation and initialization steps, to prevent resource leaks or unclear test failures.node/full_node_integration_test.go (2)
- 512-520: The addition of the
getMockApplicationfunction centralizes the creation of a mock application, which is a good practice for reducing code duplication and improving readability. This change effectively consolidates the initialization logic for the mock application used in various tests, making the test setup more maintainable and easier to understand.- 627-627: The replacement of direct mock application creation with the
getMockApplicationfunction call in thecreateNodefunction is a positive change. It ensures consistency in how mock applications are initialized across different tests and reduces the potential for errors or inconsistencies in the mock setup. This change enhances the maintainability and readability of the test code.block/manager.go (2)
- 214-214: The addition of
storeandloggerparameters toNewPendingBlocksin theNewManagerfunction is a positive change. It ensures that thePendingBlocksinstance has access to necessary resources for persistence and logging, aligning with the PR's objectives to enhance reliability and safety in block submission.- 854-858: Setting the last submitted height after successfully submitting blocks to the DA layer is a crucial step for maintaining the integrity of the submission process. This change ensures that the system can accurately track the progress of block submissions, which is essential for resuming operations after a restart or failure. The logic to determine the last submitted height based on the last block in the
submittedBlocksslice is correct and efficiently implemented.
tzdybal
force-pushed
the
tzdybal/fix_pending_blocks
branch
from
0147381
to
78883cd
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (11)
- Makefile (1 hunks)
- block/manager.go (4 hunks)
- block/manager_test.go (4 hunks)
- block/pending_blocks.go (1 hunks)
- block/pending_blocks_test.go (1 hunks)
- node/full_node_integration_test.go (2 hunks)
- node/full_node_test.go (2 hunks)
- store/store.go (3 hunks)
- store/store_test.go (2 hunks)
- store/types.go (1 hunks)
- test/mocks/DA.go (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- Makefile
- block/manager_test.go
- node/full_node_integration_test.go
- store/store.go
- store/store_test.go
- store/types.go
- test/mocks/DA.go
Additional comments: 20
block/pending_blocks_test.go (7)
- 4-11: The addition of imports for
context,test, andstorepackages aligns with the changes made in the test functions to use context and the newstorefunctionality. This ensures that the tests are up-to-date with the latest codebase changes.- 19-25: The test
TestGetPendingBlocksnow initializes a context and uses it in calls toSaveBlockandSetHeight. This is a good practice as it allows for better control over these operations, especially in a testing environment where you might want to simulate cancellations or timeouts.- 33-40: In
TestRemoveSubmittedBlocks, the use of context and thenewPendingBlockshelper function to create aPendingBlocksinstance is consistent with the changes in other test functions. This consistency is important for maintainability and readability of the test code.- 46-58: The test
TestRemoveSubsetOfBlockscorrectly uses context and thenewPendingBlockshelper. Additionally, it tests the removal of a subset of blocks, verifying the remaining blocks' heights. This test is crucial for ensuring the correct behavior of thePendingBlocksfunctionality when only a subset of blocks needs to be removed.- 64-73:
TestRemoveAllBlocksAndVerifyEmptychecks the behavior of removing all blocks and verifying that thePendingBlocksinstance is empty afterward. The use of context and thenewPendingBlockshelper function here is appropriate and follows the pattern established in other tests.- 79-82: The test
TestRemoveBlocksFromEmptyPendingBlocksensures that attempting to remove blocks from an already emptyPendingBlocksinstance does not cause a panic. This is an important edge case to test for, ensuring the robustness of thePendingBlocksimplementation.- 86-90: The
newPendingBlockshelper function is a significant addition, simplifying the creation of aPendingBlocksinstance with a store and logger for testing purposes. This function improves the readability and maintainability of the test code by abstracting the setup process.block/pending_blocks.go (8)
- 4-13: The addition of imports for
context,errors,fmt,strconv,atomic, and various packages from the project and third-party libraries is necessary to support the updated functionality inPendingBlocks. These imports are correctly placed and used throughout the file.- 17-31: The introduction of the
LastSubmittedHeightKeyconstant for persisting the last submitted height is a good practice. It avoids hardcoding the key in multiple places, making the code more maintainable and less error-prone.- 33-37: The update to the
PendingBlocksstruct to include astore,logger, andlastSubmittedHeightusing theatomic.Uint64type is crucial for thread-safe operations and better logging. This change supports the objectives of making the block submission process more reliable and safe.- 41-44: The
NewPendingBlocksconstructor now correctly initializes aPendingBlocksinstance with the provided store and logger. This change is necessary for the refactored implementation that relies on these components.- 50-78: The
getPendingBlocksmethod has been significantly refactored to use context and handle errors more gracefully. The use of atomic operations for reading thelastSubmittedHeightand the detailed error handling improve the method's robustness and reliability.- 82-82: The
isEmptymethod's update to compare the store's height with thelastSubmittedHeightusing atomic operations ensures thread safety and correctness in determining if there are any pending blocks.- 85-93: The
setLastSubmittedHeightmethod's refactoring to use atomic compare-and-swap operations and to persist the new height in the store is a significant improvement. It ensures thread safety and data persistence, aligning with the PR's objectives to enhance reliability.- 96-111: The
loadFromStoremethod for loading the last submitted height from the store is a new addition that supports the persistence objectives of this PR. It handles errors appropriately and uses atomic operations, ensuring the method's reliability and safety.node/full_node_test.go (2)
- 6-20: The addition of imports for
crypto/sha256,errors,os,strconv, and various packages from the project and third-party libraries supports the new test functionality introduced in this file. These imports are correctly placed and used throughout the file.- 256-294: The
createAggregatorWithPersistencefunction has been updated to include additional parameters and setup logic for the test. This update is necessary to simulate the DA submission behavior accurately. The function is well-structured and correctly sets up the necessary components for the test environment.block/manager.go (3)
- 222-222: The addition of
storeandloggerparameters toNewPendingBlocksin theNewManagerfunction aligns with the PR's objective to enhance the reliability and safety of block submission. This change enablesPendingBlocksto interact with persistent storage and logging, which is crucial for managing the state of pending blocks, especially across node restarts. This modification supports the persistence ofpendingBlocksdata, addressing one of the key objectives of the PR.- 840-846: The error handling logic in
submitBlocksToDAhas been updated to address a previously identified issue where errors fromgetPendingBlockscould potentially lead to unintended behavior. The revised logic now correctly handles scenarios wheregetPendingBlocksreturns an empty slice of blocks to submit along with an error. By checking for an error before processing the length ofblocksToSubmit, this change ensures that errors are appropriately logged and the function returns early in error scenarios, preventing further execution. This improvement aligns with the PR's objective to ensure a more robust implementation and safer block submission process.- 869-873: The logic for setting the last submitted height after successfully submitting blocks to the DA layer has been updated. This change is crucial for maintaining the integrity of the block submission process, ensuring that the system accurately tracks the height of the last block submitted to the DA layer. By updating the last submitted height only after successful submission, this modification helps prevent potential issues related to block re-submission or data loss, especially in scenarios where node restarts or failures occur. This aligns with the PR's objectives of enhancing reliability and prioritizing data integrity.
Manav-Aggarwal
requested changes
Mar 1, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- block/manager.go (4 hunks)
Additional comments: 2
block/manager.go (2)
- 222-222: The addition of
storeandloggerparameters toNewPendingBlocksin theNewManagerfunction aligns with the PR's objective of enhancing the reliability and safety of block submission. This change enables thePendingBlocksstructure to interact with persistent storage and logging, which is crucial for the persistence and robustness improvements mentioned.- 876-880: Setting the last submitted height after successful submission of blocks to the DA layer is a critical step for ensuring the system can track progress and potentially resume operations after interruptions. This change supports the PR's goal of improving the reliability of block submission by making the process more resilient to failures and restarts. It's important to ensure that the
setLastSubmittedHeightfunction correctly handles all edge cases, especially regarding the persistence of this state.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- block/manager.go (5 hunks)
- block/manager_test.go (4 hunks)
- block/pending_blocks.go (1 hunks)
- block/pending_blocks_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- block/manager.go
- block/manager_test.go
Additional comments: 11
block/pending_blocks_test.go (6)
- 4-11: Imports have been correctly updated to include necessary packages for the context and the new store functionality. This aligns with the PR objectives of enhancing reliability and safety in block submission by utilizing context for operations and integrating with a store for persistence.
- 33-40: The test
TestRemoveSubmittedBlockscorrectly sets up a context and uses thenewPendingBlocksfunction. It tests the removal of submitted blocks and checks if thePendingBlocksinstance is empty afterward. This test aligns with the PR's objective of ensuring data integrity and robust handling of block submissions.- 46-58: In
TestRemoveSubsetOfBlocks, the setup and execution are correct, and it properly tests the removal of a subset of blocks. The test ensures that only the expected blocks remain, which is crucial for the integrity of block handling.- 64-73:
TestRemoveAllBlocksAndVerifyEmptycorrectly tests the removal of all blocks and verifies that thePendingBlocksinstance is empty afterward. This test is essential for ensuring the system can correctly handle scenarios where all pending blocks need to be removed, such as after successful submissions.- 79-82:
TestRemoveBlocksFromEmptyPendingBlockstests the system's behavior when attempting to remove blocks from an already emptyPendingBlocksinstance. This test ensures that such an operation does not cause a panic, aligning with the goal of robust and safe error handling.- 86-92: The
newPendingBlocksfunction correctly sets up a newPendingBlocksinstance for testing, utilizing an in-memory key-value store and a test logger. This function is a good practice for reducing boilerplate code in tests and ensuring consistent test setup.block/pending_blocks.go (5)
- 4-13: The updated imports correctly include necessary packages for context handling, error management, atomic operations, and logging. This aligns with the PR's objectives of enhancing the reliability and safety of block submission by utilizing context for operations and atomic operations for thread-safe updates.
- 17-31: The documentation for the
PendingBlocksstruct has been updated to reflect the new design and assumptions. It clearly outlines the importance of safe storage, ordered submission, and atomicity in DA submission. The TODO comment about not pushing all pending blocks at once indicates future work for optimizing block submission, which is a good practice for maintaining focus on the current PR objectives while planning for future improvements.- 41-49: The
NewPendingBlocksconstructor has been correctly updated to accept a store and logger, aligning with the PR's objectives of utilizing a store for persistence and logging for debugging. The inclusion of theinitmethod call ensures that thePendingBlocksinstance is properly initialized with the last submitted height from the store.- 79-79: The
isEmptymethod correctly compares the store height with the last submitted height to determine if there are any pending blocks. This method is essential for ensuring the correct handling of block submission states.- 93-108: The
initmethod correctly loads the last submitted height from the store, handling the case where the key is not found as a special non-error case. This method is crucial for ensuring thePendingBlocksinstance is correctly initialized with persistent state information.
TestPendingBlocks should fail to proof the existence of the bug. This commit introduces a new test to check pending blocks as described in issue #1548. It mimics the behavior of a node producing blocks, stopping and restarting, then producing more blocks.
Pending blocks are retrieved directly from store. Next step is to save latest submitted block height to store (store interface needs to be modified). Tests are expected to fail.
Two new methods have been added to the store interface: SetMetadata and GetMetadata. These methods allow for the storage and retrieval of arbitrary data associated with a specified key. Demonstration of usage and functionality is provided through additional tests in store_test.go.
tests are improved to reflect added assertions (block submission always in order) and code changes (in memory kv couldn't handle big values O_o)
As PendingBlocks depend on store, it is safe to remove addPendingBlock function - all tests were updated to save blocks to store instead of calling addPendingBlock.
setLastSubmittedHeight now accepts only height, and the name better reflects it's
The "TestPendingBlocks" test function was modified to provide more specific testing for block submissions. The test now verifies that blocks from the first and second runs are submitted in order and that this information is persisted in the store. Additionally, the test scenario has been detailed for clarity.
functionally they are the same, but unified into neat tabular test
tzdybal
force-pushed
the
tzdybal/fix_pending_blocks
branch
from
0aee217
to
7b52fa5
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- block/pending_blocks_test.go (1 hunks)
Additional comments: 2
block/pending_blocks_test.go (2)
- 4-4: The addition of
context,store, andtest/logimports aligns well with the PR's objectives to enhance the handling of pending blocks and improve testing. These imports are necessary for the new testing logic and the handling of pending blocks, providing context management, storage interaction, and logging capabilities.Also applies to: 11-12
- 21-102: The modifications to the test functions and the introduction of the
newPendingBlocksfunction are well-aligned with the PR's objectives to enhance the handling of pending blocks. Utilizing context in tests is a best practice, and thenewPendingBlocksfunction simplifies setup, making tests cleaner and more maintainable. However, it would be beneficial to add more detailed comments explaining the purpose and expected outcomes of each test case for better maintainability and understanding.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (11)
- Makefile (1 hunks)
- block/manager.go (5 hunks)
- block/manager_test.go (4 hunks)
- block/pending_blocks.go (1 hunks)
- block/pending_blocks_test.go (1 hunks)
- node/full_node_integration_test.go (2 hunks)
- node/full_node_test.go (2 hunks)
- store/store.go (3 hunks)
- store/store_test.go (2 hunks)
- store/types.go (1 hunks)
- test/mocks/DA.go (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- Makefile
- block/manager.go
- block/manager_test.go
- node/full_node_integration_test.go
- store/store.go
- store/store_test.go
- store/types.go
- test/mocks/DA.go
Additional comments: 15
block/pending_blocks_test.go (6)
- 4-4: The addition of the
contextpackage is appropriate given the modifications in the test functions to include context as a parameter. This aligns with best practices for handling cancellations, timeouts, and passing request-scoped values.- 11-12: The addition of imports for the
storeandtestpackages is necessary for the new test setup, where astoreis used for persistence andtestfor logging. This change supports the enhancements made to the testing ofPendingBlocks.- 21-76: The refactoring of test cases to use context and the new
newPendingBlockshelper function improves the readability and maintainability of the tests. By passing context, the tests now support cancellation and timeouts, which is a good practice. ThenewPendingBlocksfunction encapsulates the creation ofPendingBlocksinstances, making the test setup cleaner and more consistent.- 80-85: The
newPendingBlocksfunction is a helpful addition that simplifies the creation ofPendingBlocksinstances for testing. It ensures that each test starts with a fresh instance, reducing the risk of tests affecting each other. This function also abstracts away the setup details, making the tests easier to read and maintain.- 88-102: The
fillWithBlocksfunction correctly simulates the addition of blocks to thePendingBlocksinstance. It uses thestore.SaveBlockmethod to persist blocks and updates the store's height accordingly. This setup is crucial for testing the behavior ofPendingBlocksunder different conditions, such as having a varying number of pending blocks.- 95-102: The
checkRequirementsfunction is well-designed to verify the state ofPendingBlocksafter test execution. It checks if the number of pending blocks matches expectations and ensures that the blocks are sorted by height. This function is an essential part of the test suite, as it validates the core functionality ofPendingBlocks.block/pending_blocks.go (6)
- 4-8: The addition of the
context,errors,fmt,strconv, andsync/atomicpackages supports the new functionality introduced in this file. These imports are necessary for handling context, error formatting, string conversion, and atomic operations, respectively.- 10-13: The addition of imports for the
datastore,store, andlogpackages is necessary for the new implementation ofPendingBlocks. These changes support data persistence, logging, and interaction with the underlying datastore.- 17-31: The introduction of the
LastSubmittedHeightKeyconstant and the updated documentation for thePendingBlocksstruct provide clarity on the purpose and behavior of the pending blocks management system. The documentation outlines important assertions and scenarios, which is beneficial for maintainability and understanding the system's design.- 33-37: The update to the
PendingBlocksstruct to includestoreandloggerfields, along with thelastSubmittedHeightatomic variable, is a significant improvement. These changes enable better data persistence, logging, and thread-safe operations, aligning with the objectives of enhancing the handling of pending blocks.- 41-49: The
NewPendingBlocksconstructor function has been updated to acceptstoreandloggerparameters, which is a positive change. It ensures that instances ofPendingBlocksare initialized with the necessary dependencies for data persistence and logging. The error handling in the initialization process is also a good practice.- 93-108: The
initmethod is a crucial addition for loading the last submitted height from the store during initialization. This method ensures that thePendingBlocksinstance is correctly synchronized with the persisted state, which is essential for the reliability of the system. The error handling and special case handling fords.ErrNotFoundare well-implemented.node/full_node_test.go (3)
- 6-7: The addition of imports for
crypto/sha256anderrorsis necessary for the new testTestPendingBlocks, which involves hashing and error handling. These changes support the test's objectives of simulating DA submission behavior.- 9-10: The addition of imports for
osandstrconvis required for handling file operations and string conversion in the new test setup. These changes are relevant for creating temporary directories and parsing numeric values from strings.- 14-20: The addition of imports for various packages, including
cmconfig,proxy,goDA, andtest, supports the enhanced test setup and the newTestPendingBlocksfunction. These changes enable configuration management, proxy creation, interaction with a mock DA, and logging within the test environment.
Add context as parameter and add comment on error handling.
Manav-Aggarwal
approved these changes
Mar 6, 2024
MSevey
approved these changes
Mar 6, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR creates a new test to check pending blocks as described in issue #1548. It mimics the behavior of a node producing blocks, stopping and restarting, then producing more blocks.
Looking at commit history you can check that test was initially failing and then was fixed.
New implementation of
pendingBlocksis safer because its data is persisted in store. If node is restarted, block submission to DA will restart when it ended. The worst case scenario (very unlikely), wherependingBlockscan't save information to store, results in resubmission of blocks already submitted to DA, which is only the extra cost. The solution ensures that all the blocks are successfully submitted to DA.This PR is bigger than I expected, and because of that I will work on #1524 and limiting the number of blocks returned by
getPendingBlocksin follow-up PR.Resolves #1548
Resolves #457
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores