Implement scaffolding for sentinel integration #5672
Conversation
frrist
force-pushed
the
sentinel/module-scaffold
branch
from
9b8c48d to
c7393e9
Compare
frrist
force-pushed
the
sentinel/module-scaffold
branch
from
c7393e9 to
e3a757b
Compare
frrist
changed the title
frrist
force-pushed
the
sentinel/module-scaffold
branch
from
c8a92fd to
8fb82ea
Compare
frrist
requested review from
Kubuxu,
magik6k and
whyrusleeping
as code owners
- allows lotus to be started in sentinel mode and start a watch that logs tipsets as they are received.
- replaces the sentinel flag on daemon command.
frrist
force-pushed
the
sentinel/module-scaffold
branch
from
8fb82ea to
6789392
Compare
There was a problem hiding this comment.
Looks pretty good, just a couple of comments/discussion points.
Docsgen is unhappy, probably because it expects flat interfaces (make docsgen / likely broken somewhere here - https://github.com/filecoin-project/lotus/blob/master/api/docgen/docgen.go#L411-L504)
| @@ -470,6 +471,12 @@ type WalletStruct struct { | |||
| } | |||
| } | |||
|
|
|||
| type SentinelStruct struct { | |||
There was a problem hiding this comment.
I think it might be easier to have the sentinel methods be defined on the FullNodeStruct struct, so it doesn't have to be registered in all the random places
There was a problem hiding this comment.
While I agree it would be easier, I believe separation will make future work simpler to land as changes will only occur in sentinel-related structures and files. I think this is a bit cleaner too. But, if you feel strongly about defining methods on the FullNodeStruct, we can do it that way instead.
There was a problem hiding this comment.
Looks good and not too intrusive.
But I'd like us to explore how we could make it even less intrusive, mainly to hedge our bets while we continue to explore possibilities for less coupling between sentinel and Lotus.
What would the changes look like if we had a separate binary to run that imported all the necessary parts of Lotus daemon? Could we inject the module from that code instead of adding it to the various Lotus packages?
This would avoid the clunky build flag but also potentially allows that binary to live outside the Lotus codebase
| @@ -0,0 +1,3 @@ | |||
| package build | |||
|
|
|||
| var SentinelMode string = "false" | |||
There was a problem hiding this comment.
Setting and parsing this seems really clunky. Is this the pattern already in use by Lotus? If so then let's leave it (but it's still clunky!)
In theory the Go compiler could optimize away code that isn't needed for sentinel mode but I think this would have to be a const bool for that to work.
I feel that fx must have better support for compile time switching of modules.
There was a problem hiding this comment.
The only prior art I saw for this was passing the version via a build flag. @magik6k wants to avoid adding more flags to the daemon and suggested a build flag or a go plugin. If, as you've already suggested, I can pull the required bits into a separate binary, then I think this flag can be removed.
There was a problem hiding this comment.
Unless separate binary, using build tags might be cleaner: go build -tags sentinel. It also forces to separate everything sentinel-specific to its own files.
There was a problem hiding this comment.
I've implemented a standalone binary here: #5693
|
going with #5693 |
What
tipsets as they are received. closes Add Sentinel module scaffolding to Lotus sentinel#179
lotus-sentinel.TODO
panic: no perms for method: WatchStartbut WatchStart has perms admin