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.

Sign up for GitHub

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: allow FG to be opt-in #17

Merged
merged 2 commits into from
Nov 4, 2024
Merged

feat: allow FG to be opt-in #17

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39dbb93
allow opt-in
bap2pecs Nov 2, 2024
ddf8967
update codeowners
bap2pecs Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 1 addition & 43 deletions .github/CODEOWNERS
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,43 +1 @@
# Packages
/packages/contracts-bedrock @ethereum-optimism/contract-reviewers
/packages/sdk @ethereum-optimism/devxpod

# Bedrock codebases
/bedrock-devnet @ethereum-optimism/go-reviewers
/cannon @ethereum-optimism/go-reviewers
/op-batcher @ethereum-optimism/go-reviewers
/op-bootnode @ethereum-optimism/go-reviewers
/op-chain-ops @ethereum-optimism/go-reviewers
/op-challenger @ethereum-optimism/go-reviewers
/op-dispute-mon @ethereum-optimism/go-reviewers
/op-e2e @ethereum-optimism/go-reviewers
/op-node @ethereum-optimism/go-reviewers
/op-node/rollup @protolambda @ajsutton
/op-alt-da @ethereum-optimism/go-reviewers
/op-preimage @ethereum-optimism/go-reviewers
/op-program @ethereum-optimism/go-reviewers
/op-proposer @ethereum-optimism/go-reviewers
/op-service @ethereum-optimism/go-reviewers
/op-supervisor @ethereum-optimism/go-reviewers
/op-wheel @ethereum-optimism/go-reviewers
/ops-bedrock @ethereum-optimism/go-reviewers
/op-conductor @0x00101010 @zhwrd @mslipper

# Ops
/.circleci @ethereum-optimism/monorepo-ops-reviewers
/.github @ethereum-optimism/monorepo-ops-reviewers
/ops @ethereum-optimism/monorepo-ops-reviewers
/docker-bake.hcl @ethereum-optimism/monorepo-ops-reviewers

# Misc
/proxyd @ethereum-optimism/infra-reviewers
/infra @ethereum-optimism/infra-reviewers
/specs @ethereum-optimism/contract-reviewers @ethereum-optimism/go-reviewers

# Don't add owners if only package.json is updated
/packages/*/package.json
/*/package.json

# JavaScript Releases
/packages/*/CHANGELOG.md @ethereum-optimism/release-managers
/*/CHANGELOG.md @ethereum-optimism/release-managers
* @bap2pecs @lesterli @parketh
74 changes: 45 additions & 29 deletions op-node/rollup/finality/finalizer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,19 @@ type Finalizer struct {
func NewFinalizer(ctx context.Context, log log.Logger, cfg *rollup.Config, l1Fetcher FinalizerL1Interface, l2Fetcher FinalizerL2Interface) *Finalizer {
lookback := calcFinalityLookback(cfg)

// Initialize the Babylon finality gadget client
log.Debug(
"creating Babylon Finality client",
"rpc_addr", cfg.BabylonFinalityGadgetRpc,
)
babylonFinalityClient, err := fgclient.NewFinalityGadgetGrpcClient(cfg.BabylonFinalityGadgetRpc)
if err != nil {
log.Error("failed to initialize Babylon Finality client", "error", err)
return nil
var babylonFinalityClient IFinalityGadgetClient
var err error
if cfg.BabylonFinalityGadgetRpc != "" {
// Initialize the Babylon finality gadget client
log.Debug(
"creating Babylon Finality client",
"rpc_addr", cfg.BabylonFinalityGadgetRpc,
)
babylonFinalityClient, err = fgclient.NewFinalityGadgetGrpcClient(cfg.BabylonFinalityGadgetRpc)
if err != nil {
log.Error("failed to initialize Babylon Finality client", "error", err)
return nil
}
}

return &Finalizer{
Expand Down Expand Up @@ -231,37 +235,49 @@ func (fi *Finalizer) tryFinalize() {
fi.mu.Lock()
defer fi.mu.Unlock()

gadgetActivatedTimestamp, err := fi.babylonFinalityClient.QueryBtcStakingActivatedTimestamp()
if err != nil && !strings.Contains(err.Error(), fgtypes.ErrBtcStakingNotActivated.Error()) {
fi.emitter.Emit(rollup.CriticalErrorEvent{Err: fmt.Errorf("failed to query BTC staking activated timestamp: %w", err)})
return
}

// overwritten if we finalize
finalizedL2 := fi.lastFinalizedL2 // may be zeroed if nothing was finalized since startup.
var finalizedDerivedFrom eth.BlockID

// go through the latest inclusion data, and find the last L2 block that was derived from a finalized L1 block
fi.log.Debug("try finalize", "finality_data", fi.finalityData, "last_finalized_l2", finalizedL2)
for _, fd := range fi.finalityData {
if fd.L2Block.Number > finalizedL2.Number && fd.L1Block.Number <= fi.finalizedL1.Number {
lastFinalizedBlock := fi.findLastBtcFinalizedL2Block(
fd.L2Block.Number, finalizedL2.Number, gadgetActivatedTimestamp)

// set finalized block(s)
if lastFinalizedBlock != nil {
finalizedL2 = *lastFinalizedBlock
if fi.babylonFinalityClient == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if fi.babylonFinalityClient == nil {
if fi.EnableBTCStaking() {

and we can wrap this logic inside the EnableBTCStaking function

Copy link
Member

Choose a reason for hiding this comment

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

Also what will happen if an OP node enables BTC staking and run OP node for a while, shuts down OP node, and restarts OP node with BTC staking disabled? Given that this function tries to write finalised block to DB, would it be possible that the finaliser needs to iterate over more blocks after the restart?

This seems to be another reason of keeping BTC stake finalised in a separate KV store, in parallel with unsafe/safe/finalised states of blocks in OP stack

Copy link
Author

Choose a reason for hiding this comment

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

what will happen if an OP node enables BTC staking and run OP node for a while, shuts down OP node, and restarts OP node with BTC staking disabled

in this case, once it's restarted, it will continue to operate normally (e.g. fetch L1 data, derive L2 blocks). when it tries to finalise a block, it will simply skip the BTC quorum check.

would it be possible that the finaliser needs to iterate over more blocks after the restart

what do you mean by itereating over more blocks? after the restart, it won't call the gRPC of FG. it will become a vanilla op-node

This seems to be another reason of keeping BTC stake finalised in a separate KV store, in parallel with unsafe/safe/finalised states of blocks in OP stack

this data is already stored in the FG KV db. op-node connects with it via gRPC

Copy link
Author

Choose a reason for hiding this comment

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

and we can wrap this logic inside the EnableBTCStaking function

@SebastianElvis do you mean to create a helper function like this?

func (fi *Finalizer) BTCStakingEnabled() bool {
	return fi.babylonFinalityClient != nil
}

this doesn't seem very useful

Copy link
Member

Choose a reason for hiding this comment

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

what do you mean by itereating over more blocks? after the restart, it won't call the gRPC of FG. it will become a vanilla op-node

Let's say block h is finalised in OP stack term + BTC stake finalised, and h+1 to h+5 are finalised in OP stack term but haven't been BTC stake finalised. Then upon retsart with BTC staking disabled, Op node starts iterating from h+1 but not h+5. However it's supposed to start from h+5.

Copy link
Author

Choose a reason for hiding this comment

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

got it. it's fine to re-derive from h1. although there will be some wasted rpc calls to L1, it doesn't require any changes in the codebase and such restart should be rare

Copy link
Member

Choose a reason for hiding this comment

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

yep. not blocking for now but wdyt about having some doc / TODO for this?

Copy link
Author

Choose a reason for hiding this comment

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

there isn't any TODO here as the optimization has low ROI. (i.e. it requires intrusive changes for marginally gain)

i'd say not adding any comments to avoid confusion

for _, fd := range fi.finalityData {
if fd.L2Block.Number > finalizedL2.Number && fd.L1Block.Number <= fi.finalizedL1.Number {
finalizedL2 = fd.L2Block
finalizedDerivedFrom = fd.L1Block
fi.log.Debug("set finalized block", "finalized_l2", finalizedL2, "finalized_derived_from", finalizedDerivedFrom, "fd_l2_block", fd.L2Block)
// keep iterating, there may be later L2 blocks that can also be finalized
}
}
} else {
gadgetActivatedTimestamp, err := fi.babylonFinalityClient.QueryBtcStakingActivatedTimestamp()
if err != nil && !strings.Contains(err.Error(), fgtypes.ErrBtcStakingNotActivated.Error()) {
fi.emitter.Emit(rollup.CriticalErrorEvent{Err: fmt.Errorf("failed to query BTC staking activated timestamp: %w", err)})
return
}

// some blocks in the queried range is not BTC finalized, stop iterating to honor consecutive quorom
if lastFinalizedBlock == nil || lastFinalizedBlock.Number != fd.L2Block.Number {
break
}
for _, fd := range fi.finalityData {
if fd.L2Block.Number > finalizedL2.Number && fd.L1Block.Number <= fi.finalizedL1.Number {
lastFinalizedBlock := fi.findLastBtcFinalizedL2Block(
fd.L2Block.Number, finalizedL2.Number, gadgetActivatedTimestamp)

// keep iterating, there may be later L2 blocks that can also be finalized
// set finalized block(s)
if lastFinalizedBlock != nil {
finalizedL2 = *lastFinalizedBlock
finalizedDerivedFrom = fd.L1Block
fi.log.Debug("set finalized block", "finalized_l2", finalizedL2, "finalized_derived_from", finalizedDerivedFrom, "fd_l2_block", fd.L2Block)
}

// some blocks in the queried range is not BTC finalized, stop iterating to honor consecutive quorom
if lastFinalizedBlock == nil || lastFinalizedBlock.Number != fd.L2Block.Number {
break
}

// keep iterating, there may be later L2 blocks that can also be finalized
}
}
}

if finalizedDerivedFrom != (eth.BlockID{}) {
ctx, cancel := context.WithTimeout(fi.ctx, time.Second*10)
defer cancel()
Expand Down
Loading