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 warmup by decoupling state from message receipt walk #6841

Merged
merged 2 commits into from
Jul 22, 2021

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented Jul 22, 2021

this was accidentally broken in #6775.

@vyzo vyzo requested a review from magik6k July 22, 2021 17:51
@vyzo vyzo requested a review from a team as a code owner July 22, 2021 17:51
Copy link
Contributor

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

SGWM but could use tests

@vyzo
Copy link
Contributor Author

vyzo commented Jul 22, 2021

Yeah, tests forthcoming soon -- we have #6834 open.
I probably won't get to it today, as there are a couple of things ahead of it in the queue, but I'll get to it asap.

@@ -631,14 +631,16 @@ func (s *SplitStore) endTxnProtect() {
s.txnMissing = nil
}

func (s *SplitStore) walkChain(ts *types.TipSet, inclState abi.ChainEpoch, inclMsgs abi.ChainEpoch,
func (s *SplitStore) walkChain(ts *types.TipSet, inclState, inclMsgs, inclMsgReceipts abi.ChainEpoch,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why separate height for receipts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, I wanted to decouple it from state as it was the source of the bug -- when we sync from snapshot we dont have message receipts at all!

We could ostensibly couple them with the messages, but I don't think we need them at all in order to support the use cases -- please correct me if I am wrong :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

following sync discussion, we discussed to collate them with message retention.

@magik6k magik6k enabled auto-merge July 22, 2021 19:06
@magik6k magik6k merged commit 0f21fcc into master Jul 22, 2021
@magik6k magik6k deleted the fix/splitstore-warmup branch July 22, 2021 19:22
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.

3 participants