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 a few bugs in revalidate (mini) #16401

Merged
merged 6 commits into from
Jan 14, 2025
Merged

Conversation

georgeee
Copy link
Member

@georgeee georgeee commented Dec 5, 2024

  1. Fix two similar issues originating from confusion between previous variable names t and t' (Account no longer has permission to send and Current account nonce precedes first nonce in queue)
  2. Fix the issue multiple nodes crashed #16397 by ensuring removal from applicable_by_fee is
    done only for the previous head of queue.

Explain how you tested your changes:

Checklist

  • Dependency versions are unchanged
    • Notify Velocity team if dependencies must change in CI
  • Modified the current draft of release notes with details on what is completed or incomplete within this project
  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them

Fix two similar issues originating from confusion between previous variable
names `t` and `t'`, affecting cases:

- `Account no longer has permission to send`
- `Current account nonce precedes first nonce in queue`
@georgeee georgeee requested a review from a team as a code owner December 5, 2024 15:47
Fix the issue #16397 by ensuring removal from `applicable_by_fee` is
done only for the previous head of queue.
@georgeee georgeee force-pushed the georgeee/fix-16397-mini branch from f4a890a to 1b95a6e Compare December 5, 2024 17:42
@georgeee georgeee mentioned this pull request Dec 5, 2024
8 tasks
@@ -739,14 +739,14 @@ let revalidate :
then (
[%log debug]
"Account no longer has permission to send; dropping queue" ;
let dropped, t'' = remove_with_dependents_exn' t_initial first_cmd in
(t'', Sequence.append dropped_acc dropped) )
let dropped, t_updated = remove_with_dependents_exn' t first_cmd in
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

@georgeee
Copy link
Member Author

!ci-build-me

@coveralls
Copy link

coveralls commented Dec 21, 2024

Coverage Status

coverage: 30.63% (-30.6%) from 61.247%
when pulling 1b95a6e on georgeee/fix-16397-mini
into f820d28 on master.

@georgeee georgeee changed the base branch from master to compatible January 14, 2025 15:13
@georgeee georgeee requested a review from a team as a code owner January 14, 2025 15:13
@georgeee
Copy link
Member Author

!ci-build-me

@georgeee georgeee force-pushed the georgeee/fix-16397-mini branch from 17befc9 to 1b95a6e Compare January 14, 2025 16:00
@georgeee georgeee changed the base branch from compatible to master January 14, 2025 16:01
@georgeee georgeee changed the base branch from master to release/3.0.3.1 January 14, 2025 20:07
@mrmr1993
Copy link
Member

!approved-for-mainnet

@amc-ie amc-ie merged commit cc59a03 into release/3.0.3.1 Jan 14, 2025
46 of 49 checks passed
@amc-ie amc-ie deleted the georgeee/fix-16397-mini branch January 14, 2025 21:15
sebastiencs added a commit to openmina/openmina that referenced this pull request Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants