-
Notifications
You must be signed in to change notification settings - Fork 17
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 epoch saving #909
fix epoch saving #909
Conversation
const { sctParams } = await this.querier.app.appParams(); | ||
const { sctParams } = (await this.indexedDb.getAppParams()) ?? {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also fixed this, we don't need to request sctParams
from the full node since we store them in indexed-db
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this.indexedDb.addEpoch()
should go above this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grod220 why's that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The params are used further down. There seems to be code in between that doesn't use the params.
// avoid saving the same epoch twice | ||
if (previousEpoch?.startHeight === startHeight) return; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I thought it would be best to link to epochRoot
.
But it seems that binding to a unique startHeight
is just as good, plus I'm not sure that epochRoot
is guaranteed to be unique for every epoch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will let Jesse review this one, but good catch!
const { sctParams } = await this.querier.app.appParams(); | ||
const { sctParams } = (await this.indexedDb.getAppParams()) ?? {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this.indexedDb.addEpoch()
should go above this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch — minor comment re: a variable name, then ship it!
Co-authored-by: Jesse Pinho <jesse@jessepinho.com>
oh sorry @Valentine1898 , before you merge, could you please bump the IndexedDB version number so that people's DBs re-sync from scratch without the problematic epochs? |
Yes, it makes sense even though the indexed-db structure has not changed |
close #908