-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Splitstore: add support for protecting out of chain references in the blockstore #6777
Conversation
Following sync discussion with @Stebalien , we decided that we shouldn't indirect through the chainstore and instead hook the gc hooks directly through DI. |
5ef95ef
to
0fb966d
Compare
0fb966d
to
c47fce8
Compare
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.
Some nits but this is much nicer.
@@ -58,7 +58,7 @@ func ChainBlockService(bs dtypes.ExposedBlockstore, rem dtypes.ChainBitswap) dty | |||
return blockservice.New(bs, rem) | |||
} | |||
|
|||
func MessagePool(lc fx.Lifecycle, mpp messagepool.Provider, ds dtypes.MetadataDS, nn dtypes.NetworkName, j journal.Journal) (*messagepool.MessagePool, error) { | |||
func MessagePool(lc fx.Lifecycle, mpp messagepool.Provider, ds dtypes.MetadataDS, nn dtypes.NetworkName, j journal.Journal, protector dtypes.GCReferenceProtector) (*messagepool.MessagePool, error) { |
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.
This isn't really a dependency, so I assume this would be a separate "invoke" step (next to the splitstore step). But I don't have strong opinions.
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.
Well, I have strong opinions. Just not about 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.
We could do it with an invoke step, but doing it here avoids all possible races I think.
No strong opinions on this either.
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.
basically I want to make sure that AddProtector
invocations all happen before Start
in the splitstore.
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 assume that Start
gets called from a hook, which runs after all invokes (IIRC).
flaky test failure in https://app.circleci.com/pipelines/github/filecoin-project/lotus/15905/workflows/5d4be844-ce96-4922-ac4c-09503c0d3b3e/jobs/205203 Opened issue #6799 I am going to disable the test as it gets on everyone's nerves. |
This adds a mechanism for protecting out-of-chain references during compaction (and in the future gc).
The problem is that we actually have live references in the blockstore, that are not (yet) reachable from the chain. This arises from pending messages in the mpool, and can result in archiving/discarding such messages during compaction even though they are live.
The solution is to do what garbage collecting programming languages do since the beginning of time: add a hook to protect external references.
The hook is hooked[sic] to the mpool through DI.
Open question: are there other such references that need to be protected?