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

add expensive reverse lookup #17

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aschmahmann
Copy link
Owner

This allows forcing a reverse lookup for an identity address if you really want to, it's of similar cost to enumerate-actors.

@@ -115,6 +120,28 @@ func filAddrs(cctx *cli.Context) error {
// If it's a masked 0x address, return the f0 and if enabled the expensive reverse lookup for the fX address
fmt.Println(addr)
}

if expensiveLookup {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Not sure how frequently people are going to use this, my hope is if it's common enough someone will show up with a use case to determine how to handle it (e.g. if they're calling it repeatedly the type of cache that would be most useful). At the moment I don't do this more than occasionally for debugging so leaving it alone.

@aschmahmann aschmahmann force-pushed the feat/expensive-reverse-lookup branch 2 times, most recently from 6f10072 to 96d8e31 Compare June 23, 2024 06:00
@aschmahmann aschmahmann force-pushed the feat/expensive-reverse-lookup branch from 96d8e31 to 727f420 Compare June 23, 2024 06:11
Comment on lines +187 to +200
func init() {
latestInitCode := lbiinit.AllCodes()[len(lbiinit.AllCodes())-1]
if name, av, ok := actors.GetActorMetaByCode(latestInitCode); ok {
if name != manifest.InitKey {
panic(xerrors.Errorf("actor code is not init: %s", name))
}

if av != actorstypes.Version13 {
panic(xerrors.Errorf(
"the application is out of date with the network, please update to a later version," +
" or if this is the latest version file an issue to update the init actor version"))
}
}
}
Copy link
Owner Author

@aschmahmann aschmahmann Jun 23, 2024

Choose a reason for hiding this comment

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

@ribasushi is there a better way to do this? Ideally I'd be able to just use the latest version (or map a code/actor version to the init state struct), but looking for some alternative options here.

Some notes:

  1. This could probably be in a test (there should probably be a minimal testing setup 😅)
  2. More generally is the network version in the state tree somewhere such that it'd be easy enough to check that the code is out of date with the network and may/may not be accurate/safe until updated? (maybe existing code paths will already do this for me, but not sure if they all will/do now that we're not using the state manager code anymore)

Copy link
Collaborator

Choose a reason for hiding this comment

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

What you are doing here locks you into "latest upgrade interval" only: you won't be able to query stuff before a particular epoch this way. Not sure this is the right way to go about this at all..

Ideally you recreate something like this (or have filoz put it in go-state-types where it belongs)
https://github.com/filecoin-project/lotus/blob/master/chain/actors/builtin/init/init.go#L32-L90

Copy link
Owner Author

Choose a reason for hiding this comment

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

you won't be able to query stuff before a particular epoch this way. Not sure this is the right way to go about this at all

Ah, I see. So I probably still need to keep this code in place (or with a test) so as to force updating the code when a new version comes out, but I should handle the historical versions as well with some sort of large map. Not sure if this would require committing to too many abstractions from the filoz folks since I'm operating on the IPLD object so I can load it into a parallel traversing HAMT which is lower level than the abstractions they currently have.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Afaik the HAMT never changed. I have a factored out code I will push tmrw, should make it cleaner...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a factored out code I will push tmrw, should make it cleaner...

@aschmahmann ☝️ it took 2 months, but here it is: #20

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.

2 participants