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

Disperser auth innabox #1052

Merged
merged 108 commits into from
Jan 10, 2025
Merged

Conversation

cody-littley
Copy link
Contributor

Why are these changes needed?

Enable StoreChunks() signing in inabox tests.

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
@cody-littley cody-littley marked this pull request as ready for review January 8, 2025 14:52
Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

lgtm!


keyManager := kms.New(kms.Options{
Region: "us-east-1",
BaseEndpoint: aws.String("http://localhost:4570"), // TODO don't hard code this
Copy link
Contributor

Choose a reason for hiding this comment

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

can we do this in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, was my intention but I forgot. Now fixed.

Signed-off-by: Cody Littley <[email protected]>
@@ -6,6 +6,9 @@ import (
"encoding/hex"
"encoding/json"
"fmt"
"github.com/Layr-Labs/eigenda/api"
contractEigenDADisperserRegistry "github.com/Layr-Labs/eigenda/contracts/bindings/EigenDADisperserRegistry"
Copy link
Contributor

Choose a reason for hiding this comment

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

use lowecase for alias

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to dreg

return fmt.Errorf("failed to set disperser address: %w", err)
}

return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to wait for txn to be mined?

Copy link
Contributor Author

@cody-littley cody-littley Jan 9, 2025

Choose a reason for hiding this comment

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

I've added a retry loop in the function that calls this function. It will now wait for up to a minute if the disperser address is not available on chain.

        // Read the disperser's public key from on-chain storage to verify it was written correctly
	
	retryTimeout := time.Now().Add(1 * time.Minute)
	ticker := time.NewTicker(1 * time.Second)
	
	for time.Now().Before(retryTimeout) {
		address, err := writer.GetDisperserAddress(context.Background(), 0)
		if err != nil {
			logger.Warnf("could not get disperser address: %v", err)
		} else {
			if address != env.DisperserAddress {
				return fmt.Errorf("expected disperser address %s, got %s", env.DisperserAddress, address)
			}
			return nil
		}
		
		<- ticker.C
	}

	return fmt.Errorf("timed out waiting for disperser address to be set")

Copy link
Contributor

Choose a reason for hiding this comment

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

Would EstimateGasPriceAndLimitAndSendTx in the ethclient work cleaner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This caused errors. I initially tried to use this method, but later in the test run I'd get failures with the following error message:

{
   "time":"2025-01-03T16:28:01.407636692Z",
   "level":"ERROR",
   "source":{
      "function":"github.com/Layr-Labs/eigenda/core/eth.(*Reader).GetOperatorStakesForQuorums",
      "file":"/home/runner/work/eigenda/eigenda/core/eth/reader.go",
      "line":437
   },
   "msg":"Failed to fetch operator state",
   "component":"Reader",
   "err":"execution reverted: IndexRegistry._operatorCountAtBlockNumber: quorum did not exist at given block number"
}

I spent a lot of time on this and was never able to determine how these two things were related. But as soon as I switched to using SendTransaction, the problem went away.

Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
@cody-littley cody-littley merged commit eee2309 into Layr-Labs:master Jan 10, 2025
8 checks passed
@cody-littley cody-littley deleted the disperser-auth-innabox branch January 10, 2025 17:10
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.

4 participants