Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[base node] Force sync peers doesnt work (duplicate config) #4646

Closed
sdbondi opened this issue Sep 8, 2022 · 2 comments
Closed

[base node] Force sync peers doesnt work (duplicate config) #4646

sdbondi opened this issue Sep 8, 2022 · 2 comments
Labels
A-base_node Area - The Tari base node executable and libraries C-bug Category - fixes a bug, typically associated with an issue. P-more_info_needed Process - Please provide more information

Comments

@sdbondi
Copy link
Member

sdbondi commented Sep 8, 2022

No description provided.

stringhandler pushed a commit that referenced this issue Sep 8, 2022
Description
---
- fix(rpc/server): detect stream interrupt when waiting for responses from domain service and when sending any data
- fix(rpc/client): detect early response stream drop while reading responses
- fix(rpc): correct value for number of sessions per peer
- fix(base_node/sync): use faster/simpler mutex for sync session tracking
- fix(base-node): set force_sync_peers conf from `base_node.force_sync_peers`
- tests(comms/rpc): test for session handling when a response stream with large messages is interrupted 
- fix(p2p/liveness): pong event returns the latency of the last ping/pong 

Motivation and Context
---
Whenever sync latency exceeds the max, the client interrupts the stream. The server rarely picks this up and since only one sync session is allowed, the client node can not resume syncing from the same node.

fixes #4630 
ref #4646 (fixes but duplicate config should be removed)
fixes #4636 

How Has This Been Tested?
---
New perviously failing integration test, manually by syncing a new base node
@stringhandler stringhandler added this to the Stagenet Freeze milestone Nov 11, 2022
@stringhandler stringhandler added C-bug Category - fixes a bug, typically associated with an issue. A-base_node Area - The Tari base node executable and libraries labels Nov 11, 2022
@stringhandler
Copy link
Collaborator

To confirm I understand:
"If you add a force sync peer to the config, it is not enforced"

@stringhandler stringhandler added the P-more_info_needed Process - Please provide more information label Nov 11, 2022
@sdbondi
Copy link
Member Author

sdbondi commented Nov 11, 2022

This was hack fixed in the PR that references this (https://github.com/tari-project/tari/pull/4647/files#diff-6e9e3f32e84954314cbbdf459c53d1e61da84ab09d309e787fecb99b57970ee6R98) so we can close this one.

I'll look into it and make a proper issue because I think there were a few config fields that were duplicated in different structs that could lead to bugs and should be cleaned up

@sdbondi sdbondi closed this as completed Nov 11, 2022
Repository owner moved this from Bugs to Done in Tari Esme Testnet Nov 11, 2022
stringhandler pushed a commit that referenced this issue Nov 23, 2022
Description
---
Removed `max_randomx_vms` from `BaseNodeStateMachineConfig` and the config preset, now passing the initialized RandomXFactory in via the initializer, same for `bypass_range_proof_verification` but passing only the setting


Motivation and Context
---
#4909

There are a few duplicate config keys that exist in the base node config and the base node state machine config
meaning settings and behaviour could mismatch.

`base_node.max_randomx_vms` (this setting will not work)
`base_node.bypass_range_proof_verification` (this setting will not work)
`base_node.force_sync_peers` (this was fixed in #4647 but the fix is a bit hacky) 

https://github.com/tari-project/tari/blob/956b27954dda1f15f82bff0ba0ba0ee1f0880d2d/base_layer/core/src/base_node/state_machine_service/state_machine.rs#L52

https://github.com/tari-project/tari/blob/development/applications/tari_base_node/src/config.rs#L109

ref #4646

How Has This Been Tested?
---
manually
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-base_node Area - The Tari base node executable and libraries C-bug Category - fixes a bug, typically associated with an issue. P-more_info_needed Process - Please provide more information
Projects
Archived in project
Development

No branches or pull requests

2 participants