feat(vald): moonbeam finality #1224
Conversation
fish-sammy
requested review from
cgorenflo,
haiyizxx and
jcs47
as code owners
fish-sammy
force-pushed
the
feat/moonbeam-finality
branch
2 times, most recently
from
9675cf2
to
3a7cd67
Compare
cmd/axelard/cmd/vald/evm/evm.go
Outdated
| return false | ||
| } | ||
|
|
||
| txBlock, err := rpc.BlockByNumber(context.Background(), txReceipt.BlockNumber) |
There was a problem hiding this comment.
lines 508-523 implement step 4 from here, right? Consensus & Finality | Moonbeam Docs
As as safety check, retrieve the block by number, and verify that the given transaction hash is in the block
This is a linear search through all txs in a block. Are we sure we want to do this for all evm chains?
There was a problem hiding this comment.
Yes, even for evm chains running proof-of-work, this is beneficial in terms of safety. Linear search isn't too bad considering how many evm txs usually fit one block.
| BlockByNumber(ctx context.Context, number *big.Int) (*types.Block, error) | ||
| Close() | ||
|
|
||
| ChainGetFinalizedHead(ctx context.Context) (common.Hash, error) |
There was a problem hiding this comment.
ChainGetFinalizedHead and ChainGetHeader are moonbeam-only methods, right? May wish to add Moonbeam in the name or something to clarify.
There was a problem hiding this comment.
Technically, these chain-prefixed JSON-RPCs are already not supported by any other evm chains other than moonbeam, but I am ok with prefix it further with Moonbeam. So MoonbeamChainGetFinalizedHead and MoonbeamChainGetHeader?
| _, err = client.ChainGetFinalizedHead(context.Background()) | ||
| switch err := err.(type) { | ||
| case nil: | ||
| client.isMoonbeam = true |
There was a problem hiding this comment.
Might be dangerous to infer moonbeam from non-nil error. Is moonbeam the only chain that will return a non-nil error here? Maybe we'll add other chains in the future that support the chain_getFinalizedHead rpc.
There was a problem hiding this comment.
It's currently moonbeam only and these RPCs are no standard whatsoever. It's better to be GRANDPA instead of moonbeam but we are not sure if we are gonna deal with any GRANDPA chain in the future and even if we do we don't know if they are gonna implement these non-standard JSON-RPCs.
There was a problem hiding this comment.
If there's some other chain later supports a non-standard JSON-RPC like chain_getFinalizedHead, it's highly likely they are a GRANDPA chain and the finality process remains working. In the highly unlikely case for a proof-of-work evm chain to support chain_getFinalizedHead, as long as this method still means the same thing, then we also won't have any problem with it.
fish-sammy
force-pushed
the
feat/moonbeam-finality
branch
from
3a7cd67
to
d6c9b57
Compare
fish-sammy
force-pushed
the
feat/moonbeam-finality
branch
from
d6c9b57
to
9434356
Compare
cmd/axelard/cmd/vald/evm/evm.go
Outdated
| } | ||
|
|
||
| return big.NewInt(int64(blockNumber - confHeight + 1)), nil |
There was a problem hiding this comment.
cuz having 1 confirmation means that the tx is included in the latest block.
| BlockByNumber(ctx context.Context, number *big.Int) (*types.Block, error) | ||
| Close() | ||
|
|
||
| ChainGetFinalizedHead(ctx context.Context) (common.Hash, error) | ||
| ChainGetHeader(ctx context.Context, hash common.Hash) (*MoonbeamHeader, error) | ||
| IsMoonbeam() bool |
There was a problem hiding this comment.
| BlockByNumber(ctx context.Context, number *big.Int) (*types.Block, error) | |
| Close() | |
| ChainGetFinalizedHead(ctx context.Context) (common.Hash, error) | |
| ChainGetHeader(ctx context.Context, hash common.Hash) (*MoonbeamHeader, error) | |
| IsMoonbeam() bool | |
| BlockByNumber(ctx context.Context, number *big.Int) (*types.Block, error) | |
| Close() | |
| type MoonbeamClient interface { | |
| Client | |
| ChainGetFinalizedHead(ctx context.Context) (common.Hash, error) | |
| ChainGetHeader(ctx context.Context, hash common.Hash) (*MoonbeamHeader, error) |
And instead of an isMoonbeam function, you will however have to switch on the network name that is given by the config file to know which constructor to use
| url string | ||
| isMoonbeam bool |
There was a problem hiding this comment.
Similarly
| url string | |
| isMoonbeam bool | |
| } | |
| type MoonbeamClientImpl struct { | |
| ClientImpl | |
| url string | |
| } |
Description
Todos
Steps to Test
Expected Behaviour
Other Notes