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

Now load conversions from storage even for epoch 1. #1244

Merged
merged 2 commits into from
Mar 28, 2023
Merged

Now load conversions from storage even for epoch 1. #1244

merged 2 commits into from
Mar 28, 2023

Conversation

murisi
Copy link
Collaborator

@murisi murisi commented Mar 23, 2023 •
edited
Loading

  • This pull request fixes a bug causing inconsistencies in the value of the conv key among different (or even on a single) nodes.
  • This bug can be reproduced locally by stopping a node in epoch 1, then restarting it, and then querying the conversion state using namadac conversions once epoch 2 is entered. The output observed will be different to what it would have been had the node just been left to run without interruption.
  • This bug is caused by a fencepost error which causes nodes to fail to load MASP conversion state data when that last saved state is in epoch 1. This fix merely fixes the condition that triggers the loading of past conversion state data.

@murisi murisi requested review from juped and tzemanovic March 23, 2023 07:09
@tzemanovic
Copy link
Member

there's a failed test in PoS from #1245 - we already have a fix for it so nvm it

tzemanovic
tzemanovic previously approved these changes Mar 23, 2023
View reviewed changes
Copy link
Member

@tzemanovic tzemanovic left a comment

Choose a reason for hiding this comment

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

The fix LGTM! We'll however need better test coverage for masp logic and the shell, but it's probably gonna be a bigger effort

@tzemanovic
Copy link
Member

there's a failed test in PoS from #1245 - we already have a fix for it so nvm it

ah, so this is not the same issue, but it's another one we've also seen in #714. I'm having trouble reproducing it, but either way, it's not related to this change

juped added a commit that referenced this pull request Mar 23, 2023
@juped
Merge remote-tracking branch 'namada/murisi/conversion-loading-fix' (#…
962046b 
…1244) into maint-0.14

* namada/murisi/conversion-loading-fix:
  Now load conversions from storage even for epoch 1.
  fix spelling
  chain-id added
  new docs
  remove debug messages
  add changelog
  fix: query for proof and IBC e2e test
  Update docs to be more consistent with the intnent
juped added a commit that referenced this pull request Mar 23, 2023
@juped
Revert "Merge remote-tracking branch 'namada/murisi/conversion-loadin…
44e4ced 
…g-fix' (#1244) into maint-0.14"

This reverts commit 962046b, reversing
changes made to 37e053d.
juped added a commit that referenced this pull request Mar 27, 2023
@juped
Merge branch 'murisi/conversion-loading-fix' (#1244) into maint-0.14
f351c47 
* murisi/conversion-loading-fix:
  Now load conversions from storage even for epoch 1.
@juped juped force-pushed the murisi/conversion-loading-fix branch from 72433d9 to 52b6d93 Compare March 27, 2023 11:29
@juped juped merged commit effd040 into main Mar 28, 2023
@juped juped deleted the murisi/conversion-loading-fix branch March 28, 2023 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tzemanovic tzemanovic tzemanovic left review comments

@juped juped Awaiting requested review from juped

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

Successfully merging this pull request may close these issues.
3 participants
@murisi @tzemanovic @juped