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.

Sign up for GitHub

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

Jump to bottom

increase value of MAX_SLOTS_TO_REMEMBER to 14400 #355

Merged
merged 3 commits into from
May 30, 2023
Merged

increase value of MAX_SLOTS_TO_REMEMBER to 14400 #355

merged 3 commits into from
May 30, 2023

Conversation

b-yap
Copy link
Contributor

@b-yap b-yap commented May 30, 2023

closes #354

@b-yap b-yap requested review from gonzamontiel and a team and removed request for gonzamontiel May 30, 2023 08:46
ebma
ebma requested changes May 30, 2023
View reviewed changes
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to increment this value even further, just not yet sure to what exactly.

@TorstenStueber
Copy link
Contributor

As a reference: #354

@ebma
Copy link
Member

ebma commented May 30, 2023

I updated the original ticket. We want to increase it to 14400 now. But we'll first have to check if the archive nodes break down because of that.

@b-yap b-yap changed the title increase value of MAX_SLOTS_TO_REMEMBER to 100 increase value of MAX_SLOTS_TO_REMEMBER to ~~100~~ 14400 May 30, 2023
@b-yap b-yap changed the title increase value of MAX_SLOTS_TO_REMEMBER to ~~100~~ 14400 increase value of MAX_SLOTS_TO_REMEMBER to 14400 May 30, 2023
ebma
ebma approved these changes May 30, 2023
View reviewed changes
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. We could already merge this even though the parameter is not accurate (ie. not matched by our deployed nodes). But the archive logic is broken anyways, so there is probably no harm done.

clients/vault/src/oracle/constants.rs Outdated
Comment on lines 19 to 20
/// increase the time window to 24 hours.
pub const MAX_SLOTS_TO_REMEMBER: Slot = 14400;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Let's maybe rewrite the comment though to give a bit more context.

Suggested change
/// increase the time window to 24 hours.
pub const MAX_SLOTS_TO_REMEMBER: Slot = 14400;
/// Set the _expected_ `MAX_SLOTS_TO_REMEMBER` parameter to 14400 Slots on Stellar,
/// ie. 24 hours. This parameter is only correct when connecting to SatoshiPay Stellar validators
/// as the configuration of these nodes deviates from the default configuration.
pub const MAX_SLOTS_TO_REMEMBER: Slot = 14400;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d7e8724
done

@b-yap b-yap merged commit 1fd3577 into main May 30, 2023
@b-yap b-yap deleted the 354-increase-time-window-for-fetching-scp-messages-from-overlay branch May 30, 2023 13:59
TorstenStueber
TorstenStueber reviewed Jun 1, 2023
View reviewed changes
Copy link
Contributor

@TorstenStueber TorstenStueber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TorstenStueber TorstenStueber TorstenStueber left review comments

@ebma ebma ebma approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Increase time window for fetching SCP messages from overlay
3 participants
@b-yap @TorstenStueber @ebma