-
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(f3): resolve finality for eth APIs according to F3 #12762
base: master
Are you sure you want to change the base?
Conversation
cd0f34f
to
cad6bfa
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.
No blockers. I recommend getting error construction to be consistent. Left some comments on what might be surprising from DX/maintainer perspective.
} | ||
if running, _ := tsr.f3.IsRunning(); !running { | ||
return nil, 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.
We should have an additional check here for "is F3 running with finalization/static manifest".
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, here is my take on this comment:
is F3 running with finalization
This one means if "finalization" is enabled in F3 manifest, which in Lotus lingo translates to "Should F3 checkpoint the finalized tipsets?". It is specified here.
If finalisation in disabled in F3, then the API should be using EC finality regardless of whether F3 itself is enabled or not.
/static manifest
The F3 manifest integration in lotus is dynamic by default but up to a point, beyond which it will revert exclusively to a static manifest that "activates" F3 as the consensus mechanism for the network with uniform parameters across all nodes and no longer controllable by the F3 engineering team.
In the static manifest, finalisation is enabled. If we are in a state where F3 is running, and we are using static manifest (i.e. F3 is active) then use F3 to resolve a tipset's finality.
@Kubuxu please correct me if I have missed anything.
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.
OK, thanks! I the static/dynamic thing has more nuance than I was aware of, I probably should go read the spec to understand that a bit more deeply.
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.
OK, went digging to understand a bit more of the dynamic/static situation and ended up having to dig into code. From what I can tell, there's no clear way of telling that we're not operating under a dynamic manifest, so the only check we can do here (the only check we need?) is to ask whether manifest.EC.Finalize
is true
, and if it's not, then ignore F3.
Does that sound right?
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 believe this is all handled now but I wouldn't mind a sanity check from both of you (@masih & @Kubuxu) in this function in particular.
It's now called maybeF3FinalizedTipSet
to indicate that you may not get something useful from it. If F3 can't help us then we just move on to the defaults.
- We're not going to error here unless it's from dealing with the chain store, an error from F3 is worthy of a WARN log, and only if the error isn't
ErrF3Disabled
. - If
!manifest.EC.Finalize
, we don't bother continuing - Some basic defensive checks on the certificate
- All good? load and return that tipset, it's the latest that F3 has to offer.
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.
no, sadly this doesn't work:
runtime error: invalid memory address or nil pointer dereference
github.com/filecoin-project/go-jsonrpc.doCall.func1
/home/runner/go/pkg/mod/github.com/filecoin-project/go-jsonrpc@v0.7.0/handler.go:237
runtime.gopanic
/home/runner/work/_tool/go/1.22.7/x64/src/runtime/panic.go:770
runtime.panicmem
/home/runner/work/_tool/go/1.22.7/x64/src/runtime/panic.go:261
runtime.sigpanic
/home/runner/work/_tool/go/1.22.7/x64/src/runtime/signal_unix.go:881
github.com/filecoin-project/lotus/chain/lf3.(*F3).GetManifest
/home/runner/work/lotus/lotus/chain/lf3/f3.go:205
github.com/filecoin-project/lotus/chain/tsresolver.(*tipSetResolver).maybeF3FinalizedTipSet
/home/runner/work/lotus/lotus/chain/tsresolver/tipset_resolver.go:131
So GetManifest
as it exists today isn't safe to call when it hasn't got a manifest. This is in an itest, letting it load the default F3
module. I think maybe it needs to have a check in here for nil
, but what should it return in that case?
Lines 203 to 206 in a46832f
func (fff *F3) GetManifest(ctx context.Context) (*manifest.Manifest, error) { | |
m := fff.inner.Manifest() | |
if m.InitialPowerTable.Defined() { | |
return m, 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.
So we're querying it before the manifest provider can give it a manifest, I think F3#GetManifest
should either return nil, nil
or an error I could errors.Is
on to deal with this case.
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.
GetManifest
should always fall back on returning the static manifest ( which is present at compile time ). If it does not then we should fix that. It seems like a bug.
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.
Yeah, it initialises with its manifest
atomic.Pointer
empty, and it only gets set after Start()
is called and manifestProvider.ManifestUpdates()
successfully sends it a new manifest. https://github.com/filecoin-project/go-f3/blob/6c3d2deca27e361b68680156eef99271d82c29a6/f3.go#L170
f55cd57
to
40bc77c
Compare
FYI this is getting more of a refactor. It's been suggested that (for now at least), we should only expose the differences on a /v2/ API so /v1/ users don't experience any change even when F3 goes live so that it's an opt-in thing. I'll need to do some work to introduce the /v2/ in this PR and try and keep the code duplication minimal, which will be a challenge. |
40bc77c
to
b73c42c
Compare
From CHANGELOG:
lf3.F3API
interface that I can mock and replace the defaultlf3.F3
IsEnabled()
to the F3 interface,lf3.F3
returnstrue
lf3.DisabledF3
implementation that gets loaded when F3 isn't enabled, it returnsfalse
forIsEnabled()
. All methods now have an error return and they'll returnapi.ErrF3Disabled
; so this simplifiesnode/impl/full/f3.go
which just has to pass through the calls instead of checking whetherfff
isnil
.MockF3API
that can be used as a replacement to modify some of its behaviour (focused on the changes in here fore now).TipSetResolver
, which for now, just exposesResolveEthBlockSelector
for the Ethereum APIs. But this will be the extension point for the additional non-eth API work (I'll rebase feat(api): v2 API integrating with F3 #12719 ontop of this). This replaces the eth utility functiongetTipsetByBlockNumber
and integrates with F3.So, now when you resolve
"safe"
or"finalized"
, we first query F3, then:"safe"
= head-30-1,"finalized"
= head-900-1) (we always -1 for the Eth APIs because the queries are transaction-centric and care about transaction execution).@Kubuxu @masih PTAL
@Stebalien I'm pinging you because I wouldn't mind you additional sanity checking on the eth tipset-1 behaviour in here.