fix: improve responsiveness of wallet base node switching #3488
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
sdbondi
force-pushed
the
wallet-reset-chain-metadata-on-peer-change
branch
2 times, most recently
from
90f4f74
to
f139a9f
Compare
sdbondi
force-pushed
the
wallet-reset-chain-metadata-on-peer-change
branch
from
f139a9f
to
c412884
Compare
sdbondi
added a commit
to sdbondi/tari
that referenced
this pull request
Oct 28, 2021
…ct-chain-metadata * development: (32 commits) test: improve cucumber with wallets (tari-project#3507) feat: optimize pending transactions inbound query (tari-project#3500) test: add trace logs to wallet's base node monitor (tari-project#3502) fix: improve test Wallet should display transactions made (tari-project#3501) test: change timeouts in ci to reasonable values (tari-project#3494) fix: correct panic in tracing for comms (tari-project#3499) feat: optimize get transactions query (tari-project#3496) fix: fix config file whitespace issue when auto generated in windows (tari-project#3491) bump to rerun tests fix: improve responsiveness of wallet base node switching (tari-project#3488) feat: add decay_params method (tari-project#3454) Revert "macos-11" macos-11 clean remove scripts after install install to tmp then use script to copy to home wip wip path wip ...
stringhandler
added a commit
that referenced
this pull request
Nov 3, 2021
* ci: run libwallet daily * chore: update rust toolchain refs * fix: ban peer that advertises higher PoW than able to provide - Can only transition to `HeaderSync` if claimed chain metadata is advertised - `HeaderSync` is now aware of the claimed `ChainMetadata` - `HeaderSync` now assumes (invariant) that all sync peers have claimed a higher accumulated PoW and will ban them if the validated accumulated difficulty does not reach the claimed acc diff. - Adds ban condition in `determine_sync_status` phase, if a peer is not able to improve on the local chain strength (because we know that in order to be selected for header sync it must have advertised a stronger chain) - Adds ban condition if header sync completes but is less than the claimed PoW. This is not strictly necessary since they were still able to provide a stronger chain as per Nakamoto consensus, but could still indicate some malicious intent. - If sync fails for all peers, the state machine continues rather than `WAITING`. This removes the disruption that false metadata can cause. - fix `select_sync_peers` to include peers claim that provide a enough full blocks for _our_ pruning horison (fixes cucumber test) higher than the local pruned * update osx zipper * wip * wip2 * wip3 * wip4 * wip * wip * wip * wip * wip * yolo * wip * path * wip * wip * install to tmp then use script to copy to home * remove scripts after install * Fix missing awaits in cucumber tests * Increase timeouts for tip height waiting * Fix silly mistake on cucumber step * clean * ci: delete versioning action (#3482) Description --- Removes the versioning action, it is no longer required. * macos-11 * Revert "macos-11" This reverts commit 6d9549e. * ci: mark test 'pruned mode reorg past horizon' as flaky * feat: add decay_params method (#3454) Wondering how the emission schedule parameters are derived? This provides a `decay_params` method that calculates the decay array parameters used in EmissionSchedule. The method serves as reference and means of determining the array parameters. * fix: improve responsiveness of wallet base node switching (#3488) Description --- - clear previous base node's state if a new base node is selected - interrupt sleep in bn monitor early if base node has changed - interrupt get_tip_info RPC call early if base node has changed - dont trigger set peer events if same peer is set Motivation and Context --- When the user selects a base node, the UI could appear slow when a sleep is in progress in the base node monitor. During this time, the previous node's latency and height are displayed, which also gives the impression of an unresponsive UI. How Has This Been Tested? --- Manually by switching base nodes * fix after merge * bump to rerun tests * fix: fix config file whitespace issue when auto generated in windows (#3491) Description --- - Added whitespace at the end of each individual config file so that the generated file do not have file end and file beginning overlaps in a single line when generated for Windows. - Re-inserted banners for each section that were removed by a previous PR. Motivation and Context --- The generated `tari_config_example.toml` had overlapping information in joining lines. How Has This Been Tested? --- Generated the combined `tari_config_example.toml` file with the provided `generate_config.bat`. * feat: optimize get transactions query (#3496) Description --- - Optimized the get transactions query (`broadcast_all_completed_transactions`) for transactions that need to be broadcast/rebroadcast by sending a single diesel SQL query that only returns the result, instead of multiple queries that return all the transactions in the database with filtering and selection in the Rust code. - Added a new unit test 'test_get_tranactions_to_be_rebroadcast'. Motivation and Context --- It is much more efficient to have a SQL query perform the filtering upfront. How Has This Been Tested? --- Unit tests, cucumber tests. * refactor: move payload processor to prepare stage * fix: correct panic in tracing for comms (#3499) Description --- Fixes a panic for tracing, Adds additional comments for viewing the Jaeger container after running the image in docker. Motivation and Context --- Without this PR, the following error is encountered when using the `--tracing-enabled` flag ``` thread 'tokio-runtime-worker' panicked at 'Span to follow not found, this is a bug', .cargo\registry\src\github.aaakk.us.kg-1ecc6299db9ec823\tracing-opentelemetry-0.15.0\src\layer.rs:484:14 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace thread 'tokio-runtime-worker' panicked at 'Mutex poisoned: PoisonError { .. }', .cargo\registry\src\github.aaakk.us.kg-1ecc6299db9ec823\tracing-subscriber-0.2.20\src\registry\sharded.rs:400:58 ``` How Has This Been Tested? --- Manually * ci: fix windows installer build github action * test: change timeouts in ci to reasonable values (#3494) Description --- Reduces timeouts to reasonable values. Used max values from reports of successful passes. Motivation and Context --- CI takes up to 4+ hours when issues happen and we have to wait when the full timeout expires. How Has This Been Tested? --- CI pass expected. * fix: improve test Wallet should display transactions made (#3501) Description --- This test can have the wallets fail to talk to each other before sending begins which results in the wallets talking over SAF. This is a much slower communication method which can result in the step `And I send 100000 uT from wallet WALLET_A to wallet WALLET_B at fee 100` failing to reach the broadcast stage. This PR moves the init step of Wallet_B much earlier to increase the changes that the two wallets should communicate directly. * test: add trace logs to wallet's base node monitor (#3502) Description --- - Re-added trace logs to the console wallet's base node monitor to enable profiling of slow responses. - Consistent fixed ms format rather than significant digits for log entries were used to enable easy creation of timing graphs with post-processing of the log files. Motivation and Context --- These trace logs are still needed for the improvement effort of the slow console wallet responses. How Has This Been Tested? --- Ran cargo clippy and cargo format. * messy wip * feat: optimize pending transactions inbound query (#3500) Description --- - Optimized pending inbound transaction query by doing filtering with SQL. - Expanded the unit tests to also test the new query. Motivation and Context --- This is a part of a series of PRs to reduce the memory footprint of the console wallet. How Has This Been Tested? --- Unit tests, wallet cucumber tests * fix: wallet grpc setting * test: improve cucumber with wallets (#3507) Description --- It is beneficial to start all wallets as soon as possible in a cucumber test if the test logic allows it to improve wallet discovery finishing within the set time out limits. Motivation and Context --- Flaky cucumber tests with CI. How Has This Been Tested? --- Running cucumber on CI * feat(wallet_ffi)!: add get_balance callback to wallet ffi (#3475) Description --- - Added get_balance callback to the wallet ffi callback handler that fires only if the balance has actually changed. - Expanded the wallet ffi callback handler test framework to include a mock output manager request-response server. - _**Update:** Added required methods and interfaces to the cucumber test framework._ - _**Update:** Fixed flaky wallet FFI cucumber tests._ - _**Update:** Fixed a bug in the wallet FFI cucumber test framework where the return type of `ref.types.ulonglong` did not correspond to the Rust return type and had a memory alignment problem._ - _**Update:** Fixed an issue whereby on a fast 8-core multi-tasking computer (e.g. AMD Ryzen 7 2700X) some of the wallet FFI cucumber tests did not complete properly after the test and went into an endless wait. The root cause of this issue has been traced down to incorrect use of synchronous calls to wallet FFI destroy methods where in actual fact those methods have async behaviour._ - _**Update:** Re-applied #3271._ ~~The following anomaly exists when compiling the proposed `wallet_ffi/tests` module:~~ ``` 24 | use tari_wallet_ffi::callback_handler::CallbackHandler; | ^^^^^^^^^^^^^^^ use of undeclared crate or module `tari_wallet_ffi` ``` _**Update**_ ~~Various code organizations have been tried, all with the same result. As an alternative, a working test and output manager service mock has been added into the test module in `callback_handler.rs`. Hopefully, the anomaly can be fixed. Duplicate code will be removed before the final commit.~~ Motivation and Context --- - Mobile wallet efficiency. - Resilient wallet FFI cucumber tests. How Has This Been Tested? --- - Expanded the current FFI `test_callback_handler` unit test. - _**Update:** Ran all the wallet FFI cucumber tests to verify the new callback is working properly when using the wallet FFI library:_ ``` 2021-10-21T06:29:32.160Z callbackTransactionValidationComplete(9123501482775375388,0) 2021-10-21T06:29:32.161Z callbackBalanceUpdated: available = 0, time locked = 0 pending incoming = 2000000 pending outgoing = 0 2021-10-21T06:29:32.263Z received Transaction with txID 14659183447022727953 ... 2021-10-21T06:31:38.358Z Transaction with txID 14659183447022727953 was mined. 2021-10-21T06:31:38.358Z callbackBalanceUpdated: available = 2000000, time locked = 2000000 pending incoming = 0 pending outgoing = 0 2021-10-21T06:31:38.359Z callbackTransactionValidationComplete(17755253868227079780,0) ``` * v0.12.0 * v0.12.0 * wip * test: increase limit for cucumber * ci: disable builds on ci * feat: add caching and clippy annotations to CI (#3518) * Adds caching to the standard CI flow; this should drastically reduce build times when pushing small fixes to PRs by 90% or more. * Introduce clippy-check that puts annotations in the PR for identifying clippy warnings. * The motivation for this is that recent changes to clippy make warnings into hard errors, which shouldn't break an entire PR build (e.g. `missing impl Default` warning). * Fixing clippy errors are always "Good first issues", and having annotations in the code makes it clear where first-time contributors need to focus. * test: add logs on non-passing tests (#3516) Description --- Change the test to add logs from `=== 'failed'` to `!== 'passed'`. Motivation and Context --- When a step would timeout, it would not trigger the step to add logs How Has This Been Tested? --- Checking on ci * test: fix stack overflow for noise::larger_writes test 1.57-nightly (#3515) Description --- Remove large slices from the stack in noise tests Motivation and Context --- Possible to have a stack overflow when running tests on new nightly How Has This Been Tested? --- Tests pass without overflow * wip * chore: update toolchain to nightly-2021-09-18 (#3514) This seemingly innoucous update required several changes due to changes in how the compiler interprets dead code. * A few variables were renamed _x where it looked like x might be used in upcoming PRs. YAGNI maybe, but I gave the benefit of the doubt in these cases. * Some crates that had LOADS of dead code (wallet ffi crate I'm looking at you), I just changed the `deny(dead_code)` to `warn(dead_code)` * And in some places, I just nuked the dead code, which tidied up some heavy code in the wallet in particular. Since the tests pass, I presume this is fine and there isn't a weird new compiler bug that is missing where the code is needed. * wip * fixes after compile * refactor: move to dan_core * chore: merge development Co-authored-by: Byron Hambly <[email protected]> Co-authored-by: Stanimal <[email protected]> Co-authored-by: mergequeue[bot] <48659329+mergequeue[bot]@users.noreply.github.com> Co-authored-by: Cayle Sharrock <[email protected]> Co-authored-by: Hansie Odendaal <[email protected]> Co-authored-by: David Main <[email protected]> Co-authored-by: Denis Kolodin <[email protected]> Co-authored-by: SW van Heerden <[email protected]>
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.
Description
Motivation and Context
When the user selects a base node, the UI could appear slow when a sleep is in progress in the base node monitor.
During this time, the previous node's latency and height are displayed, which also gives the impression of an unresponsive UI.
How Has This Been Tested?
Manually by switching base nodes