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

feat(rfq-relayer): wait for finality before proving #3062

Merged
merged 18 commits into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
2 changes: 1 addition & 1 deletion services/rfq/guard/guardconfig/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@
for chainID, chainCfg := range relayerCfg.GetChains() {
cfg.Chains[chainID] = ChainConfig{
RFQAddress: chainCfg.RFQAddress,
Confirmations: chainCfg.Confirmations,
Confirmations: chainCfg.FinalityConfirmations,

Check warning on line 119 in services/rfq/guard/guardconfig/config.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/guardconfig/config.go#L119

Added line #L119 was not covered by tests
}
}
return cfg
Expand Down
4 changes: 4 additions & 0 deletions services/rfq/guard/guarddb/base/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@
TxHash string
// Status is the status of the event
Status guarddb.PendingProvenStatus
// BlockNumber is the block number of the event
BlockNumber uint64
}

// FromPendingProven converts a quote request to an object that can be stored in the db.
Expand All @@ -52,6 +54,7 @@
TransactionID: hexutil.Encode(proven.TransactionID[:]),
TxHash: proven.TxHash.Hex(),
Status: proven.Status,
BlockNumber: proven.BlockNumber,

Check warning on line 57 in services/rfq/guard/guarddb/base/model.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/guarddb/base/model.go#L57

Added line #L57 was not covered by tests
}
}

Expand All @@ -73,6 +76,7 @@
TransactionID: transactionID,
TxHash: common.HexToHash(p.TxHash),
Status: p.Status,
BlockNumber: p.BlockNumber,

Check warning on line 79 in services/rfq/guard/guarddb/base/model.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/guarddb/base/model.go#L79

Added line #L79 was not covered by tests
}, nil
}

Expand Down
1 change: 1 addition & 0 deletions services/rfq/guard/guarddb/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ type PendingProven struct {
TransactionID [32]byte
TxHash common.Hash
Status PendingProvenStatus
BlockNumber uint64
}

// PendingProvenStatus is the status of a quote request in the db.
Expand Down
42 changes: 41 additions & 1 deletion services/rfq/guard/service/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
TransactionID: event.TransactionId,
TxHash: event.TransactionHash,
Status: guarddb.ProveCalled,
BlockNumber: event.Raw.BlockNumber,

Check warning on line 85 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L85

Added line #L85 was not covered by tests
}
err = g.db.StorePendingProven(ctx, proven)
if err != nil {
Expand Down Expand Up @@ -115,7 +116,17 @@
metrics.EndSpanWithErr(span, err)
}()

// first, get the corresponding bridge request
// first, verify that the prove tx is finalized
finalized, err := g.isFinalized(ctx, int(proven.Origin), proven.BlockNumber)
if err != nil {
return fmt.Errorf("could not check if tx is finalized: %w", err)
}
span.SetAttributes(attribute.Bool("finalized", finalized))
if !finalized {
return nil
}

Check warning on line 127 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L120-L127

Added lines #L120 - L127 were not covered by tests

// get the corresponding bridge request
bridgeRequest, err := g.db.GetBridgeRequestByID(ctx, proven.TransactionID)
if err != nil {
return fmt.Errorf("could not get bridge request for txid %s: %w", hexutil.Encode(proven.TransactionID[:]), err)
Expand Down Expand Up @@ -242,3 +253,32 @@
}
return true
}

// isFinalized checks if a transaction is finalized versus the configured confirmations threshold.
func (g *Guard) isFinalized(ctx context.Context, chainID int, txBlockNumber uint64) (bool, error) {
span := trace.SpanFromContext(ctx)

client, err := g.client.GetChainClient(ctx, chainID)
if err != nil {
return false, fmt.Errorf("could not get chain client: %w", err)
}

Check warning on line 264 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L258-L264

Added lines #L258 - L264 were not covered by tests

currentBlockNumber, err := client.BlockNumber(ctx)
if err != nil {
return false, fmt.Errorf("could not get block number: %w", err)
}

Check warning on line 269 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L266-L269

Added lines #L266 - L269 were not covered by tests

chainCfg, ok := g.cfg.Chains[chainID]
if !ok {
return false, fmt.Errorf("could not get chain config for chain %d", chainID)
}
threshBlockNumber := txBlockNumber + chainCfg.Confirmations
span.SetAttributes(
attribute.Int("chain_id", chainID),
attribute.Int("current_block_number", int(currentBlockNumber)),
attribute.Int("tx_block_number", int(txBlockNumber)),
attribute.Int("confirmations", int(chainCfg.Confirmations)),
)

return currentBlockNumber >= threshBlockNumber, nil

Check warning on line 283 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L271-L283

Added lines #L271 - L283 were not covered by tests
}
2 changes: 2 additions & 0 deletions services/rfq/relayer/relconfig/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ type ChainConfig struct {
RFQAddress string `yaml:"rfq_address"`
// Confirmations is the number of required confirmations.
Confirmations uint64 `yaml:"confirmations"`
// FinalityConfirmations is the number of required confirmations before proving.
FinalityConfirmations uint64 `yaml:"prove_confirmations"`
// Tokens is a map of token name -> token config.
Tokens map[string]TokenConfig `yaml:"tokens"`
// NativeToken is the native token of the chain (pays gas).
Expand Down
14 changes: 14 additions & 0 deletions services/rfq/relayer/relconfig/getters.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,20 @@
return value, nil
}

// GetFinalityConfirmations returns the FinalityConfirmations for the given chainID.
func (c Config) GetFinalityConfirmations(chainID int) (value uint64, err error) {
rawValue, err := c.getChainConfigValue(chainID, "FinalityConfirmations")
if err != nil {
return value, err
}

Check warning on line 223 in services/rfq/relayer/relconfig/getters.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relconfig/getters.go#L219-L223

Added lines #L219 - L223 were not covered by tests

value, ok := rawValue.(uint64)
if !ok {
return value, fmt.Errorf("failed to cast FinalityConfirmations to int")
}
return value, nil

Check warning on line 229 in services/rfq/relayer/relconfig/getters.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relconfig/getters.go#L225-L229

Added lines #L225 - L229 were not covered by tests
}
Comment on lines +219 to +230
Copy link
Contributor

Choose a reason for hiding this comment

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

Add unit tests for the new function.

The function should include unit tests to ensure its correctness.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 221-223: services/rfq/relayer/relconfig/getters.go#L221-L223
Added lines #L221 - L223 were not covered by tests


[warning] 225-226: services/rfq/relayer/relconfig/getters.go#L225-L226
Added lines #L225 - L226 were not covered by tests


[warning] 228-229: services/rfq/relayer/relconfig/getters.go#L228-L229
Added lines #L228 - L229 were not covered by tests


Correct the error message in the type assertion failure.

The error message should mention uint64 instead of int.

- return value, fmt.Errorf("failed to cast FinalityConfirmations to int")
+ return value, fmt.Errorf("failed to cast FinalityConfirmations to uint64")
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (c Config) GetFinalityConfirmations(chainID int) (value uint64, err error) {
rawValue, err := c.getChainConfigValue(chainID, "FinalityConfirmations")
if err != nil {
return value, err
}
value, ok := rawValue.(uint64)
if !ok {
return value, fmt.Errorf("failed to cast FinalityConfirmations to int")
}
return value, nil
}
func (c Config) GetFinalityConfirmations(chainID int) (value uint64, err error) {
rawValue, err := c.getChainConfigValue(chainID, "FinalityConfirmations")
if err != nil {
return value, err
}
value, ok := rawValue.(uint64)
if !ok {
return value, fmt.Errorf("failed to cast FinalityConfirmations to uint64")
}
return value, nil
}
Tools
GitHub Check: codecov/patch

[warning] 221-223: services/rfq/relayer/relconfig/getters.go#L221-L223
Added lines #L221 - L223 were not covered by tests


[warning] 225-226: services/rfq/relayer/relconfig/getters.go#L225-L226
Added lines #L225 - L226 were not covered by tests


[warning] 228-229: services/rfq/relayer/relconfig/getters.go#L228-L229
Added lines #L228 - L229 were not covered by tests


// GetNativeToken returns the NativeToken for the given chainID.
func (c Config) GetNativeToken(chainID int) (value string, err error) {
rawValue, err := c.getChainConfigValue(chainID, "NativeToken")
Expand Down
54 changes: 52 additions & 2 deletions services/rfq/relayer/service/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -378,8 +378,28 @@
// Step 6: ProvePosting
//
// This is the sixth step in the bridge process. Here we submit the claim transaction to the origin chain.
func (q *QuoteRequestHandler) handleRelayCompleted(ctx context.Context, _ trace.Span, request reldb.QuoteRequest) (err error) {
// relays been completed, it's time to go back to the origin chain and try to prove
func (q *QuoteRequestHandler) handleRelayCompleted(ctx context.Context, span trace.Span, request reldb.QuoteRequest) (err error) {
relayBlockNumber, err := q.getRelayBlockNumber(ctx, request)
if err != nil {
return fmt.Errorf("could not get relay block number: %w", err)
}

Check warning on line 385 in services/rfq/relayer/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/service/handlers.go#L381-L385

Added lines #L381 - L385 were not covered by tests
currentBlockNumber := q.Origin.LatestBlock()
proveConfirmations, err := q.cfg.GetFinalityConfirmations(int(q.Dest.ChainID))
if err != nil {
return fmt.Errorf("could not get prove confirmations: %w", err)
}

span.SetAttributes(
attribute.Int("current_block_number", int(currentBlockNumber)),
attribute.Int("relay_block_number", int(relayBlockNumber)),
attribute.Int("prove_confirmations", int(proveConfirmations)),
)
if currentBlockNumber < relayBlockNumber+proveConfirmations {
span.AddEvent("not enough confirmations")
return nil
}

Check warning on line 401 in services/rfq/relayer/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/service/handlers.go#L392-L401

Added lines #L392 - L401 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM! Ensure tests cover the new logic.

The changes improve the reliability of the proof submission process by ensuring that it only occurs when the blockchain state is sufficiently advanced. Ensure that the new logic is adequately tested.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

// relay has been finalized, it's time to go back to the origin chain and try to prove
Copy link
Collaborator

Choose a reason for hiding this comment

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

@dwasse we need to do a check similar to Guard's isProveValid here to handle the case where our relay tx was reorged.

In the event where we submitted a tx, witnessed a BridgeRelayed event, and later on this tx was reorged and ended up being reverted, we need a way for the Relayer to mark the tx as RelayRaceLost here (I'm not sure if we can safely assume that we will index the correct event with another relayer before that).

The current code will still get a valid receipt for DestTxHash, it's just the receipt will not have a corresponding BridgeRelayed event there.

_, err = q.Origin.SubmitTransaction(ctx, func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) {
tx, err = q.Origin.Bridge.Prove(transactor, request.RawRequest, request.DestTxHash)
if err != nil {
Expand All @@ -399,6 +419,36 @@
return nil
}

// getRelayBlockNumber fetches the block number of the relay transaction for a given quote request.
func (q *QuoteRequestHandler) getRelayBlockNumber(ctx context.Context, request reldb.QuoteRequest) (blockNumber uint64, err error) {
// fetch the transaction receipt for corresponding tx hash

Check warning on line 424 in services/rfq/relayer/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/service/handlers.go#L422-L424

Added lines #L422 - L424 were not covered by tests
receipt, err := q.Dest.Client.TransactionReceipt(ctx, request.DestTxHash)
if err != nil {
return blockNumber, fmt.Errorf("could not get transaction receipt: %w", err)
}
parser, err := fastbridge.NewParser(q.Dest.Bridge.Address())
if err != nil {

Check warning on line 430 in services/rfq/relayer/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/service/handlers.go#L426-L430

Added lines #L426 - L430 were not covered by tests
return blockNumber, fmt.Errorf("could not create parser: %w", err)
}

// check that a Relayed event was emitted
for _, log := range receipt.Logs {
if log == nil {
continue
}
_, parsedEvent, ok := parser.ParseEvent(*log)
if !ok {
continue
}
_, ok = parsedEvent.(*fastbridge.FastBridgeBridgeRelayed)

Check warning on line 443 in services/rfq/relayer/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/service/handlers.go#L434-L443

Added lines #L434 - L443 were not covered by tests
if ok {
return receipt.BlockNumber.Uint64(), nil
}
}

Check warning on line 448 in services/rfq/relayer/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/service/handlers.go#L446-L448

Added lines #L446 - L448 were not covered by tests
return blockNumber, fmt.Errorf("relayed event not found for dest tx hash: %s", request.DestTxHash.Hex())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM! Consider adding error handling for potential reorgs.

The function encapsulates the logic for fetching the block number of the relay transaction, improving code modularity and readability. Consider adding error handling for potential reorgs to ensure robustness.

Add error handling for potential reorgs.


Check warning on line 451 in services/rfq/relayer/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/service/handlers.go#L450-L451

Added lines #L450 - L451 were not covered by tests
// handleProofProvided handles the ProofProvided event emitted by the Bridge.
// Step 7: ProvePosted
//
Expand Down
4 changes: 4 additions & 0 deletions services/rfq/relayer/service/statushandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"github.com/synapsecns/sanguine/services/rfq/relayer/chain"
"github.com/synapsecns/sanguine/services/rfq/relayer/inventory"
"github.com/synapsecns/sanguine/services/rfq/relayer/quoter"
"github.com/synapsecns/sanguine/services/rfq/relayer/relconfig"
"github.com/synapsecns/sanguine/services/rfq/relayer/reldb"
"github.com/synapsecns/sanguine/services/rfq/util"
"go.opentelemetry.io/otel/attribute"
Expand Down Expand Up @@ -54,6 +55,8 @@
handlerMtx mapmutex.StringMapMutex
// balanceMtx is the mutex for balances.
balanceMtx mapmutex.StringMapMutex
// cfg is the relayer config.
cfg relconfig.Config
}

func getBalanceMtxKey(chainID uint32, token common.Address) string {
Expand Down Expand Up @@ -88,6 +91,7 @@
mutexMiddlewareFunc: r.mutexMiddleware,
handlerMtx: r.handlerMtx,
balanceMtx: r.balanceMtx,
cfg: r.cfg,

Check warning on line 94 in services/rfq/relayer/service/statushandler.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/service/statushandler.go#L94

Added line #L94 was not covered by tests
}

// wrap in deadline middleware since the relay has not yet happened
Expand Down
Loading