debug native demo with more sqlite nodes #2545
Closed
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
* Import hs-builder-core (#94) * Workspace * Import legacy builder * Replace flake * Replace build_nix action * Fix Cargo.toml formatting * init push * merge conflict * adjust sleep * add a test should fail * format * clippy * adjust sleep * adjust sleep * rustdoc * random rounds chosing refactor * fork case refactor * clippy * adjust sleep time * adjust sleep time * adjust sleep * adjust sleep * sleep adjust and pending condition * comments for build_block() * Replace version and edition shorthands in crates The `cargo-sort` utility seems to not be able to understand the shorthand assignment for the workspace. Example: This is problematic and causes cargo-sort to error. ``` version.workspace = true ``` This works with cargo-sort ``` version = { workspace = true } ``` This replaces the shorthand with the longer form so that `cargo-sort` can work and not interfere with the nix precommit hook. * Apply cargo-sort to Cargo files * Refactor order tests for deterministic behavior The current order tests can be flaky at times as they rely on waiting for the builder's logic to process via `async_sleep` without being able to determine if its in a state that is ready. To address this specifically the code has been refactored to work with `ProxyGlobalState` instead of the `GlobalState` directly as `ProxyGlobalState` already has some mechanisms for waiting for things to be completed. This also aids in simplifying a bit of the internal logic, as we were reimplementing what `ProxyGlobalState` was already doing. In addition to these changes this also attempts to refactor the code in such a way as to remove the conditionals that occur within the body of the for each loops. This is done by adding an enum for round behavior that can adjust the transactions returned by the bundle call as needed. The order of progression within the for each loop has also been adjusted to perform the consensus portion at the top of the loop before transactions are submitted. Since this order isn't that important, it seems better to have the consensus to occur at the top of the loop rather than after the transactions as it makes it more clear which portion of the logic we are currently in. I.E. are we currently considering consensus' role, or the builder's role. * Add asserts to verify specific fork behavior When a fork happens such that we have separate requests submitted for the builder, we are expecting the builder to be aware of these and to issue any missing transactions for a block in a subsequent block. At the moment, this is not occurring, and the assert at the bottom of the `test_builder_order_chain_fork` that ensures that the two transaction history forks don't match is backwards. Additional asserts have been added when producing the next propsoal to verify that the transaction lists are the same in the event that their previous lists were the same, or differ if their previous lists differ while not having any transactions in the fork, or if the transactions are the same when the previous proposal contained all of the missing transactions. * Fix comment on fork test to match assert * Fix BuilderState unecessary forking The current `BuilderState` extending behavior fails to anticipate enough of the various edge cases that determine whether the current `BuilderState` is the best, or one of the only `BuilderState`s that should be extended. Due to this behavior many more `BuilderState`s are spawned that really should not be spawned. This causes a lot of unecessary computation and memory usage that will never get cleaned up as the `Sender`s that are registered for them get overwritten by subsequent clones that have the same `BuilderStateId`. This effectively causes an async memory leak, async processing leak, and a potential fork-bomb that will effectively prevent the builder from being responsive. Additionally, the race condition that stems from multiple `BuilderState`s spawning a clone that extends them for the next `QuorumProposal` means that we often recieve an unexpected list of transactions as a result. This behavior can be best seen by the test marketplace::testing::order_test::test_builder_order_chain_fork`. To fix these issues a few changes have been maded. First a new method has been added called `spawn_clone_that_extends_self` that ensures a standard way of spawning a new `BuilderState` that extends the current `BuilderState`. Another method has been added called `am_i_the_best_builder_state_to_extend` that attempts to ensure that we are the best fit, or one of the only fits that can extend the current `BuilderState` based on the given `QuorumProposal`. Finally, as a safe guard, a check has been added when calling `spawn_clone` that ensures that the same `BuilderStateId` will not be registered more than once. IE, if there is already an async process spawned, there is no need to spawn another. Remove newly added asserts as they fail to represent the correct anticipated intermediate state * Fix `calc_proposal_msg` `QuorumProposal`'s `justify_qc`'s `view_number` The `calc_proposal_msg` is currently returning the same `view_number` for its justify_qc that it is for the entire `QuorumProposal`. Instead the `justify_qc` view should come from the passed previous proposal's view_number. * Relocate `BuiltFromProposedBlock` to `utils.rs` The `BuiltFromProposedBlock` definitions is going to be referenced in `GlobalState`. In order to prepare for this change the struct needs to be moved to a different crate in order to avoid circular dependencies. * Add `BuiltFromProposedBlock` to `spawned_builder_states` In order to be able to consistently pick the correct `BuilderState` to extend when we receive a `QuorumProposal`, additional information is needed when understanding the spawned `BuilderState`s. In order to add this additional information the `BuiltFromProposedBlock` struct has been added as a `Pair` to the value stored in the `spawned_builder_states`. This addition will allow for inspectors to better understand the `QuorumProposal` information that the `BuilderState` was spawned from. This field is also made an `Option` to make it easier to initialize. * Fix best `BuilderState` algorithm With the addition of the the `BuiltForProposedBlock` to the `spawned_builder_states` we can not make better decisions about which `BlockerState` a given `QuorumProposal` can extend. As such, this splits the determiniation of the best `BuilderState`s to extend from the `am_i_the_best_builder_state_to_extend` call. Additionally the new `best_builder_states_to_extend` function is deliberately made into a function rather than a `method` in order to ensure that information contained within the current `BuilderState` that is only visible to that `BuilderState` is used. This should allow for more consistent determiniations of the proper `BuilderState` to extend. * some loud logging to debug * update module path * Add comment describing the Option value of `spawned_builder_states` Addresses feeback given by @shenkeyao: EspressoSystems/marketplace-builder-core#114 (comment) * tmp test * one sub-test in test_process_da_proposal() * re-sync with main * format * add assert for global_state * Logging + bump limits (#132) * all sub-task finished * format * clean up * test refactor * fmt * more clean up * Rename BuiltFromProposedBlock and update tuple * Add view check * panic when hitting unexpected thing * one more panic * merge conflict * Add unit tests for ProxyGlobalState::available_blocks_implementation Add Error enum variants for ProxyGlobalState::available_blocks_implementation Add Error enum variants for ProxyGlobalState::claim_block_implementation Add Error enum variants for ProxyGlobalState::claim_block_header_input In order to effectively add test cases for ProxyGlobalState::available_blocks, ProxyGlobalState::claim_block, and ProxyGlobalState::claim_block_header_input we need to be able to reach and verify that we have reached the specific edge case / error condition states. In order to do this without breaking the current API contract, new implementation methods were added that return Results that have very specificly defined enum variants to address each of the edge cases. These enum variants have also been configured to be convertable into the original error BuilderError type. Using these new implementation methods, and the specific enum error variants, we can easily reach each edge case, and verify / document this specific error conditions, while still allowing for the original API to also be implemented. * Add tests for ProxyGlobalState::claim_block_header_input Now that we have separated the implementation of `claim_block_header_input` from the API declaration, so that we can have it return the specific error enum variant `ClaimBlockHeaderInputError`, we have the ability to add unit tests to trigger its specific error scenarios. This adds unit tests to cover teh various error cases. Additionally, comments have been added to various structus and methods to help further the understanding of what these do, what they are for, and to draw attention as to whether they need to stay around, or if their purpose needs to be rethought. * Add test for ProxyGlobalState::claim_block_implementation Now that the implementation of the `claim_block` methods has been separated from the API calling, and that specific enum variants have been created for its error cases, unit tests can now be added to target each of the specific error cases. In addition to adding unit tests for `claim_block_implementation`, previously misnamed tests have been renamed to accurately indicate that they are targetting `claim_block_header_input` rather than `claim_block`. * Add unit testes for GlobalState::new These unit tests are fairly basic, but they codeify that the arguments that are being passed into the `new` function of `GlobalState` will setup the `GlobalState` with a specific state. * Add unit tests for GlobalState::register_builder_state These unit tests verify that the state modifications that do happen as a result of invoking `register_builder_state` are expected, and indicate the current behavior. Additionally, they serve a purpose to draw attention to these behaviors in an effort to verify that they are the intended correct behavior, and not some missing edge case. * Add unit tests for GlobalState::update_global_state These unit tests have been added in order to check that the modification of the `GlobalState` object is intended with the invocation of `update_global_state`. Specific unit tests have also been added to verify the behavior of ovewriting previously stored values of hashmap, or not updating the stored value of the LRU in order to verify that these behaviors are explicitly intended. * Add unit tests for GlobalState::remove_handles The `remove_handles` method of `GlobalState` ends up modifying the stored data in `GlobalState` in somewhat interesting ways. In order to ensure that this behavior is correct, and to draw attention to its implementation, unit tests have been added in order to verify and assert that the current behavior is the intended behavior. * Add unit tests for `handle_da_event` The function `handle_da_event` doesn't have any return values, and as such doesn't really have a testing service to check specific failure conditions. In order to add unit tests to `handle_da_event` a new function called `handle_da_event_implementation` has been added which does return a `Result` whose error value is an enum whose values encompass the various failure conditions that can occur. Then the original function `handle_da_event` is made to call this new function, and simply ignore the error. This allows the implementation function to be tested against, while maintaining the behavior and call signature of the original function. These unit tests do not target `handle_da_event` directly, but rather `handle_da_event_implementation`. Each failure case variant, in addition to a single success case, has had a unit test added to codify its behavior and ensure that it behaves correctly, and as expected. * Add unit tests for `handle_da_event` At the moment `handle_da_event` doesn't have a return value which makes it difficult to create unit tests that target specific behavior regarding successes or failures. In order to effectively test this function, an implementation function has been created for it called `handle_da_event_implementation` which does return a new error enum type, `HandleQcEventError` that encodes all of the various failure conditions that apply when attempting to handle the qc event. The code to `handle_qc_event` is then just updated to invoke this `handle_qc_event_implementation` function, and to ignore the error in order to maintain the previous function signature and behavior. In addition to this refactoring, the processing of `handle_qc_event_implementation` has been modified to take the happy path coding paradigm of early returns in the case of errors. With the creation of the `handle_qc_event_implementation` function now in existence the creation of unit tests that target the specific error conditions can now be created in order to trigger this conditions explicitly, as well as the success cases. Unit tests that address these conditions explicitly have been added. * Add unit tests for `handle_received_txns` The function `handle_received_txns` is currently written in such a way that makes it difficult to write unit tests for. In order to remedy this, a new error enum has been created called `HandleReceivedTxnsError` which codifies the different failure reasons that can cause the transactions not to be broadcast to the given receiver. Additionally, since the original return value is a Vec, and in order to prevent a double Vec allocation unnecessarily, this has been implemented as an Iterator to make it collectable with a single allocation. The original function `handle_received_txns` has modified to create this `Iterator` called `HandleReceivedTxns` and to collect it's results into a `Vec` with its errors being mapped back to a `BuildError`. With this modification unit tests can be added to target the `HandleReceivedTxns` `Iterator` implementation directly in order to assert the failure and success cases of the function's processing. * Add unit tests for `connect_to_events_service` In it's current state the function `connect_to_events_service` is not easily testable with reproducible / controlled conditions. Due to this, and the nature of what the function `connect_to_events_service` is meant to be doing, the code has been refactored to introduce a `trait`, `HotShotEventsService`, that can be interacted with that exposes the relevant API endpoints analogs. This `trait` can then be implemented with different implementations that makes it suitable for testing with mocks. An implementation of the trait `HotShotEventsService` has also been provided called `HotShotEventsServiceTideDiscoClient` that implements the `trait` with a tide disco client. This is the path that allows for the original behavior to be maintained. Additionally `connect_to_events_service` has been refactored to return a `Result` instead of an `Option` so that a new enum type, `ConnectToEventsServiceError`, can be returned when the specific failure condition arises. The function `connect_to_events_service` now takes a `HotShotEventsService` client in order to perform the various function calls. All of these changes allow for `connect_to_events_service` to be effectively tested for its edge cases with specifically designed mock implementations that will trigger the specific conditions without the need to actually establish a connection to a static external source. * Remove `ProposedBlockId` and `BuilderStatesInfo` * Remove duplicate "for tracking purposes" comment * Fix typo for "metadata" * Remove capitalization on "trigger" * Fix comment specifying "before after" instead of "after blocks" * Rename `test_handle_received_txns_error_something` to `test_handle_received_txns_error_internal` * Add logging for when `register_builder_state` replaces existing entry * Update `update_global_state` behavior to be consistent with `register_builder_state` * Fix linting errors * Fix `test_global_state_update_global_state_replacement` The test `test_global_state_update_global_state_replacement` is no longer passing due the change of the method's underlying behavior. The method `update_global_state` has been updated such that the LRU gets updated in the same way as the `blocks` `HashMap`. * Add another assert to a test to ensure before prune HashMap size * Remove checks for non existant `BuilderStateId` From a conversation with @shenkeyao: EspressoSystems/marketplace-builder-core#96 (comment) This removes this for loop the checks for the non-existence of `BuilderStateId`s within the `spawned_builder_states` under the idea that it is uncessary due to other asserts for the `HashMap` queries themselves. * Relocate `ParentBlockReferences` fore create dependency The `service.rs` file is going to be updated to match the adjustments and improvements that have been made in the `marketplace_builder_core`. In order to prepare for this appropriately `ParentBlockReferences` needs to be relocated os that it can also be referenced within the `service.rs` file. * Refactor `builder_state.rs` using "Happy Path" methods Adjust `service.rs` so that specific `BuilderState` information can be tracked Apply changes to address unecessary `BuilderState` clones * Update legacy basic_test logic * Reorder `BuilderState` transaction members This change groupst he transaction structures in `BuilderState` together for better association. For instance `tx_queue` should always be kept in sync with `txns_in_queue` but with these parameters being separated in the structure definition it seems to imply that they might have a looser association. Additionally, this renames `txns_in_queue` to `txn_commits_in_queue` to help add clarity for what is being stored, and why. Added some comments on these fields to help expand on what they are for, and why they are grouped together. * Simply hotshot types traits import statements * Refactor `process_da_proposal` method to "Happy Path" The `process_da_propsoal` currently has a few nested `if` statements whose else statements do some basic logic and then exit. These have been modified to move the else conditions further up in the code to favor an early-return rather than an "if true do x, else return" flow. This should help with cognitive load as less conditional context should need to be kept in mind when going into each conditional. Instead we can rule out the cases we don't need to worry about. An additional benefit is that we end up with fewer indentation levels and we avoid indentation / conditional pyramids in our code structure. A comment about what `process_da_propsoal` has also been added to help add context for what this method is actually doing, and the conditions under which it operates. * Refactor `process_quorum_proposal` method to "Happy Path" The `process_quorum_proposal` method currently has a few nested `if` statements that do some basic logic and exit. These have been modified to move the else conditions further up in the code in order to favor an early-return so as to keep the "Happy Path" out of a conditional path. A comment about what `process_quorum_proposal` does has also been added to help add context for what this method is doing, and the conditions under which it operates. * Refactor `process_block_request` to be "Happy Path" The method `process_block_request` currently has a few nested conditionals that evaluate two branches. Each "negative" case of the branch is accompanied by a return without further progress. These nested statements have been flattened with their "negative" case being placed higher. This should allow for less cognitive load to be needed when reading the code and understanding what it does in the future. Additionally a comment has been added to the method to explain what this function is doing and how it returns its response. * Refactor `build_block` to take the "Happy Path" The method `build_block` has a single conditional whose else statement just logs and returns a `None`. This method has been refactored to move that result to the top of method for an early return. This allows the remaining "Happy Path" to be flattened and live outside of the conditional for better clarity of purpose. Additionally a comment has been added to `build_block` to explain its function and the steps taken to produce the result. * Refactor `event_loop` into individual functions for each channel The main purpose of `event_loop` is to process requests as they come in through their channels as soon as possible. However, it is incredibly easy to get lost inside of the `event_loop` itself as it has many different control flows happening in a large function body. This has been refactored so that each receiving channel decision has been split into a separate function whose conditional paths have been converted into a "Happy Path". Each helper method in addition to the `event_loop` itself has had a comment added to it to help describe what it is doing, and in an effort to add clarity for the functionality that is occuring. * Add comments to help annotate specific sections of code with their purpose Add breathing room between some statements for better readability * unit tests for process_quorum_proposal * HotShot API updates rc-0.5.75 - Pin tag hotshot rc-0.5.75 - Pass `num_transactions` to `TestMetadata` - Add a trait for legacy commitment. To be removed when we updated to the new commitment. - Supply `TestVersions` to `QuorumCertificate::genesis` - Run `cargo sort` * Fix conflict resolution errors * Fix fmt * Add issue link * Update comments * test_process_decide_event * some clippy * use generic types * type alias * format * refactor * fmt * larger refactor * Replace qc with quorum * pass Ver: StaticVersionType generic param HotShotEventsServiceTideDiscoClient * lint * Rename remaining `qc` names After the merge of the merge of the Happy Path fixes, more names for `qc` were introduced. This change corrects the remaining ones. * bump hs version to 0.5.77 * Add behavior to allow for a grace period of empty blocks Under normal circumstances the Builder never wants to return an empty block. However, since it takes a few rounds for consensus to finalize transactions submitted to HotShot, it is a beneficial UX to make an effort to finalize any submitted transactions as quickly as possible. To that end, this pull request adds an option to the legacy builder to allow for a grace period of empty block proposal after receiving a message from consensus that indicates that a decide was not missing transactions. When in this grace period, the Builder will propose empty blocks, if no other transactions are able to be submitted, up until the grace period has ended. * Add comment to `TEST_NUM_NODES_IN_VID_COMPUTATION` * Add comment to `TEST_NUM_CONSENSUS_RETRIES` This commitment addresses the discussions from @dailinsubjam: EspressoSystems/marketplace-builder-core#152 (comment) Add a comment to explain what `TEST_NUM_CONSENSUS_RETRIES` governs within the the `finalization_test.rs` file. This explanation explains why the value is being declared and what it is controlling. * Add `TEST_CHANNEL_BUFFER_SIZE` for broadcast channel size Based on discussion with @dailinsubjam: EspressoSystems/marketplace-builder-core#152 (comment) Currently all of the `broadcast` calls are being created with a hard-coded buffer size of `32`. This decision is somewhat arbitrary, but for clarity it is beneficial to replace these hard-coded values with a `const` value that can have a describing comment. The variable `TEST_CHANNEL_BUFFER_SIZE` represents this value with a comment that is provided to add context. * Replace parameter with hard-coded constant Based on discussion with @shenkeyao: EspressoSystems/marketplace-builder-core#152 (comment) We don't expect the number of subsequent blocks that are needed in order to reach finalization to have a loose or changing requirement. As such, it makes more sense to keep the period as a hard-coded constant rather than a configurable value. This change removes the `allow_empty_block_period` parameter, and adds the `ALLOW_EMPTY_BLOCK_PERIOD` constant instead. * tmp push * tmp push * Fix docs & clean up CI (#159) * Fix docs and simplify CI * Add release flag to doctests in CI * compilable but failed version * Extract shared crate (#158) * bug fixed * fmt * clippy * only one testing * only one testing * remove rkyv * format * use TestTypes from hotshot * rename to * remove 'legacy' in test name * more readable code * comment * format * comment * refactor * remove duplicate def of seed and key * `max_block_size` tweaks (#180) * Consistent generic type naming (#181) * [WEEKLY RELEASE] HotShot - rc-0.5.78 (#177) * Integration tests (#148) * version that will fail the test * const test_max_tx_len * update calc of tx len * clippy * upd * fmt * fmt * add one more param * remove param * Save change before sync with main * Fix build * Fix EventServiceStream (#226) * Handle overwrite and add test * Update tags * add the mapping of tx to status * private submit endpoint tx status * fix * upd hotshot * add status sequenced * fmt * add tx_status for marketplace builder * add status pending test * added tests for txn status rejected * test of sequenced tx on test_get_txn_status() added * fmt * clippy * wait more time * fmt * fmt * clean up * clean up * fmt * Remove unused deps (#189) * update hotshot * [Refactor] 1: Add builder state coordinator (#237) * [Refactor] 2: Switch marketplace to coordinator (#238) * solve merge conflict, build successful * clippy * wrap tx_status in Arc<RwLock<LRU>> * fmt * lint * fix recursive call * not clone the whole tx but only tx commit * do not allow txn status change from sequenced/rejected to other status * remove sequenced status * add test for txn marked status twice * more tests to cover txn marked twice * change pending status setting place * remove impl in marketplace * better dealing with unwrap() * lock refactor * refactor all lock relevant to set_txn_status * remove Arc<> wrap on tx_status * downgrade not_implemented to error * allow a rejected tx be changed status * update test * update test * remove unwrap() * update unwrap() * add event period checking * upd comment * try tests with CI * upd comment * upd to use tokio::timeout * upd without tests * clean up comment * upd tests * use RETRY_PERIOD as IDLE_TIMEOUT * add test_ * remove last_event_time * add a line to check whether stream connection returns an error * test test * clippy * restore to correct test * [WEEKLY RELEASE] Bump HotShot (#261) * Bump HotShot * [Refactor] 3: Switch legacy to coordinator (#239) * Move legacy and refactored legacy to separate crates (#271) * Bump maximum allowed period between events (#272) * Update hotshot: 0.5.83 (#284) - Replace DAProposal with DAProposal2 - Add arbirary epoch and EPOCH_HEIGHT values - Set empty (None) next_epoch_justify_qc - Setting `next_drb_result` to `None`. - Remove `drb_seed`, `drb_result`. --------- Co-authored-by: tbro <tbro@users.noreply.github.com> Co-authored-by: ss-es <155648797+ss-es@users.noreply.github.com> * Various fixes for optional epoch and removed vid precompute * bump hotshot dependency hash * keep vid commit calc parallel but remove vid precommit * monorepo -> `marketplace-builder-core` * toolchain -> stable * group builder imports --------- Co-authored-by: Artemii Gerasimovich <artemii@espressosys.com> Co-authored-by: Sishan Long <dlsubjam@gmail.com> Co-authored-by: Theodore Schnepper <tschnepper@espressosys.com> Co-authored-by: Abdul Basit <baronbasit@gmail.com> Co-authored-by: Theodore Schnepper <Ayiga@MSN.com> Co-authored-by: Abdul Basit <45506001+imabdulbasit@users.noreply.github.com> Co-authored-by: Keyao Shen <shenkeyao@gmail.com> Co-authored-by: Mathis Antony <sveitser@gmail.com> Co-authored-by: Luke Iannucci <luke.iannucci@outlook.com> Co-authored-by: ss-es <155648797+ss-es@users.noreply.github.com> Co-authored-by: tbro <tbro@users.noreply.github.com> Co-authored-by: pls148 <184445976+pls148@users.noreply.github.com>
* remove some unused deps * fix audit job * toolchain -> stable * fix lockfile --------- Co-authored-by: Jarred Parr <jparr721@gmail.com>
This PR - adds a flag leaf_only to the FetchingDataSource. This only stores leaves, headers and VID. It does not store/fetch block payloads and supports only relevant endpoints. - supports merklized pruning by deleting all the nodes with created < h and only if it is not the newest node from the fee_merkle_tree state and block_merkle_tree_state - makes query module required - `state`, `catchup` and `status` module are enabled by default for `SQL` data source - includes a test for merklized state pruning in `query-service` crate - test for basic light weight functionality in `sequencer` crate Note: This PR does not remove the file system storage as we weren't able to sucessfully test the sqlite nodes on staging. After we have tested sqlite feature, we can completely remove the file system storage and enable `state`,` catchup` and `status` modules by default for all nodes
imabdulbasit
changed the base branch from
main
to
ab/fix-process-compose-logs
imabdulbasit
changed the title
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.
Closes #<ISSUE_NUMBER>
This PR:
This PR does not:
Key places to review: