feat: improved base node monitoring #5390

stringhandler · 2023-05-17T16:06:48Z

Add the ability to run tokio_console on the console_wallet.

I found one case where the UI would use a lot of CPU, but I couldn't find the cause.

Reduced the UI caching time to be more responsive.

Renamed base_node_monitor_refresh_interval to base_node_monitor_max_refresh_interval and implemented a backoff strategy.
If a new block is found when querying the base node, immediately query it again, backing off each time it has the same block. This allows you to update quickly if the node is syncing. Otherwise, backoff to the max interval. The default is 90 seconds.
(was previously 3 seconds)

Changed output and transaction manager service to rely on the base node service to send them an event when a new block is encountered. Both were doing this check themselves in addition to the check the base node service was doing.

The base node service also only checked on the height, now the hash is also checked.

github-actions · 2023-05-17T16:24:21Z

Test Results (CI)

1 147 tests ±0 1 147 ✔️ ±0 9m 58s ⏱️ - 5m 31s
    37 suites ±0       0 💤 ±0
      1 files ±0       0 ❌ ±0

Results for commit dd3ee20. ± Comparison against base commit 8631bc2.

♻️ This comment has been updated with latest results.

github-actions · 2023-05-17T16:24:45Z

Test Results (Integration tests)

  2 files ±0 12 suites ±0 16m 11s ⏱️ - 9m 4s
29 tests ±0 29 ✔️ +1 0 💤 ±0 0 ❌ - 1
29 runs - 1 29 ✔️ ±0 0 💤 ±0 0 ❌ - 1

Results for commit dd3ee20. ± Comparison against base commit 8631bc2.

♻️ This comment has been updated with latest results.

SWvheerden

Are we fine with not triggering on node sync and only when it received a block via propagation?
But apart from that it looks good

SWvheerden · 2023-05-23T15:04:55Z

base_layer/wallet/src/output_manager_service/service.rs

-                    });
-                }
-                self.last_seen_tip_height = state.chain_metadata.map(|cm| cm.height_of_longest_chain());
+            BaseNodeEvent::BaseNodeStateChanged(_state) => {


This will stop the wallet from refreshing on node sync done, which might not be ideal

This state is published when only the latency changes, so it was triggering much more frequently than needed

See related comment ^^. I think we should add a ConnectedBaseNodeChanged event so we can refresh on that specifically.

Resolved with comment below

SWvheerden · 2023-05-23T15:05:35Z

base_layer/wallet/src/transaction_service/service.rs

-                    (None, _) => true,
-                    _ => false,
-                };
+            BaseNodeEvent::BaseNodeStateChanged(_state) => {


This will trigger the first time it gets a block from the node, so I think that makes sense

See related comment ^^.

Resolved with comment below

…r-ffi-tests

hansieodendaal

Overall this is a good change. Please see the comments noted below, and maybe the PR title should be feat: improved base node monitoring.

base_layer/wallet/src/base_node_service/handle.rs

hansieodendaal · 2023-05-24T06:02:19Z

base_layer/wallet/src/base_node_service/monitor.rs

+    // returns true if a new block, otherwise false
+    async fn update_state(&self, new_state: BaseNodeState) -> bool {


I think we should monitor changes to the base node_id here as well (not all the rest of BaseNodeState) - if it changed then this function should return true as well. That way, if a base node is swapped, we can trigger validate_outputs to run upon that change,

Resolved with comment below

hansieodendaal · 2023-05-24T06:02:42Z

base_layer/wallet/src/output_manager_service/service.rs

-                    });
-                }
-                self.last_seen_tip_height = state.chain_metadata.map(|cm| cm.height_of_longest_chain());
+            BaseNodeEvent::BaseNodeStateChanged(_state) => {


See related comment ^^. I think we should add a ConnectedBaseNodeChanged event so we can refresh on that specifically.

hansieodendaal · 2023-05-24T06:06:50Z

base_layer/wallet/src/transaction_service/service.rs

-                    (None, _) => true,
-                    _ => false,
-                };
+            BaseNodeEvent::BaseNodeStateChanged(_state) => {


See related comment ^^.

hansieodendaal · 2023-05-24T06:13:43Z

base_layer/wallet/src/base_node_service/monitor.rs

        let mut lock = self.state.write().await;
-        let (new_block_detected, height) = match (new_state.chain_metadata.clone(), lock.chain_metadata.clone()) {
+        let (new_block_detected, height, hash) = match (new_state.chain_metadata.clone(), lock.chain_metadata.clone()) {


Suggested change

let (new_block_detected, height, hash) = match (new_state.chain_metadata.clone(), lock.chain_metadata.clone()) {

let (new_block_detected, connected_base_node_change_detected , height, hash) = match (new_state.chain_metadata.clone(), lock.chain_metadata.clone()) {

It would be better to open a ticket for this, so that we add the handling of this event in the correct places.

I don't think it's necessary, because I believe the logic of the code is to send a Default state when the base node changes. This will not publish the NewBlockEvent, but when we do get a chain metadata from the new base node, if it has the same block hash, there is no need to update, but if it changes, it will trigger the appropriate jobs

Yeah, evaluating the block hash will sort this out

hansieodendaal · 2023-05-24T06:13:52Z

base_layer/wallet/src/base_node_service/monitor.rs

        };

        if new_block_detected {
-            self.publish_event(BaseNodeEvent::NewBlockDetected(height));
+            self.publish_event(BaseNodeEvent::NewBlockDetected(hash, height));
        }



Suggested change

if connected_base_node_change_detected {

self.publish_event(BaseNodeEvent::ConnectedBaseNodeChanged)

}

Co-authored-by: Hansie Odendaal <[email protected]>

hansieodendaal

utACK

Add the ability to run tokio_console on the console_wallet. I found one case where the UI would use a lot of CPU, but I couldn't find the cause. Reduced the UI caching time to be more responsive. Renamed base_node_monitor_refresh_interval to base_node_monitor_max_refresh_interval and implemented a backoff strategy. If a new block is found when querying the base node, immediately query it again, backing off each time it has the same block. This allows you to update quickly if the node is syncing. Otherwise, backoff to the max interval. The default is 90 seconds. (was previously 3 seconds) Changed output and transaction manager service to rely on the base node service to send them an event when a new block is encountered. Both were doing this check themselves in addition to the check the base node service was doing. The base node service also only checked on the height, now the hash is also checked. --------- Co-authored-by: Hansie Odendaal <[email protected]>

## [0.50.0-pre.2](v0.50.0-pre.1...v0.50.0-pre.2) (2023-05-29) ### ⚠ BREAKING CHANGES * add optional range proof types (5372) ### Features * add metadata signature check ([5411](#5411)) ([9c2bf41](9c2bf41)) * add optional range proof types ([5372](#5372)) ([f24784f](f24784f)) * added burn feature to the console wallet ([5322](#5322)) ([45685b9](45685b9)) * improved base node monitoring ([5390](#5390)) ([c704890](c704890)) ### Bug Fixes * **comms:** only set final forward address if configured to port 0 ([5406](#5406)) ([ff7fb6d](ff7fb6d)) * deeplink to rfc spec ([5342](#5342)) ([806d3b8](806d3b8)) * don't use in memory datastores for chat client dht in integration tests ([#5399](#5399)) ([cbdca6f](cbdca6f)) * fix panic when no public addresses ([5367](#5367)) ([49be2a2](49be2a2)) * loop on mismatched passphrase entry ([5396](#5396)) ([ed120b2](ed120b2)) * use domain separation for wallet message signing ([5400](#5400)) ([7d71f8b](7d71f8b))

test: better ffi testing

582f6db

ghpbot-tari-project added the CR-insufficient_context Your PRs commit messages don't provide enough context to justify accepting the change. label May 17, 2023

stringhandler marked this pull request as draft May 17, 2023 16:06

ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels May 17, 2023

stringhandler added 3 commits May 18, 2023 12:18

wip

b247ac3

wip

af0839d

fix: add backoff to base node polling

da0c330

stringhandler changed the title ~~WIP test: better ffi testing~~ test: better ffi testing May 23, 2023

stringhandler marked this pull request as ready for review May 23, 2023 14:50

Merge branch 'development' into st-better-ffi-tests

8323377

SWvheerden reviewed May 23, 2023

View reviewed changes

stringhandler added 3 commits May 23, 2023 17:46

clippy

0ae76cf

Merge remote-tracking branch 'mike/st-better-ffi-tests' into st-bette…

5e0b854

…r-ffi-tests

Merge branch 'development' into st-better-ffi-tests

7408af3

hansieodendaal reviewed May 24, 2023

View reviewed changes

merge

4ba9996

stringhandler changed the title ~~test: better ffi testing~~ feat: improved base node monitoring May 24, 2023

stringhandler and others added 2 commits May 24, 2023 11:53

Update base_layer/wallet/src/base_node_service/handle.rs

82461ca

Co-authored-by: Hansie Odendaal <[email protected]>

fix

eaf5df0

hansieodendaal previously approved these changes May 24, 2023

View reviewed changes

ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label May 24, 2023

stringhandler added 2 commits May 24, 2023 12:56

Merge branch 'development' into st-better-ffi-tests

d86d80c

fix

dd3ee20

stringhandler dismissed hansieodendaal’s stale review via dd3ee20 May 24, 2023 13:32

ghpbot-tari-project added the P-reviews_required Process - Requires a review from a lead maintainer to be merged label May 24, 2023

stringhandler merged commit c704890 into tari-project:development May 24, 2023

stringhandler deleted the st-better-ffi-tests branch May 24, 2023 14:03

SWvheerden mentioned this pull request May 29, 2023

chore: new Esmeralda version #5423

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improved base node monitoring #5390

feat: improved base node monitoring #5390

stringhandler commented May 17, 2023 •

edited

Loading

github-actions bot commented May 17, 2023 •

edited

Loading

github-actions bot commented May 17, 2023 •

edited

Loading

SWvheerden left a comment •

edited

Loading

SWvheerden May 23, 2023

stringhandler May 23, 2023

hansieodendaal May 24, 2023

hansieodendaal May 24, 2023

SWvheerden May 23, 2023

stringhandler May 23, 2023

hansieodendaal May 24, 2023

hansieodendaal May 24, 2023

hansieodendaal left a comment

hansieodendaal May 24, 2023

hansieodendaal May 24, 2023

hansieodendaal May 24, 2023

hansieodendaal May 24, 2023

hansieodendaal May 24, 2023

stringhandler May 24, 2023

hansieodendaal May 24, 2023

hansieodendaal May 24, 2023

hansieodendaal left a comment

		// returns true if a new block, otherwise false
		async fn update_state(&self, new_state: BaseNodeState) -> bool {

	let (new_block_detected, height, hash) = match (new_state.chain_metadata.clone(), lock.chain_metadata.clone()) {
	let (new_block_detected, connected_base_node_change_detected , height, hash) = match (new_state.chain_metadata.clone(), lock.chain_metadata.clone()) {

+if connected_base_node_change_detected {
+  self.publish_event(BaseNodeEvent::ConnectedBaseNodeChanged)
+}

feat: improved base node monitoring #5390

feat: improved base node monitoring #5390

Conversation

stringhandler commented May 17, 2023 • edited Loading

github-actions bot commented May 17, 2023 • edited Loading

Test Results (CI)

github-actions bot commented May 17, 2023 • edited Loading

Test Results (Integration tests)

SWvheerden left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansieodendaal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansieodendaal left a comment

Choose a reason for hiding this comment

stringhandler commented May 17, 2023 •

edited

Loading

github-actions bot commented May 17, 2023 •

edited

Loading

github-actions bot commented May 17, 2023 •

edited

Loading

SWvheerden left a comment •

edited

Loading