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: eots-manager add eots verifier #360

Open
wants to merge 13 commits into
base: base/consumer-chain-support
Choose a base branch
from

Conversation

lesterli
Copy link

@lesterli lesterli commented May 28, 2024

Summary

Add a new daemon eots-verifier that provides one gRPC API VerifyFinalitySig, the user can use it to do EOTS verification for one tx hash.

Test Plan

  1. Build: run go build -o build/eots-verifier eots-verifier/cmd/main.go
  2. Init: run eots-verifier init --home <home_dir>
  3. Config: edit the config file <home_dir>/eots-verifier.conf
  4. Start: run eots-verifier start --home <home_dir>

TODO:

  • integration test with op-node, Babylon node, EOTS aggregator

@SebastianElvis
Copy link
Member

  • What's the relationship between this EOTS verifier and the EOTS verifier at https://github.com/babylonchain/babylon-evm-contracts/pull/8? I was under the impression that verification only takes place at EOTS aggregator and L1/L2 contract
  • Why is EOTS manager providing this functionality? The EOTS manager here aims to manage the Schnorr/EOTS secret keys and secret randomness, rather than being a RPC service for verifying EOTS signatures

@lesterli
Copy link
Author

  1. It is considered an off-chain verifier that any rollup users can run locally to verify the EOTS signatures themselves to achieve fast confirmation. And the contract in babylon-evm-contracts is the on-chain verifier.
  2. We originally thought to use the EOTSManager codebase to reuse some code. If it is confused, we can split it as a separate daemon. Wdyt?

@SebastianElvis
Copy link
Member

  1. It is considered an off-chain verifier that any rollup users can run locally to verify the EOTS signatures themselves to achieve fast confirmation. And the contract in babylon-evm-contracts is the on-chain verifier.

I see. Correct me if I'm wrong, this verifier will be a part of the EOTS aggregator?

  1. We originally thought to use the EOTSManager codebase to reuse some code. If it is confused, we can split it as a separate daemon. Wdyt?

Yep let's move the functionality to another daemon program. In fact, I feel it might not be a part of the finality-provider repository as this targets different groups of users. If it's used in EOTS aggregator, how about we put it in the repository for EOTS aggregator?

@lesterli
Copy link
Author

I see. Correct me if I'm wrong, this verifier will be a part of the EOTS aggregator?

In our design, it doesn't do the verification in the EOTS aggregator.

If it's used in EOTS aggregator, how about we put it in the repository for EOTS aggregator?

The EOTS aggregator doesn't use it, but I think we can put this gRPC in the EOTS aggregator repo.

@bap2pecs Wdyt?

@bap2pecs
Copy link
Contributor

bap2pecs commented May 29, 2024

In fact, I feel it might not be a part of the finality-provider repository as this targets different groups of users

@SebastianElvis I think this part makes sense. so it will be confusing if the user of the chain has to run a program in the FP repo. I am in for the move as long as we don't need to share lots of code from this FP repo.

I am fine to create a new repo finality-verifier or just another daemon program within the same repo

I think a daemon should work b/c it's technically still “finality" related. and the benefit is we can reuse the CI / test frameworks here

@lesterli
Copy link
Author

I think we can put this gRPC in the EOTS aggregator repo.

Correct it.

Their target user is different, especially if the aggregator functions as a P2P network.

I am fine to create a new repo finality-verifier or just another daemon program within the same repo

It makes sense.

@SebastianElvis
Copy link
Member

Yep having a new daemon for EOTS verifier sounds good to me. @gitferry wdyt?

@bap2pecs
Copy link
Contributor

rollup users will be running EOTS verifier

EOTS aggregator is run by a diff group of people

but for you q offline, EOTS aggregator will still verify the EOTS before storing in its database

@gitferry
Copy link
Contributor

Had an offline discussion with @SebastianElvis. A separate daemon for eots-verifier under the finality-provider repo sounds good to me 👍


const (
homeFlag = "home"
forceFlag = "force"
Copy link
Contributor

Choose a reason for hiding this comment

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

what's force?

Comment on lines 21 to 22
defaultRollupRPC = "http://127.0.0.1:8545"
defaultRollupChainId = "42069"
Copy link
Contributor

Choose a reason for hiding this comment

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

called rollup is too specific. we should call it defaultConsumerChainRPC

Copy link
Contributor

Choose a reason for hiding this comment

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

also default values should be sth that can work most of the time

but it's almost certain that the last 4 values should be changed when we run the daemon. so we should just remove those and only rely on the config file

RpcListener string `long:"rpclistener" description:"the listener for RPC connections, e.g., 127.0.0.1:1234"`
BabylonRPC string `long:"babylon_rpc" description:"connect to the Babylon RPC service"`
EotsAggRPC string `long:"eots_agg_rpc" description:"connect to the EOTS Aggregator RPC service"`
RollupRPC string `long:"rollup_rpc" description:"connect to the Rollup RPC service"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Rollup -> consumer


message VerifyFinalitySigRequest {
// block_number is the block which need to be verified
uint64 block_number = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

per my comments below, we need to also pass in the tx hash to be verified

also per design 3 in https://www.notion.so/snapchain/Security-Requirements-1076b96c44144e82a61b4ed393364399, we might ask FP to sign a range of blocks. so instead of verify

func (s *Server) VerifyFinalitySig(ctx context.Context, req *proto.VerifyFinalitySigRequest) (
*proto.VerifyFinalitySigResponse, error) {
height := req.BlockNumber
l2Block, err := s.rollupClient.BlockByNumber(ctx, new(big.Int).SetUint64(height))
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we shouldn't trust getting the data from the RPC. because we can't guarantee the RPC returns the same block that contains the same tx as the block the user accepts

let's think of the goal of the verifier program: to help a user to build up confidence that a tx he observes is in a block that has enough valid EOTS sigs

so the minimum input should be the tx hash. it has to be something that doesn't allow you to change this tx. block number itself doesn't provide the guarantee.

the flow should be:

  • user obtains the tx hash
  • user queries the RPC to get the block that contains the tx
  • user gets all data (e.g. tx list) from the RPC
  • user gets the signatures from the aggregator
  • user gets the FP VP ...
  • ...
  • user verifies the EOTS signatures

add block number can ensure more security: to help a user to build up confidence that a tx he observes is in a specific block that has enough valid EOTS sigs

the flow should be:

  • user obtains the tx hash
  • user queries the RPC to get the block that contains the tx
  • user check if the block number matches
  • user gets all data (e.g. tx list) from the RPC
  • user gets the signatures from the aggregator
  • user gets the FP VP ...
  • ...
  • user verifies the EOTS signatures

return bbnRes.FinalityProviders, totalPower, nil
}

func (s *Server) calculateValidPower(ctx context.Context, msg []byte, fpList []*bsctypes.FinalityProviderResponse, eotsInfos []*eotsservice.EOTSInfoResponse) (uint64, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

calculateTotalPowerFromEotsVerifiedFps

Comment on lines 240 to 241
for _, fp := range fpList {
for _, info := range eotsInfos {
Copy link
Contributor

Choose a reason for hiding this comment

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

why are there two levels of for loops?

@SebastianElvis SebastianElvis force-pushed the base/consumer-chain-support branch from 623964e to 2cc4a79 Compare June 5, 2024 12:18
@bap2pecs
Copy link
Contributor

bap2pecs commented Jul 7, 2024

Had an offline discussion with @SebastianElvis. A separate daemon for eots-verifier under the finality-provider repo sounds good to me 👍

per my conversation w @SebastianElvis recently, it's probably better to put this in the https://github.com/babylonchain/babylon-da-sdk repo, making it kind of like a demo case for using the SDK as well

If there is some code sharing needed, we can always import from this FP repo

@SebastianElvis
Copy link
Member

Yes, putting the EOTS verifier under a new folder of the babylon-da-sdk repo seems to be a proper way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants