-
Notifications
You must be signed in to change notification settings - Fork 32
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
chore: modify verifier to not require eth archive node #241
Changes from all commits
4d85d3b
c4e6c90
1d6cff9
cc3538f
d11a364
dc0b205
9614a4f
ec8d910
4a86753
275acd6
e2d09e5
0739f3b
f255bc8
9c43884
5674628
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
package consts | ||
|
||
// EthHappyPathFinalizationDepth is the number of blocks that must be included on top of a block for it to be considered "final", | ||
// under happy-path aka normal network conditions. | ||
// | ||
// See https://www.alchemy.com/overviews/ethereum-commitment-levels for a quick TLDR explanation, | ||
// or https://eth2book.info/capella/part3/transition/epoch/#finalisation for full details. | ||
var EthHappyPathFinalizationDepthBlocks = uint8(64) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,11 @@ import ( | |
"bytes" | ||
"context" | ||
"fmt" | ||
"math" | ||
"math/big" | ||
"time" | ||
|
||
"github.com/Layr-Labs/eigenda-proxy/common/consts" | ||
"github.com/Layr-Labs/eigenda/api/grpc/disperser" | ||
binding "github.com/Layr-Labs/eigenda/contracts/bindings/EigenDAServiceManager" | ||
|
||
|
@@ -22,14 +24,27 @@ import ( | |
// CertVerifier verifies the DA certificate against on-chain EigenDA contracts | ||
// to ensure disperser returned fields haven't been tampered with | ||
type CertVerifier struct { | ||
l log.Logger | ||
l log.Logger | ||
// ethConfirmationDepth is used to verify that a blob's batch commitment has been bridged to the EigenDAServiceManager contract at least | ||
// this many blocks in the past. To do so we make an eth_call to the contract at the current block_number - ethConfirmationDepth. | ||
// Hence in order to not require an archive node, this value should be kept low. We force it to be < 64 (consts.EthHappyPathFinalizationDepthBlocks). | ||
// waitForFinalization should be used instead of ethConfirmationDepth if the user wants to wait for finality (typically 64 blocks in happy case). | ||
ethConfirmationDepth uint64 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks to me this variable is the upper bound for the punctuality gap. In v2 we don't have to worry about it. Tangential question, how to ensure all proxy set the same parameters, and upgrades. |
||
waitForFinalization bool | ||
manager *binding.ContractEigenDAServiceManagerCaller | ||
ethClient *ethclient.Client | ||
// The two fields below are fetched from the EigenDAServiceManager contract in the constructor. | ||
// They are used to verify the quorums in the received certificates. | ||
// See getQuorumParametersAtLatestBlock for more details. | ||
quorumsRequired []uint8 | ||
quorumAdversaryThresholds map[uint8]uint8 | ||
} | ||
|
||
func NewCertVerifier(cfg *Config, l log.Logger) (*CertVerifier, error) { | ||
if cfg.EthConfirmationDepth >= uint64(consts.EthHappyPathFinalizationDepthBlocks) { | ||
// We keep this low (<128) to avoid requiring an archive node. | ||
litt3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return nil, fmt.Errorf("confirmation depth must be less than 64; consider using cfg.WaitForFinalization=true instead") | ||
} | ||
log.Info("Enabling certificate verification", "confirmation_depth", cfg.EthConfirmationDepth) | ||
|
||
client, err := ethclient.Dial(cfg.RPCURL) | ||
|
@@ -43,11 +58,18 @@ func NewCertVerifier(cfg *Config, l log.Logger) (*CertVerifier, error) { | |
return nil, err | ||
} | ||
|
||
quorumsRequired, quorumAdversaryThresholds, err := getQuorumParametersAtLatestBlock(m) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to fetch quorum parameters from EigenDAServiceManager: %w", err) | ||
} | ||
|
||
return &CertVerifier{ | ||
l: l, | ||
manager: m, | ||
ethConfirmationDepth: cfg.EthConfirmationDepth, | ||
ethClient: client, | ||
l: l, | ||
manager: m, | ||
ethConfirmationDepth: cfg.EthConfirmationDepth, | ||
ethClient: client, | ||
quorumsRequired: quorumsRequired, | ||
quorumAdversaryThresholds: quorumAdversaryThresholds, | ||
}, nil | ||
} | ||
|
||
|
@@ -155,7 +177,10 @@ func (cv *CertVerifier) getConfDeepBlockNumber(ctx context.Context) (*big.Int, e | |
} | ||
|
||
// retrieveBatchMetadataHash retrieves the batch metadata hash stored on-chain at a specific blockNumber for a given batchID | ||
// returns an error if some problem calling the contract happens, or the hash is not found | ||
// returns an error if some problem calling the contract happens, or the hash is not found. | ||
// We make an eth_call to the EigenDAServiceManager at the given blockNumber to retrieve the hash. | ||
// Therefore, make sure that blockNumber is <128 blocks behind the latest block, to avoid requiring an archive node. | ||
litt3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// This is currently enforced by having EthConfirmationDepth be <64. | ||
ethenotethan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
func (cv *CertVerifier) retrieveBatchMetadataHash(ctx context.Context, batchID uint32, blockNumber *big.Int) ([32]byte, error) { | ||
onchainHash, err := cv.manager.BatchIdToBatchMetadataHash(&bind.CallOpts{Context: ctx, BlockNumber: blockNumber}, batchID) | ||
if err != nil { | ||
|
@@ -166,3 +191,44 @@ func (cv *CertVerifier) retrieveBatchMetadataHash(ctx context.Context, batchID u | |
} | ||
return onchainHash, nil | ||
} | ||
|
||
// getQuorumParametersAtLatestBlock fetches the required quorums and quorum adversary thresholds | ||
// from the EigenDAServiceManager contract at the latest block. | ||
// We then cache these parameters and use them in the Verifier to verify the certificates. | ||
// | ||
// Note: this strategy (fetching once and caching) only works because these parameters are immutable. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. immutable per the implementation contract right? IIUC we could technically still upgrade the source proxy contract with a new implementation with alternative bytecode defining different immutable values There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but you're the one asking me to reduce comment cognitive overload :P There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nah not necessary for an explanation - more just calling out a potential risk. i.e if the service manager were to be upgraded arbitrarily to use a different quorum then proxy would have no conception? maybe lets callout in a small issue There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep agree. Thanks for raising this. Will go ahead and merge as is, but let's keep in mind the entire space of interaction with the smart contracts as we go forward, especially for V2 as we try to make the SecurityThresholds upgradeable again. |
||
// They might be different in different environments (e.g. on a devnet or testnet), but they are fixed on a given network. | ||
// We used to allow these parameters to change (via a setter function on the contract), but that then forced us here in the proxy | ||
// to query for these parameters on every request, at the batch's reference block number (RBN). | ||
// This in turn required rollup validators running this proxy to have an archive node, in case the RBN was >128 blocks in the past, | ||
litt3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// which was not ideal. So we decided to make these parameters immutable, and cache them here. | ||
func getQuorumParametersAtLatestBlock( | ||
manager *binding.ContractEigenDAServiceManagerCaller, | ||
) ([]uint8, map[uint8]uint8, error) { | ||
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
litt3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
defer cancel() | ||
requiredQuorums, err := manager.QuorumNumbersRequired(&bind.CallOpts{Context: ctx}) | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("failed to fetch QuorumNumbersRequired from EigenDAServiceManager: %w", err) | ||
} | ||
ctx, cancel = context.WithTimeout(context.Background(), 5*time.Second) | ||
defer cancel() | ||
thresholds, err := manager.QuorumAdversaryThresholdPercentages(&bind.CallOpts{Context: ctx}) | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("failed to fetch QuorumAdversaryThresholdPercentages from EigenDAServiceManager: %w", err) | ||
} | ||
if len(thresholds) > math.MaxUint8 { | ||
ethenotethan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return nil, nil, fmt.Errorf("thresholds received from EigenDAServiceManager contains %d > 256 quorums, which isn't possible", len(thresholds)) | ||
} | ||
var quorumAdversaryThresholds = make(map[uint8]uint8) | ||
for quorumNum, threshold := range thresholds { | ||
quorumAdversaryThresholds[uint8(quorumNum)] = threshold //nolint:gosec // disable G115 // We checked the length of thresholds above | ||
} | ||
// Sanity check: ensure that the required quorums are a subset of the quorums for which we have adversary thresholds | ||
for _, quorum := range requiredQuorums { | ||
if _, ok := quorumAdversaryThresholds[quorum]; !ok { | ||
return nil, nil, fmt.Errorf("required quorum %d does not have an adversary threshold. Was the EigenDAServiceManager properly deployed?", quorum) | ||
} | ||
} | ||
return requiredQuorums, quorumAdversaryThresholds, 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.
knit - this comment is redundant with the error action message below
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.
Yes but I prefer to keep it there. An action typically is not meant for deprecation, so on a first reading without the comment one would read through this entire flag and it would take some brain processing to figure out its deprecated. Typically some redundancy is always good in programming. Also helps catch errors.