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

fix: expire all previous epochs #5279

Merged
merged 8 commits into from
Oct 24, 2024
Merged

fix: expire all previous epochs #5279

merged 8 commits into from
Oct 24, 2024

Conversation

j4m1ef0rd
Copy link
Contributor

Pull Request

Closes: PRO-1613

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have written sufficient tests.
  • I have written and tested required migrations.
  • I have updated documentation where appropriate.

Summary

  • Changed the expire_epoch function to recursively expire all previous epochs that had not expired yet in chronological order. This should avoid the bug stated in the issue.
    • Added a test to make sure it expires as expected.
  • Renamed BlocksPerEpoch -> EpochDuration
    • Added a migration and migration test for the rename.

Non-Breaking changes

I think this is non breaking, i'll let you tag it @dandanlen

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 84.09091% with 21 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (b3a2e3b) to head (bb3a729).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
state-chain/custom-rpc/src/monitoring.rs 0% 14 Missing ⚠️
state-chain/pallets/cf-validator/src/lib.rs 86% 3 Missing ⚠️
state-chain/runtime/src/lib.rs 0% 0 Missing and 3 partials ⚠️
state-chain/custom-rpc/src/lib.rs 0% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #5279    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        494     496     +2     
  Lines      85777   86262   +485     
  Branches   85777   86262   +485     
======================================
+ Hits       61165   61474   +309     
- Misses     21878   22018   +140     
- Partials    2734    2770    +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

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

@j4m1ef0rd I'll make the below changes.

Will wait for @acdibble and @marcellorigotti 's inputs re. api breakage before merging.

@@ -350,7 +350,7 @@ type RpcSuspensions = Vec<(Offence, Vec<(u32, state_chain_runtime::AccountId)>)>

#[derive(Serialize, Deserialize)]
pub struct RpcAuctionState {
blocks_per_epoch: u32,
epoch_duration: u32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately this is a breaking change 🙈
We could keep the old name around and flag it as deprecated... or maybe just keep the old name with a comment explaining that the name is for historical reasons...

@acdibble how much of a pain is it for you to change this?

Copy link
Contributor

Choose a reason for hiding this comment

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

should be fine as long as we are aware ahead of time. thanks for pointing it out

state-chain/pallets/cf-validator/src/lib.rs Outdated Show resolved Hide resolved
state-chain/runtime/src/monitoring_apis.rs Show resolved Hide resolved
@kylezs
Copy link
Contributor

kylezs commented Oct 18, 2024

What's the status on this one?

Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

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

Conflicts need to be resolved, then it should be good.

@dandanlen dandanlen added this pull request to the merge queue Oct 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 24, 2024
@kylezs
Copy link
Contributor

kylezs commented Oct 24, 2024

Looks like it's hitting a migration issue @j4m1ef0rd

@dandanlen dandanlen added this pull request to the merge queue Oct 24, 2024
Merged via the queue into main with commit 5baa3dd Oct 24, 2024
48 of 49 checks passed
@dandanlen dandanlen deleted the fix/expire-prev-epochs branch October 24, 2024 17:34
syan095 added a commit that referenced this pull request Oct 29, 2024
…waps-close-accounts

* origin/main: (44 commits)
  fix: expire all previous epochs (#5279)
  feat: add/update contract swaps parameters (#5343)
  chore: add address to solana logging (#5353)
  fix: ignore dust underflows in order fills rpc (#5352)
  chore: consistent naming prewitnessed (#5351)
  feat: engine-runner verifies gpg signature of old dylib when downloaded (#5339)
  feat: tainted transaction reporting (#5310)
  bug: change_utxo not always present (#5340)
  feat: structured error return types for rpcs (#5346)
  chore: unify dependencies to root cargo.toml (#5333)
  feat: Submit a slot number alongside nonce (#5297)
  chore: use node version from `.nvmrc` 📌 (#5336)
  chore: add engine account_info logging (#5347)
  chore: replace manual scale encoding for ts-scale (#5335)
  chore: more consistent params in Broker API (#5342)
  feat: broker can encode btc smart contract call (#5329)
  chore: localnet recreate script can use defaults (#5338)
  feat: witnessing btc smart contract swaps (#5331)
  feat: Solana CCM fallback (#5316)
  fix: scale types for pending ceremonies (#5286)
  ...

# Conflicts:
#	Cargo.lock
#	state-chain/chains/src/sol/api.rs
#	state-chain/pallets/cf-broadcast/src/migrations.rs
#	state-chain/pallets/cf-environment/Cargo.toml
kylezs added a commit that referenced this pull request Oct 29, 2024
github-merge-queue bot pushed a commit that referenced this pull request Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants