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 badger double open on daemon --import-snapshot; chainstore lifecycle #4872

Merged
merged 6 commits into from
Nov 18, 2020

Conversation

raulk
Copy link
Member

@raulk raulk commented Nov 16, 2020

This PR fixes #4850. The problem was that the IdStore did not implement io.Closer, therefore the store remained open after the import. This required a release of go-ipfs-blockstore, which has now been integrated here.

I also noticed this error (which I think has been there since forever; EDIT: confirmed, this was already there! #4428) just after the import is done, and before we start the node:

2020-11-16T15:08:17.027Z        INFO    main    lotus/daemon.go:474     accepting [bafy2bzaceckovkpb5vua5s2sbltjg2lynikeehepztyjsf7n2nrehot36bwdc] as new head
2020-11-16T15:08:17.027Z        INFO    chainstore      store/store.go:500      New heaviest tipset! [bafy2bzaceckovkpb5vua5s2sbltjg2lynikeehepztyjsf7n2nrehot36bwdc] (height=240581)
2020-11-16T15:08:17.027Z        INFO    chainstore      store/store.go:687      failed to fetch right.Parents: get block bafy2bzaceaoyzhu6i2qpsi2efx7wp2z7kta54typ5fewd52mlsvg4azjrnkjg: badger blockstore closed
2020-11-16T15:08:17.027Z        ERROR   chainstore      store/store.go:420      computing reorg ops failed: get block bafy2bzaceaoyzhu6i2qpsi2efx7wp2z7kta54typ5fewd52mlsvg4azjrnkjg: badger blockstore closed

This happens because the SetHead inside ImportChain triggers a reorg, but by the team the event arrives to the event loop, the badger store is closed. There are two problems here:

  1. The chainstore does not accept a context, thus it the reorg runner has no way to stop. Currently, this would leak a goroutine. Fixes dangling reorgWorker after snapshot import #4428.
  2. This forced SetHead should not trigger a reorg; we can use the newly-added ForceHeadSilent operation to set the head without a reorg.

This PR fixes both those things too.

@@ -383,7 +382,7 @@ func (cs *ChainStore) MaybeTakeHeavierTipSet(ctx context.Context, ts *types.TipS
// particular tipset to carry out a benchmark, verification, etc. on a chain
// segment.
func (cs *ChainStore) ForceHeadSilent(_ context.Context, ts *types.TipSet) error {
log.Warnf("(!!!) forcing a new head silently; only use this only for testing; new head: %s", ts)
Copy link
Member

Choose a reason for hiding this comment

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

eh?

Copy link
Member Author

Choose a reason for hiding this comment

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

?

Copy link
Contributor

Choose a reason for hiding this comment

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

There was no valid use for this outside testing before. When importing snapshot it doesn't make sense to emit reorg notifications as nothing is listening on that chainstore, and everything that cares about chain state does check current state when subscribing

tldr this is fine

@Kubuxu
Copy link
Contributor

Kubuxu commented Nov 16, 2020

Personally I would prefer if chainstore had a Close method instead of relying on the context for the lifetime.

@raulk raulk force-pushed the raulk/fix-blockstore-import branch from aec4fdf to 0c6072a Compare November 16, 2020 22:29
@raulk
Copy link
Member Author

raulk commented Nov 16, 2020

@Kubuxu agree; addressed in 0c6072a.

@raulk raulk changed the title fix badger double open on daemon --import-snapshot; pass ctx to chainstore fix badger double open on daemon --import-snapshot; chainstore lifecycle Nov 16, 2020
@raulk
Copy link
Member Author

raulk commented Nov 17, 2020

@magik6k wanna merge this?

@magik6k magik6k merged commit 50146fb into master Nov 18, 2020
@magik6k magik6k deleted the raulk/fix-blockstore-import branch November 18, 2020 00:36
bibibong pushed a commit to EpiK-Protocol/go-epik that referenced this pull request Jan 7, 2021
…fix-blockstore-import

fix badger double open on daemon --import-snapshot; chainstore lifecycle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants