-
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
feat(tx_index): fill transaction index gap on enabling automatic backfill #12353
base: master
Are you sure you want to change the base?
feat(tx_index): fill transaction index gap on enabling automatic backfill #12353
Conversation
@aarshkshah1992 As per our discussion about waiting till the observer gets the latest head and process 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.
Round 1 of review. @akaladarshi Please can we also add logic to populate this from a snapshot just like we do for the MsgIndex
?
@@ -78,7 +84,7 @@ func EthModuleAPI(cfg config.FevmConfig) func(helpers.MetricsCtx, repo.LockedRep | |||
} | |||
|
|||
// Tipset listener | |||
_ = ev.Observe(ðTxHashManager) | |||
ev.Observe(ðTxHashManager) | |||
|
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 don't need this change.
node/config/types.go
Outdated
@@ -621,6 +621,9 @@ type IndexConfig struct { | |||
// EXPERIMENTAL FEATURE. USE WITH CAUTION | |||
// EnableMsgIndex enables indexing of messages on chain. | |||
EnableMsgIndex bool | |||
|
|||
// EnableAutomaticBackFill enables automatic index back-filling | |||
EnableAutomaticBackFill bool |
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.
For now, we need to rename this to EnableAutomaticBackFillTxIndex
as we're only doing it for the TX Index.
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.
Done.
node/impl/full/txhashmanager.go
Outdated
return nil | ||
} | ||
|
||
// PopulateExistingMappings walk back from the current head to the minimum height and populate the eth transaction hash lookup database |
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.
nit: check grammar
node/impl/full/txhashmanager.go
Outdated
@@ -23,6 +23,49 @@ func (m *EthTxHashManager) Revert(ctx context.Context, from, to *types.TipSet) e | |||
return nil | |||
} | |||
|
|||
// FillIndexGap back-fills the gap between the current head and the last populated transaction index |
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.
nit: "last populated transaction index in* the Index"
node/impl/full/txhashmanager.go
Outdated
|
||
totalProcessedBlock := 0 | ||
ts := m.StateAPI.Chain.GetHeaviestTipSet() | ||
for _, block := range ts.Blocks() { |
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 think we need to keep going back in the chain to backfill. This loop terminates after backfilling messages in the heaviest tipset only.
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 need to be careful on two fronts with going backward up the chain for two reasons:
- the last processed message CID may not be on the canonical chain anymore so you may never encounter it; and
- the last processed message may not even be in the blockstore due to a splitstore compaciton or some other happening (e.g. you've copied across someone else's txhash.db or restored an old one from disk, whatever)
I suspect ChainGetPath
might be a good API to try here: get the tipset of the lastTxHash
, get the latest tipset, find the path between. I'm just not sure what the error conditions are for that - how big a gap is too large, and how do we deal with splitstore GC problems (I suspect "find me the tipset for lastTxHash
would error in that case if it was too old, in which case what's the strategy then?).
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.
@rvagg I have been going through the ChainGetPath
code, but I didn't see it returning a too old
error, it only returns a not found
error.
If it is not found then nodes have to import the snapshot.
Is there any other way to get the state?
Also ChainGetPath
returns both revert
and apply
changes, so do we have to filter out apply
changes and start backfilling?
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.
Discuss this with @akaladarshi async. We worked through multiple solutions here. He will post a note about it.
node/impl/full/txhashmanager.go
Outdated
totalProcessedBlock++ | ||
} | ||
|
||
ts, err = m.StateAPI.Chain.GetTipSetFromKey(ctx, ts.Parents()) |
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 needs to move to the loop above
for _, msg := range msgs { | ||
if msg.Cid() == lastTxCid { | ||
// We've reached the last indexed transaction | ||
return nil |
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.
log completion and then return. Add logging around how many entires were inserted, which tipset it started from and which one it ended at etc.
@aarshkshah1992 For the observer part, I thought we could do something like this:
|
0ec35b0
to
4b56a36
Compare
Related Issues
Fixes part of: #11007
Proposed Changes
EnableAutomaticBackFill
in index config infullNode
MaxAutomaticBackFillTxIndexHeight
in index config infullnode
current tipset tx hashes
till:MaxAutomaticBackFillTxIndexHeight
oreth tx hash index
databasePopulateAfterSnapshot
for eth tx hash index to populate data for while loading from snapshot