-
Notifications
You must be signed in to change notification settings - Fork 2
Complete pdp implementation #2
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
base: master
Are you sure you want to change the base?
Complete pdp implementation #2
Conversation
- Extract PDPVerifier ABI from synapse-sdk reference - Generate Go bindings using abigen for PDPVerifier contract - Add contract address constants for Mainnet and Calibration networks - Contract addresses: - Mainnet: 0xBADd0B92C1c71d02E7d520f64c0876538fa2557F - Calibration: 0x85e366Cf9DD2c0aE37E963d9556F5f4718d6417C This provides the foundation for implementing direct on-chain PDP operations including ProofSetManager.
- Add pkg/txutil package with transaction management utilities: - WaitForConfirmation: wait for transaction confirmations - EstimateGasWithBuffer: gas estimation with safety buffer - SendTransactionWithRetry: retry logic with exponential backoff - NonceManager: thread-safe nonce management - Retryable error detection and gas/nonce error handling - Implement ProofSetManager in pdp/manager.go: - CreateProofSet: create new proof sets on-chain - GetProofSet: retrieve proof set details - AddRoots: add piece CIDs to proof sets - GetRoots: retrieve pieces with pagination - DeleteProofSet: remove proof sets - GetNextChallengeEpoch: query challenge schedule - DataSetLive: check proof set status - Event parsing for DataSetCreated and PiecesAdded This completes the core PDP functionality for on-chain proof set management and integrates with PDPVerifier contract.
- Add comprehensive tests for retry logic functions - Test retryable error detection (nonce, gas, network errors) - Test backoff calculation with exponential growth - Test retry configuration defaults - Test error wrapping utilities - Test gas estimation validation - Fix format string in manager.go for network type All tests pass successfully.
- Add main README.md with: - Installation instructions - Quick start guide - API overview for all components - Configuration details - Contract addresses for Mainnet and Calibration - Development and testing instructions - Add create-proof-set example: - Complete working example of creating proof sets - Environment variable configuration - Optional piece addition - Example output and usage instructions - README with detailed setup steps The documentation provides clear guidance for users to get started with the go-synapse SDK for PDP operations.
- Add integration test suite with build tag - Test proof set lifecycle (create, query, check status) - Test contract connection and verification - Configure via environment variables: - CALIBRATION_RPC: RPC endpoint - TEST_PRIVATE_KEY: test wallet - TEST_LISTENER_ADDRESS: record keeper address Run with: go test -tags=integration -v ./... These tests verify end-to-end functionality against Calibration testnet including contract interactions and transaction confirmations.
Fixes staticcheck SA1012 linting error by using proper context instead of passing nil to EstimateGasWithBuffer in tests.
lanzafame
left a comment
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.
Really good start!
|
@lanzafame take a look again please? |
|
@anjor some claude generated pr comments, but I genuinely think those are issues that need addressing. |
|
Thanks @lanzafame! Pushed up fixes. |
parkan
left a comment
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.
overall looking pretty good, need to be more careful with error handling/looping
I'm not sure gas price discovery is ideal but we'll take it for now
pkg/txutil/nonce.go
Outdated
| delete(nm.pendingTxs, nonce) | ||
|
|
||
| // If this was the most recently allocated nonce, roll back | ||
| if nm.nonce != nil && nonce == *nm.nonce-1 { |
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.
does fevm allow gaps in nonce sequences? you can still race even w/mutex and send failed subtractions back in time
| return nil, ctx.Err() | ||
| case <-ticker.C: | ||
| receipt, err := client.TransactionReceipt(ctx, txHash) | ||
| if err != 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.
this should check error type for recoverable (i.e. not included yet) vs not
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.
(and should emit/log non-recoverable errors and bail out)
| } | ||
|
|
||
| currentBlock, err := client.BlockNumber(ctx) | ||
| if err != 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.
ibid
| select { | ||
| case <-ctx.Done(): | ||
| return nil, fmt.Errorf("timeout waiting for transaction receipt: %w", ctx.Err()) | ||
| case <-ticker.C: |
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.
this will also loop until timeout and report an ambiguous error (failed to confirm tx or RPC timeout report the same message)
pkg/txutil/retry.go
Outdated
| // Apply decorrelated jitter to prevent synchronized retry storms | ||
| // Returns backoff/2 + random(0, backoff/2) | ||
| halfBackoff := backoff / 2 | ||
| jitter := time.Duration(rand.Int63n(int64(halfBackoff) + 1)) |
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.
not a huge deal here but good habit to use crypto/rand over math/rand just so you don't make an oopsie when it's important, it's cheap enough and don't need to worry about seeds
pdp/manager.go
Outdated
|
|
||
| // NewManager creates a new ProofSetManager with default configuration. | ||
| // | ||
| // Deprecated: Use NewManagerWithContext for proper context support or |
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.
why are we committing deprecated code on first merge? prefer removing and having clean slate since no one depends on this
|
presumably this is still a draft PR since we're missing major functionality, or is below out of scope? in storage: don't exist (totally makes sense to separate those form this layer but responding to the "complete" framing in title) |
README.md
Outdated
| - `UploadFile()` - Upload a file to storage provider | ||
| - `UploadData()` - Upload raw data | ||
| - `FindPiece()` - Check if a piece exists | ||
| - `DownloadPiece()` - Retrieve piece data |
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.
this is Download on storage manager currently, and DownloadPiece on pdp server
pkg/txutil/confirmation.go
Outdated
|
|
||
| // WaitForReceipt waits for a transaction receipt without confirmation requirements | ||
| func WaitForReceipt(ctx context.Context, client *ethclient.Client, txHash common.Hash, timeout time.Duration) (*types.Receipt, error) { | ||
| ctx, cancel := context.WithTimeout(ctx, timeout) |
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.
hmmm does this do what you want it to do? our context/timeout behavior is inconsistent across APIs (cf WaitForConfirmation) and this signature can be misleading (can only tighten not increase timeout)
90s is also kind of a short default given 30s blocks
| @@ -0,0 +1,1266 @@ | |||
| [ | |||
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.
should decide whether the json or the golang should be checked in; I would personally keep this for readability/compatibility with tooling and .gitignore the generated pdp verifier and go generate during build
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.
or, conversely delete this and keep the generated go; either way, two 1k+loc that could conceivably go out of sync if generate doesn't run at the right time seems less than ideal -- I still lean towards keeping JSON so diffs are legible but keeping the go might make sense so go get just works with no depends (however we can build artifacts in CI...)
|
|
||
| // NewManagerWithConfig creates a new ProofSetManager with custom configuration. | ||
| // If config is nil, default configuration will be used. | ||
| func NewManagerWithConfig(ctx context.Context, client *ethclient.Client, privateKey *ecdsa.PrivateKey, network constants.Network, config *ManagerConfig) (*Manager, error) { |
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.
hmmm ok so what I don't like here is that the client network is inferred from RPC, network is an explicit choice, so it's entirely possible to specify mismatched RPC + network and then blow up with unhelpful error instead of config validation
we may also want flexibility to interop with any contract hierarchy that complies with the interfaces, per user wishes, as well as to override RPC endpoint, while keeping everything sane
prefer something like:
func NewManagerWithConfig(ctx context.Context, client *ethclient.Client, privateKey *ecdsa.PrivateKey, network constants.Network, config *ManagerConfig) (*Manager, error) {
chainID, err := client.ChainID(ctx)
if err != nil {
return nil, fmt.Errorf("failed to get chain ID: %w", err)
}
expectedChainID := constants.ChainID(network) // needs to be added
if expectedChainID != chainID.Int64() {
return nil, fmt.Errorf("network %v expects chain ID %d, but RPC returned %d",
network, expectedChainID, chainID)
}
// Contract address: explicit override or derive from network
contractAddr := config.ContractAddress
if contractAddr == (common.Address{}) {
contractAddr = constants.GetPDPVerifierAddress(network)
}
// ...
}
|
|
||
| return &Manager{ | ||
| client: client, | ||
| privateKey: privateKey, |
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.
we need to figure out how client is coupled to manager; the way I inject privateKey in client is... sloppy and should really be a signer interface, then spawning manager we need to retain the key then inject it again in the constructor
I am refactoring singularity to have a separate keystore/signer and ideally that would just get passed into the main synapse entrypoint so we don't needlessly have private keys in random memory locations, we should discuss this
Completes the go-synapse library with full PDP (Proof of Data Possession) functionality, enabling integration with Singularity for f41 PDP deals.