SQL Nonce Manager for HA Payer #578
Conversation
WalkthroughThis pull request introduces a new nonce management system integrated within the blockchain package. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant BP as BlockchainPublisher
participant NM as NonceManager
Client->>BP: Call PublishGroupMessage()
BP->>NM: GetNonce(ctx)
NM-->>BP: Return NonceContext (nonce value)
BP->>BP: Create transaction using retrieved nonce
alt "Nonce too low" error occurs
BP->>NM: Consume nonce & re-request nonce
BP->>BP: Retry transaction creation
end
BP-->>Client: Return transaction receipt
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (14)
pkg/config/validation.go (1)
42-44: Inconsistent flag naming style in error message.The error message uses
--DB.WriterConnectionStringwhich is inconsistent with other flag error messages in this file. Most other flags follow a lowercase kebab-case format (e.g.,--contracts.rpc-url).Consider standardizing the error message format:
- missingSet["--DB.WriterConnectionString"] = struct{}{} + missingSet["--db.writer-connection-string"] = struct{}{}pkg/db/queries/models.go (1)
48-51: Consider adding documentation for the new NonceTable type.Since this is a newly added type that appears to be central to the SQL Nonce Manager feature, it would be beneficial to include a comment explaining its purpose and how it's used in the system.
Consider adding documentation like this:
+// NonceTable represents a record in the nonce management table, tracking +// transaction nonces and when they were created. type NonceTable struct { Nonce int64 CreatedAt sql.NullTime }pkg/authn/tokenFactory.go (1)
26-27: Inconsistency between factory return type and method receiver.The factory now returns a value type (
TokenFactory), but theCreateTokenmethod on line 34 still uses a pointer receiver (*TokenFactory). This inconsistency may lead to confusion when using the factory, as callers would need to take the address of the returned value to call methods.Either change the method receiver to match the factory's return type, or update the factory to return a pointer:
Option 1 - Change method receiver to value receiver:
-func (f *TokenFactory) CreateToken(forNodeID uint32) (*Token, error) { +func (f TokenFactory) CreateToken(forNodeID uint32) (*Token, error) {Option 2 - Return to returning a pointer from the factory:
-) TokenFactory { - return TokenFactory{ +) *TokenFactory { + return &TokenFactory{Also applies to: 34-34
pkg/db/queries.sql (1)
205-214: Efficient concurrent-safe nonce retrieval strategy.The
GetNextAvailableNoncequery makes excellent use of PostgreSQL'sFOR UPDATE SKIP LOCKEDfunctionality, which is perfect for high-concurrency nonce allocation. This ensures multiple clients can retrieve different nonces without blocking each other.Minor formatting suggestion: Consider consolidating the
ASC LIMIT 1clause onto a single line for better readability.- ORDER BY - nonce - ASC LIMIT 1 - FOR UPDATE SKIP LOCKED; + ORDER BY + nonce ASC + LIMIT 1 + FOR UPDATE SKIP LOCKED;pkg/db/sequences_test.go (1)
138-163: Test for idempotent nonce requests needs clarification.The test
TestRequestsUnusedappears to be checking that repeated calls toGetNextAvailableNoncereturn the same nonce if not consumed, but lacks documentation explaining this behavior.Add a comment explaining the expected behavior:
func TestRequestsUnused(t *testing.T) { + // This test verifies that repeated calls to GetNextAvailableNonce + // return the same nonce value when nonces aren't consumed + // (not deleted after retrieval) ctx := context.Background()pkg/db/queries/queries.sql.go (2)
28-46: Confirm concurrency performance.
UsingFOR UPDATE SKIP LOCKEDis effective for concurrency control, but ensure that there's an index onnonce_table.nonceto speed up row lookups and deletes when dealing with large data sets.
61-73: Consider making the batch size configurable.
TheFillNonceSequencemethod inserts 10,000 rows by default. Allowing a configurable batch size could make the system more flexible and prevent excessive inserts under varying workloads.pkg/blockchain/blockchainPublisher.go (2)
67-70: Ensure correct usage of FastForwardNonce.
If a chain reorganization happens, it could invalidate the fast-forwarded nonce. Consider verifying the nonce again if you detect a chain reorg.Do you want me to propose a plan for chain reorg detection and nonce re-verification?
252-255: Consider graceful shutdown flow.
Close()cancels the replenishment goroutine, but you may want to wait for it to exit fully to avoid unexpected race conditions if a nonce operation is mid-flight.pkg/blockchain/nonceManager.go (3)
11-20: Interface design is straightforward.
TheNonceContextstruct separates canceling and consuming responsibilities, which is clear and flexible. Just ensure documentation clarifies precisely when to callCancel()versusConsume().
22-32: Constructor logic is clear.
NewSQLBackedNonceManagersets up the manager with a named logger. Depending on your needs, you could optionally verify database connectivity at this stage.Do you want me to open a new issue to add an optional DB health check or readiness probe?
74-80: Monitor large insert actions.
The default batch size of 10,000 infillNoncescan cause performance overhead if triggered frequently. Making this limit environment-specific or load-based might help scalability.pkg/blockchain/nonceManager_test.go (2)
24-40: Simplify big.Int initialization in GetNonce method.The nonce creation with
new(big.Int).SetInt64(nonce)is unnecessarily verbose.- Nonce: *new(big.Int).SetInt64(nonce), + Nonce: *big.NewInt(nonce),
117-145: Enhance concurrent test with nonce uniqueness verification.The concurrent test demonstrates thread safety but doesn't actually verify that the nonces allocated are unique, which is a critical property for a nonce manager.
func TestGetNonce_ConsumeManyConcurrent(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() db, _, cleanup := testutils.NewDB(t, ctx) defer cleanup() logger, err := zap.NewDevelopment() require.NoError(t, err) nonceManager := blockchain.NewSQLBackedNonceManager(db, logger) err = nonceManager.Replenish(ctx, *big.NewInt(0)) require.NoError(t, err) var wg sync.WaitGroup numClients := 20 + var mu sync.Mutex + usedNonces := make(map[int64]bool) for i := 0; i < numClients; i++ { wg.Add(1) go func() { defer wg.Done() nonce, err := nonceManager.GetNonce(ctx) require.NoError(t, err) + nonceVal := nonce.Nonce.Int64() + mu.Lock() + _, exists := usedNonces[nonceVal] + require.False(t, exists, "Nonce %d was already used", nonceVal) + usedNonces[nonceVal] = true + mu.Unlock() err = nonce.Consume() require.NoError(t, err) }() } wg.Wait() + require.Equal(t, numClients, len(usedNonces), "Expected unique nonce for each client") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.mockery.yaml(1 hunks)cmd/replication/main.go(2 hunks)pkg/authn/tokenFactory.go(1 hunks)pkg/blockchain/blockchainPublisher.go(7 hunks)pkg/blockchain/blockchainPublisher_test.go(1 hunks)pkg/blockchain/nonceManager.go(1 hunks)pkg/blockchain/nonceManager_test.go(1 hunks)pkg/blockchain/rpcLogStreamer.go(10 hunks)pkg/blockchain/rpcLogStreamer_test.go(3 hunks)pkg/config/validation.go(1 hunks)pkg/db/gatewayEnvelope_test.go(5 hunks)pkg/db/pgx_test.go(7 hunks)pkg/db/queries.sql(1 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/queries.sql.go(3 hunks)pkg/db/queries_test.go(7 hunks)pkg/db/sequences_test.go(1 hunks)pkg/db/unsettledUsage_test.go(1 hunks)pkg/indexer/blockTracker_test.go(8 hunks)pkg/indexer/e2e_test.go(3 hunks)pkg/interceptors/client/auth.go(1 hunks)pkg/migrations/00008_payer-nonces.down.sql(1 hunks)pkg/migrations/00008_payer-nonces.up.sql(1 hunks)pkg/mocks/blockchain/mock_NonceManager.go(1 hunks)pkg/registrant/registrant.go(2 hunks)pkg/server/server.go(8 hunks)
✅ Files skipped from review due to trivial changes (2)
- pkg/migrations/00008_payer-nonces.down.sql
- pkg/db/unsettledUsage_test.go
🧰 Additional context used
🪛 GitHub Actions: Lint
pkg/blockchain/blockchainPublisher.go
[error] 1-1: Command failed with exit code 1.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
🔇 Additional comments (96)
pkg/db/pgx_test.go (4)
1-1: Appropriate package renaming for better test isolationChanging from
package dbtopackage db_testis a best practice that enhances test isolation. This approach ensures tests are written from the perspective of package consumers rather than allowing them to access unexported package members.
6-6: Clean dependency management with explicit importAdding an explicit import for the package being tested is the necessary companion change to the package renaming, ensuring proper qualification of all db package references.
23-30: Properly qualified DB constructor functionThe function call has been properly updated to use the qualified name
db.NewNamespacedDB. This change maintains consistency with the package separation approach.
51-58: Consistent function qualification across all test casesAll instances of
NewNamespacedDBhave been consistently updated to use the qualified name throughout the file. This thorough approach prevents any runtime errors that would occur from unqualified function names.Also applies to: 64-71, 80-87, 94-101, 165-172
pkg/db/gatewayEnvelope_test.go (4)
1-1: Appropriate package renaming for better test isolationChanging from
package dbtopackage db_testis a best practice that enhances test isolation. This approach ensures tests are written from the perspective of package consumers rather than allowing them to access unexported package members.
5-5: Clear package aliasing improves code readabilityUsing a descriptive alias
xmtpd_dbfor the imported package makes it clear when functions from the db package are being called in the test code.
26-26: Properly qualified null helper functionThe
NullInt32function is now properly qualified with the package alias, maintaining consistency with the package separation approach.
52-57: Consistent transaction function qualificationAll instances of
InsertGatewayEnvelopeAndIncrementUnsettledUsagehave been properly qualified with the package alias. This ensures clarity about which package's function is being called and prevents any name resolution issues.Also applies to: 80-85, 109-114
pkg/db/queries_test.go (4)
1-1: Appropriate package renaming for better test isolationChanging from
package dbtopackage db_testis a best practice that enhances test isolation. This approach ensures tests are written from the perspective of package consumers rather than allowing them to access unexported package members.
5-5: Clear package aliasing improves code readabilityUsing a descriptive alias
xmtpd_dbfor the imported package makes it clear when functions from the db package are being called in the test code.
46-47: Properly qualified null helper functionsAll instances of
NullInt64have been consistently qualified with the package aliasxmtpd_db. This maintains the separation between test code and implementation code.Also applies to: 61-62, 76-77, 102-103, 112-113, 128-129
1-161:Details
✅ Verification successful
Verify compatibility with the new NonceManager system
Since this PR introduces a new nonce management system as mentioned in the summary, it would be beneficial to ensure these test files properly interact with that system if relevant.
🏁 Script executed:
#!/bin/bash # Check for any NonceManager references in the test files and see how they're used echo "Looking for NonceManager references in test files..." rg -i "noncemanager" --type go --glob "*_test.go" echo "Looking for nonce-related SQL migrations..." fd -e sql | grep -i nonceLength of output: 2349
Compatibility with the new NonceManager system – No Action Required
After verifying the repository, it turns out that the tests in
pkg/db/queries_test.godo not reference or interact with the NonceManager system. Nonce management is exercised in separate tests (e.g., those inpkg/blockchain/blockchainPublisher_test.goandpkg/blockchain/nonceManager_test.go) and via specific SQL migrations inpkg/migrations/00008_payer-nonces.up.sql/.down.sql.
- The address log and payer creation tests here remain focused on database operations unrelated to nonce handling.
- NonceManager functionality is already covered in the blockchain-related modules.
Please continue to ensure that NonceManager behavior is adequately tested in the dedicated components. Overall, no changes are needed in this file.
pkg/config/validation.go (1)
42-44: Validate DB writer connection for Payer functionality.The addition of a check for the database writer connection string when the Payer functionality is enabled makes sense and follows the existing validation pattern used for other features in the system.
.mockery.yaml (1)
31-31: Adding NonceManager to mockery configuration is appropriate.Adding the
NonceManagerinterface to the mockery configuration enables proper mock generation for testing. This is a good practice for maintaining testability of the nonce management functionality.pkg/db/queries/models.go (1)
48-51: Good structure for nonce tracking with timestamps.The
NonceTablestructure withNonceandCreatedAtfields is well-designed for nonce management. Theint64type forNonceprovides sufficient range for blockchain nonces, and including a timestamp is useful for monitoring and troubleshooting.pkg/authn/tokenFactory.go (1)
26-27: Return value semantics for TokenFactory struct.Changing from pointer to value return type for
NewTokenFactorypromotes immutability and follows good Go practices for small, non-pointer-dependent structs.pkg/migrations/00008_payer-nonces.up.sql (2)
1-4: Good table structure for nonce management.The table design is appropriate with a
nonceprimary key and acreated_attimestamp for audit/cleanup purposes.
6-19: Well-designed function for efficiently filling nonce gaps.The
fill_nonce_gapfunction is well implemented with:
- Proper generation of sequential nonces using
generate_series- Skipping existing nonces with the
NOT EXISTSclause- Handling duplicates with
ON CONFLICT DO NOTHINGfor idempotencyThis design effectively supports concurrent operations in a high-availability environment.
Consider monitoring performance if this function is called with very large
num_elementsvalues, as it could generate many rows in a single transaction.cmd/replication/main.go (2)
75-76: Appropriate database initialization for the payer feature.Adding
options.Payer.Enableto the database initialization condition is a logical extension to support the new nonce management system for payers.
124-124: Improved logging consistency.Changing from
log.Fataltologger.Fatalenhances consistency with the structured logging system used throughout the application.pkg/db/queries.sql (3)
202-204: Well integrated function call for nonce gap filling.This query correctly calls the
fill_nonce_gapfunction defined in the migration script to populate the nonce sequence.
215-218: Simple and direct nonce deletion.This query provides a direct way to remove a specific nonce from the table, which is appropriate for when a nonce has been successfully used.
219-228: Efficient batch cleanup of obsolete nonces.The
DeleteObsoleteNoncesquery effectively:
- Uses a Common Table Expression (CTE) to identify rows to delete
- Employs
FOR UPDATE SKIP LOCKEDto avoid contention in concurrent scenarios- Removes all nonces below a certain threshold in a single operation
This approach is well-suited for maintenance operations while minimizing lock contention.
pkg/interceptors/client/auth.go (3)
17-17: Improved type semantics for TokenFactory.Changing from pointer to value semantics for
tokenFactoryimproves type safety and reduces the need for nil checks. This is a good design choice for types that should always have a valid value.
23-24: Consistent parameter type update.The function parameter type has been correctly updated to match the struct field type change.
26-26: Removed redundant nil check.The removal of what was likely a nil check at this line is appropriate since the value semantics change eliminates the need for such validation.
pkg/indexer/blockTracker_test.go (4)
1-1: Clean package renaming to follow Go convention.The package declaration has been changed from
indexertoindexer_test, which follows the Go convention for test packages. This is a good practice as it avoids circular imports and properly separates test code from the implementation.
5-5: Explicit import of the indexer package.With the package name change, the code correctly adds an explicit import for the
indexerpackage to access the functionality being tested.
24-25: Improved error handling after function call.The code now properly checks for errors immediately after the
NewBlockTrackercall withrequire.NoError(t, err). This ensures test failures occur at the point of failure rather than later in the test execution.
40-41: Consistent error handling pattern applied throughout the file.The change consistently applies the same pattern of immediately checking for errors after calling
NewBlockTrackeracross all test functions, which improves the robustness and readability of the tests.Also applies to: 68-69, 82-83, 117-118, 134-137, 158-160
pkg/registrant/registrant.go (1)
75-75: Updated method return type to match field type change.The
TokenFactory()method now returns the value type instead of a pointer, which is consistent with the field type change. This maintains API consistency with the internal implementation.The change from
*authn.TokenFactorytoauthn.TokenFactoryas a return type means that callers will receive a copy of the TokenFactory rather than a reference to the internal instance. This is safer as it prevents external code from modifying the internal state.pkg/server/server.go (5)
45-55: Added blockchainPublisher field to ReplicationServer struct.The addition of the
blockchainPublisherfield to theReplicationServerstruct enables the server to manage blockchain interactions through a dedicated component. This enhances the server's capabilities for blockchain operations.
171-171: Renamed log variable to logger for better consistency.The variable renaming from
logtologgerimproves naming consistency and clarity. This is a good practice that makes the code more readable and maintainable.
242-243: Introduced SQL-backed nonce management.The code initializes a new
SQLBackedNonceManagerwhich provides nonce management backed by a SQL database. This approach allows for persistence and coordination of nonces across server restarts or in distributed environments.The SQL-backed implementation is a robust choice for a high-availability system as it ensures nonces are properly tracked and not reused, even when multiple instances or processes are involved.
250-250: Updated BlockchainPublisher to use the NonceManager interface.The
BlockchainPublisherconstructor now takes anonceManagerparameter, which indicates a design shift to use a dedicated component for nonce management rather than handling it internally. This improves separation of concerns.
331-333: Added proper cleanup for blockchainPublisher in server shutdown.The
Shutdownmethod now ensures that theblockchainPublisheris properly closed, preventing resource leaks and ensuring clean shutdown. This is an important improvement for resource management.pkg/db/sequences_test.go (8)
1-14: Well-structured imports and package declaration.The file has appropriate imports and follows Go testing conventions with the
_testpackage suffix. The set of imports is comprehensive but not excessive, importing only what's needed for the tests.
16-28: Good basic test for filling nonce sequences.This test verifies the fundamental capability to fill the nonce sequence without errors, providing a baseline for more complex tests.
30-48: Comprehensive error handling test for empty sequences.The test correctly verifies that attempting to get a nonce from an empty sequence returns an error, and then confirms that after filling the sequence, nonces can be successfully retrieved.
50-83: Well-designed helper functions for nonce operations.The helper functions
getNextPayerSequenceandfailNextPayerSequenceencapsulate common test logic for acquiring and intentionally failing nonce operations, making the tests more readable and maintainable.
85-136: Thorough concurrent access test.This test effectively validates that nonce allocation is thread-safe by concurrently requesting nonces from multiple goroutines and ensuring each gets a unique value. This is critical for a high-availability system where concurrent operations will be common.
220-256: Good test for transaction isolation.
TestFillerCanProceedWithOpenTxneffectively tests that the nonce filler can continue to operate even when there are open transactions, which is important for ensuring system availability during ongoing operations.
341-384: Clear test for transaction rollback handling.This test verifies the "at least once" semantics of the nonce manager when transactions are rolled back, which is a critical aspect to test in systems dealing with transactional databases.
The test confirms that after a transaction rollback, the nonce manager behaves correctly by making nonces available again, consistent with the documented "at least once" semantics. This is important for error recovery scenarios.
278-299: Verifies obsolete nonce cleanup functionality.The test confirms that the system can properly delete obsolete nonces and continue operation with higher nonce values, which is essential for long-running systems to prevent database growth with unused entries.
pkg/mocks/blockchain/mock_NonceManager.go (4)
1-16: LGTM! Mockery-generated code following best practices.The mock structure includes all necessary imports and follows mockery's standard structure for interface mocks. The package declaration is appropriate for the mock's location.
26-42: LGTM! Well-structured mock method implementation.The
FastForwardNoncemock method correctly implements error handling, return type management, and panic prevention for unspecified returns.
73-101: LGTM! Thorough mock implementation for GetNonce.The
GetNoncemethod provides comprehensive handling of various mock return situations, properly managing both theNonceContextpointer and error return values.
131-143: LGTM! Factory function follows best practices.The
NewMockNonceManagerfunction properly registers the mock with the testing interface and sets up cleanup to verify expectations after tests are complete.pkg/indexer/e2e_test.go (4)
21-36: Improved database connection handling in test setup.The function now correctly returns the database connection to allow its use in downstream test functions. This change properly supports the new SQL-backed nonce management.
38-53: LGTM! Clean integration of SQL-backed nonce manager.The function has been updated to accept a database connection and properly initialize a SQL-backed nonce manager. This aligns with the PR objective of implementing an HA-ready nonce management solution.
54-61: LGTM! Updated publisher initialization with nonce manager dependency.The
NewBlockchainPublishercall correctly passes the newly created nonce manager as an argument, ensuring proper integration.
67-70: LGTM! Properly updated test flow.The test flow now correctly handles the database connection by capturing it from
startIndexingand passing it tomessagePublisher.pkg/blockchain/blockchainPublisher_test.go (7)
1-1: Good package naming for test separation.Changing from
package blockchaintopackage blockchain_testis a good practice that avoids circular imports while clearly indicating this is a test for the blockchain package.
5-5: LGTM! Explicit import supports package separation.With the package name change, the explicit import of the blockchain package is necessary and correctly implemented.
13-13: LGTM! Updated return type to match package changes.The function signature now correctly specifies the return type with the package prefix to match the new test package context.
20-23: LGTM! Updated function reference with package prefix.The
NewPrivateKeySignercall has been updated with the blockchain package prefix, aligning with the package restructuring.
26-27: LGTM! Consistent package prefix use.The
NewClientcall has been updated with the blockchain package prefix, maintaining consistency with other changes.
31-38: LGTM! Updated BlockchainPublisher initialization.The
NewBlockchainPublishercall now correctly includes the nonce manager parameter, ensuring that the publisher uses the test-specific implementation during tests.
29-30:Details
❓ Verification inconclusive
Verify implementation of TestNonceManager.
The code introduces
NewTestNonceManager, but its implementation isn't visible in this file. Ensure that the test nonce manager provides appropriate behavior for testing scenarios.
🏁 Script executed:
#!/bin/bash # Find the TestNonceManager implementation ast-grep --pattern 'func NewTestNonceManager($$$) $$$'Length of output: 517
TestNonceManager implementation confirmed.
I located theNewTestNonceManagerimplementation inpkg/blockchain/nonceManager_test.goaround lines 20–22. The current implementation simply initializes aTestNonceManagerwith the provided logger. Please verify that this minimal setup sufficiently simulates nonce behavior for your testing scenarios or consider enhancing it if additional functionality is required.pkg/blockchain/rpcLogStreamer.go (13)
33-33: LGTM! Updated type to exported type.Changed
contractConfigstype from[]contractConfigto[]ContractConfig, supporting the export of this type.
56-64: LGTM! Updated struct initialization.The initialization now uses the renamed
ContractConfigtype, maintaining consistency with the type rename.
73-80: Correct export of the ContractConfig struct.Renaming from
contractConfigtoContractConfigand exporting the fields make this structure accessible outside the package. This change is consistent with the nonce management refactoring, allowing the configuration to be shared across components.
91-91: LGTM! Updated type in RpcLogStreamer struct.The
watchersfield has been updated to use the exportedContractConfigtype, maintaining consistency.
102-102: LGTM! Updated parameter type in constructor.The
watchersparameter inNewRpcLogStreamerhas been updated to use the exportedContractConfigtype.
120-120: LGTM! Updated field access.Updated string formatting to use the exported
ContractAddressfield.
127-132: LGTM! Updated method signature and field access.The method signature now uses the exported
ContractConfigtype, and internal references have been updated to use the exported field names.
149-149: LGTM! Updated method call.Updated call from
getNextPagetoGetNextPageto match the exported method name.
180-180: LGTM! Updated field access.Updated to use the exported
EventChannelfield.
189-191: Method name exported for external use.Renamed
getNextPagetoGetNextPageto make it accessible outside the package. This is consistent with the overall approach of making components more modular and reusable for the new nonce management system.
193-193: LGTM! Updated field access.Updated to use the exported
ContractAddressfield.
239-239: LGTM! Updated parameter type.Updated parameter type in
buildFilterQueryto use the exportedContractConfigtype.
243-246: LGTM! Updated field access.Updated field access to use the exported field names
ContractAddressandTopics.pkg/db/queries/queries.sql.go (2)
15-26: Add unit tests for new DeleteAvailableNonce.
This operation removes entries for a specific nonce and returns the number of rows affected. It would be beneficial to write a test verifying that the correct entries are deleted and that the row count matches expectations.
270-286: Ensure concurrency testing.
TheFOR UPDATE SKIP LOCKEDquery is appropriate for concurrent nonce access. However, multi-threaded tests should confirm that multiple threads can safely retrieve available nonces and that no deadlocks occur.pkg/blockchain/blockchainPublisher.go (5)
29-30: NonceManager usage is well integrated.
Introducing thenonceManagerfield and storingreplenishCancelis a clean approach to nonce management and lifecycle handling.
39-40: Constructor parameter addition is consistent.
Ensure that any callers ofNewBlockchainPublisherare updated to provide the newnonceManagerparameter.
102-103: Reinforce constructor consistency.
Storing bothnonceManagerandreplenishCancelhere aligns with the updated initializer. No further concerns.
116-141: Ensure test coverage for withNonce usage.
Publishing a group message now depends on dynamic nonce retrieval. Adding tests for edge cases, such as invalid parameters or rapid consecutive calls, would help validate robust nonce handling.
153-178: Maintain parity with group message publishing.
The logic inPublishIdentityUpdateclosely mirrorsPublishGroupMessage; this consistency is good. Be sure to cover any corner cases specific to identity updates in testing.pkg/blockchain/nonceManager.go (3)
34-72: Transaction pattern is safe, yet concurrent use should be tested.
You correctly rollback on error and commit only upon successfulConsume(). Ensure thorough testing under load to confirm multiple callers can simultaneously retrieve nonces without conflicts.
82-86: Check for concurrency safety between fillNonces and abandonNonces.
abandonNoncesunconditionally deletes nonces below the given threshold. Ensure multiple threads calling this method cannot create partial gaps iffillNonceshasn't fully completed.
88-91: Replenish design is concise.
This function focuses on only one operation (fillNonces), which is straightforward.pkg/blockchain/nonceManager_test.go (6)
1-12: Implementation of package structure and imports looks solid.The package declaration as
blockchain_testis the correct practice for external tests, allowing access to both unexported package symbols and private test utilities.
14-22: Good implementation of the test manager.The
TestNonceManagerstruct with mutex protection for thread safety is well designed. The constructor correctly initializes the required fields.
42-52: Implement actual functionality in Replenish method for completeness.The
Replenishmethod is a no-op implementation returning nil, which might not provide adequate test coverage for components that rely on this method's behavior.Consider implementing actual functionality in the
Replenishmethod to match the behavior of the production implementation and ensure thorough testing.
54-72: Test setup and verification look correct.Good practice setting up the database context with proper cleanup and verifying the initial nonce value. The defer for nonce cancellation is appropriately placed.
74-93: Test for nonce reversion works as expected.The test correctly verifies that without consuming a nonce, repeated calls to GetNonce return the same value. This confirms the reservation behavior is working.
95-115: Consumption test correctly verifies sequential behavior.This test correctly verifies that consuming nonces leads to incremented values in subsequent calls, which is a key requirement for the nonce manager.
pkg/blockchain/rpcLogStreamer_test.go (8)
1-1: Package name change enhances separation of concerns.The package name change to
blockchain_testfollows Go's recommended practice for external tests, allowing them to be in a separate package while still maintaining access to test helpers.
5-7: Organized imports by improving dependency clarity.The imports now properly include the necessary packages with clear dependencies on Ethereum-related types and the business logic package being tested.
21-27: Function signature properly updated with qualified type names.The parameter and return types have been correctly updated with the
blockchainpackage qualifier, which is necessary given the package name change.
31-36: Struct field names updated to follow Go conventions.The field names are now using PascalCase, which is the correct convention for exported fields in Go. This change ensures better API consistency.
37-42: Function call properly uses qualified package names.The function call has been updated to use the qualified
blockchainpackage name, and the parameters are correctly organized and passed.
46-55: Client creation properly utilizes qualified package names.The client creation and builder instantiation now correctly use the
blockchainpackage qualifier, ensuring proper type references.
91-96: Configuration updated to use proper struct type.The
ContractConfigproperly uses the qualified type and PascalCase field names, maintaining consistency with the rest of the codebase.
98-98: Method call correctly updated to exported method name.The method call has been changed from
getNextPagetoGetNextPage, which indicates the method is now exported in the implementation. This change ensures proper encapsulation while allowing tests to access the functionality.
| @@ -0,0 +1,19 @@ | |||
| CREATE TABLE nonce_table ( | |||
| nonce BIGINT PRIMARY KEY, | |||
| created_at TIMESTAMP DEFAULT NOW() | |||
There was a problem hiding this comment.
technically all the table needs is the nonce column, but I added a timestamp too. Just for debugging purposes. It is never used in the code.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/blockchain/blockchainPublisher.go (2)
72-93:⚠️ Potential issueTimer will only fire once - use a time.Ticker instead.
The current implementation uses
time.NewTimerwhich will only fire once after 10 seconds. For periodic replenishment, atime.Tickershould be used instead.- timer := time.NewTimer(time.Second * 10) + ticker := time.NewTicker(time.Second * 10) ... - case <-timer.C: + case <-ticker.C:Also, remember to add
defer ticker.Stop()in the goroutine to clean up resources.
220-273: 🛠️ Refactor suggestionAvoid relying on string comparison for "nonce too low".
Different Ethereum clients or versions may return varying error messages. Consider using error wrapping or type assertions for more robust error detection.
- if err.Error() == "nonce too low" { + var nonceTooLowErr *NonceError // Define a custom error type or use a package-specific error + if errors.As(err, &nonceTooLowErr) || strings.Contains(err.Error(), "nonce too low") {Also, consider adding a mechanism to handle other common transaction errors like "underpriced" that were mentioned in the PR objectives.
🧹 Nitpick comments (1)
pkg/blockchain/blockchainPublisher.go (1)
230-254: Add backoff for nonce retries.The current implementation will immediately retry on "nonce too low" errors. Consider adding a short backoff to avoid tight loops and excessive resource usage.
+ var retries int for { + if retries > 0 { + backoffDuration := time.Duration(retries*100) * time.Millisecond + if backoffDuration > time.Second { + backoffDuration = time.Second + } + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(backoffDuration): + } + } nonceContext, err = nonceManager.GetNonce(ctx) if err != nil { return nil, err } nonce := nonceContext.Nonce tx, err = create(ctx, nonce) if err != nil { if err.Error() == "nonce too low" { logger.Debug( "nonce too low, consuming and moving on...", zap.Uint64("nonce", nonce.Uint64()), + zap.Int("retry", retries), ) err = nonceContext.Consume() if err != nil { nonceContext.Cancel() return nil, err } + retries++ continue } nonceContext.Cancel() return nil, err } break }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/blockchain/blockchainPublisher.go(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test (Node)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (7)
pkg/blockchain/blockchainPublisher.go (7)
29-30: Good addition of NonceManager interface.The structure changes look good. The replacement of mutex/atomic operations with a dedicated NonceManager interface aligns with the PR's goal of supporting high availability for payers using the same wallet.
39-40: Good dependency injection of NonceManager.Adding NonceManager as a parameter improves testability and flexibility, allowing different nonce management strategies to be used.
67-70: Properly initializes the nonce manager.Good job initializing the NonceManager with the current blockchain nonce using FastForwardNonce.
116-151: Good refactoring to use withNonce helper.The refactoring of PublishGroupMessage to use the withNonce helper function centralizes nonce management logic and reduces code duplication.
163-198: Good consistency in applying the nonce pattern.PublishIdentityUpdate follows the same pattern as PublishGroupMessage, maintaining consistency in how nonces are managed across different transaction types.
256-261: Good use of deferred cancellation.Properly defers cancellation of the nonce context in case of errors, ensuring resources are cleaned up even if an error occurs.
275-278: Good resource cleanup.The Close method properly cancels the replenishment goroutine, preventing resource leaks when the BlockchainPublisher is no longer needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/blockchain/blockchainPublisher.go (2)
73-94: Consider stopping the ticker to prevent resource leaks.You launch a goroutine that calls
Replenishevery 10 seconds, but the ticker isn't stopped when the context ends. Although the goroutine returns onreplenishCtx.Done(), consider callingticker.Stop()as well to clean up the underlying ticker channel.go func() { for { select { case <-replenishCtx.Done(): + ticker.Stop() return case <-ticker.C: ...
220-274: Robust nonce consumption witherrors.Is(err, core.ErrNonceTooLow).This logic properly handles the "nonce too low" error by consuming and retrying. Check if you might need to also handle other known error variants (e.g., "transaction underpriced").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(5 hunks)pkg/blockchain/blockchainPublisher.go(8 hunks)pkg/mocks/blockchain/mock_NonceManager.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
pkg/blockchain/blockchainPublisher.go (2)
Learnt from: mkysel
PR: xmtp/xmtpd#578
File: pkg/blockchain/blockchainPublisher.go:0-0
Timestamp: 2025-03-03T17:22:41.099Z
Learning: Go-ethereum defines a specific error constant `core.ErrNonceTooLow` for nonce too low errors. Instead of string comparison, use `errors.Is(err, core.ErrNonceTooLow)` for more robust error handling in blockchain transaction code.
Learnt from: mkysel
PR: xmtp/xmtpd#578
File: pkg/blockchain/blockchainPublisher.go:0-0
Timestamp: 2025-03-03T17:22:41.099Z
Learning: In go-ethereum, there's no specific error type or constant for "nonce too low" errors - they're returned as string-based errors. A more robust approach than direct string comparison is to use a helper function that checks for multiple error message patterns.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test (Node)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (9)
go.mod (1)
47-47:Details
❓ Verification inconclusive
Verify necessity and usage of newly introduced dependencies.
These new dependencies may be adding overhead if they are not utilized. Ensure they are indeed needed for the new nonce manager functionalities or other code changes. Otherwise, consider pruning them to maintain a lean codebase.
You can run the following script to search for references to these new packages throughout the codebase:
Also applies to: 71-71, 80-80, 89-89, 96-96, 106-106
🏁 Script executed:
#!/bin/bash rg --fixed-strings --glob '!*vendor*' 'github.com/VictoriaMetrics/fastcache' rg --fixed-strings --glob '!*vendor*' 'github.com/golang/snappy' rg --fixed-strings --glob '!*vendor*' 'github.com/holiman/bloomfilter/v2' rg --fixed-strings --glob '!*vendor*' 'github.com/mattn/go-runewidth' rg --fixed-strings --glob '!*vendor*' 'github.com/olekukonko/tablewriter' rg --fixed-strings --glob '!*vendor*' 'github.com/rivo/uniseg'Length of output: 4639
Action Required: Reassess the Necessity of Indirect Dependencies
After verifying the repository using the provided search script, the following observations were made:
- The dependency
github.com/VictoriaMetrics/fastcacheis present in both the primary (go.mod,go.sum) and auxiliary (tools/go.mod,tools/go.sum) module files, along with similar occurrences for the other packages.- None of these packages are referenced directly in the main source code outside of the module files. Their presence as “indirect” dependencies suggests they are brought in transitively.
Please verify whether these dependencies are required to support the new nonce manager functionalities (or any other recent changes) or if they are solely residual imports. If they are not explicitly needed, consider pruning them to keep the codebase lean.
pkg/blockchain/blockchainPublisher.go (7)
7-7: Confirm usage of the newly importedgithub.com/ethereum/go-ethereum/core.It's introduced to reference
core.ErrNonceTooLow, which is good for robust error handling. Confirm that there's no duplication of error checks or leftover string-based checks.
30-31: New fields and constructor parameter forNonceManager.Storing the
nonceManagerandreplenishCancelfunction in the struct is clear. This ensures we can gracefully handle the ticker lifecycle.Also applies to: 40-40
68-71: Fast-forwarding the nonce at initialization.Ensures the nonce in the manager is aligned with the current pending nonce. Verify that no transactions are missed if
FastForwardNonceis run after any in-flight transactions.
103-104: Clear association ofnonceManagerwith the publisher.Storing
nonceManagerandreplenishCancelensures consistent usage across multiple publish methods.
117-153: Streamlined nonce usage withwithNonce.This approach improves readability and centralizes nonce retrieval and consuming logic. Good job.
164-199: Consistent usage ofwithNoncein identity updates.Reusing the same pattern ensures consistent transaction creation and handling.
276-279: Graceful resource cleanup inClose().Invoking
replenishCancel()stops the background goroutine from replenishing. Combine with stopping the ticker if adopting the suggestion above.pkg/mocks/blockchain/mock_NonceManager.go (1)
1-192: Auto-generated mock file forNonceManager.All interface methods (
FastForwardNonce,GetNonce,Replenish) are included. Generally no changes are required, but ensure test coverage is sufficient for each method.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
pkg/blockchain/registryAdmin_test.go (1)
45-48: Good improvement to test reliability with retry mechanismThe implementation of
require.Eventuallyreplaces the direct call toAddNodewith a retry mechanism that attempts the operation for up to 1 second with 50ms intervals. This approach is more robust for blockchain operations where transient errors can occur.You could add a brief comment explaining why the retry is necessary here to help future developers understand the reason for using Eventually:
-require.Eventually(t, func() bool { +// Use Eventually to handle potential transient errors in blockchain operations +require.Eventually(t, func() bool {pkg/blockchain/migrator/migrator_test.go (1)
62-65: Good improvement to test reliability with retry mechanismThe implementation of
require.Eventuallyreplaces the direct call toAddNodewith a retry mechanism that attempts the operation for up to 1 second with 50ms intervals. This approach is more robust for blockchain operations where transient errors can occur.You could add a brief comment explaining why the retry is necessary here to help future developers understand the reason for using Eventually:
-require.Eventually(t, func() bool { +// Use Eventually to handle potential transient errors in blockchain operations +require.Eventually(t, func() bool {pkg/testutils/contracts.go (1)
6-6: Consider a more flexible retry interval.Thank you for adding the
timepackage to support retry intervals. You may consider making the interval configurable or adopting an exponential backoff to reduce potential flooding if the Ethereum client temporarily takes time to respond.pkg/db/queries/queries.sql.go (2)
28-46: Handles range-based deletion with row-level locking.
deleteObsoleteNoncesusesFOR UPDATE SKIP LOCKEDto avoid blocking on locked rows. This is aligned with concurrency best practices. Check concurrency usage to confirm no unexpected locked range expansions.
270-286: Sorted retrieval of the smallest nonce.
GetNextAvailableNonceusesORDER BY nonce ASC LIMIT 1 FOR UPDATE SKIP LOCKED. This ensures concurrency safety but be mindful of potential performance issues with large tables if lacking an appropriate index onnonce.pkg/blockchain/nonceManager_test.go (2)
50-52: Consider implementing actual functionality in Replenish method.The
Replenishmethod is a no-op implementation, which is fine for basic tests but might not adequately test interactions with the replenishment mechanism.Consider updating the implementation to better simulate the actual behavior:
func (tm *TestNonceManager) Replenish(ctx context.Context, nonce big.Int) error { + tm.mu.Lock() + defer tm.mu.Unlock() + if nonce.Int64() > tm.nonce { + tm.nonce = nonce.Int64() + tm.logger.Debug("Replenished nonce", zap.Int64("nonce", tm.nonce)) + } return nil }
117-145: Add verification of nonce sequence in concurrent test.The concurrent test creates multiple goroutines that get and consume nonces, but it doesn't verify that each goroutine received a unique nonce or that all nonces are used exactly once.
Consider enhancing the test to verify the uniqueness of nonces:
func TestGetNonce_ConsumeManyConcurrent(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() db, _, cleanup := testutils.NewDB(t, ctx) defer cleanup() logger, err := zap.NewDevelopment() require.NoError(t, err) nonceManager := blockchain.NewSQLBackedNonceManager(db, logger) err = nonceManager.Replenish(ctx, *big.NewInt(0)) require.NoError(t, err) var wg sync.WaitGroup numClients := 20 + var mu sync.Mutex + usedNonces := make(map[int64]bool) for i := 0; i < numClients; i++ { wg.Add(1) go func() { defer wg.Done() nonce, err := nonceManager.GetNonce(ctx) require.NoError(t, err) + nonceVal := nonce.Nonce.Int64() + mu.Lock() + _, exists := usedNonces[nonceVal] + require.False(t, exists, "Nonce %d was used more than once", nonceVal) + usedNonces[nonceVal] = true + mu.Unlock() err = nonce.Consume() require.NoError(t, err) }() } wg.Wait() + + // Verify that we used exactly numClients nonces + require.Equal(t, numClients, len(usedNonces), "Expected to use exactly %d unique nonces", numClients) + + // Verify that we used a continuous sequence starting from 0 + for i := 0; i < numClients; i++ { + _, exists := usedNonces[int64(i)] + require.True(t, exists, "Expected nonce %d to be used", i) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
.mockery.yaml(1 hunks)cmd/replication/main.go(1 hunks)go.mod(5 hunks)pkg/blockchain/blockchainPublisher.go(8 hunks)pkg/blockchain/blockchainPublisher_test.go(3 hunks)pkg/blockchain/migrator/migrator_test.go(2 hunks)pkg/blockchain/nonceManager.go(1 hunks)pkg/blockchain/nonceManager_test.go(1 hunks)pkg/blockchain/registryAdmin_test.go(2 hunks)pkg/blockchain/rpcLogStreamer.go(10 hunks)pkg/blockchain/rpcLogStreamer_test.go(3 hunks)pkg/config/validation.go(1 hunks)pkg/db/gatewayEnvelope_test.go(5 hunks)pkg/db/pgx_test.go(7 hunks)pkg/db/queries.sql(1 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/queries.sql.go(3 hunks)pkg/db/queries_test.go(7 hunks)pkg/db/sequences_test.go(1 hunks)pkg/db/unsettledUsage_test.go(1 hunks)pkg/indexer/blockTracker_test.go(8 hunks)pkg/indexer/e2e_test.go(3 hunks)pkg/migrations/00008_payer-nonces.down.sql(1 hunks)pkg/migrations/00008_payer-nonces.up.sql(1 hunks)pkg/mocks/blockchain/mock_NonceManager.go(1 hunks)pkg/server/server.go(3 hunks)pkg/testutils/contracts.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- pkg/migrations/00008_payer-nonces.down.sql
- pkg/config/validation.go
- .mockery.yaml
- pkg/db/queries/models.go
- pkg/migrations/00008_payer-nonces.up.sql
- pkg/server/server.go
- pkg/db/gatewayEnvelope_test.go
- pkg/db/pgx_test.go
- pkg/db/queries_test.go
- pkg/indexer/blockTracker_test.go
- pkg/db/unsettledUsage_test.go
- pkg/blockchain/rpcLogStreamer_test.go
- pkg/indexer/e2e_test.go
- pkg/blockchain/rpcLogStreamer.go
- cmd/replication/main.go
- pkg/mocks/blockchain/mock_NonceManager.go
- pkg/blockchain/nonceManager.go
- pkg/blockchain/blockchainPublisher_test.go
🧰 Additional context used
🧠 Learnings (1)
pkg/blockchain/blockchainPublisher.go (2)
Learnt from: mkysel
PR: xmtp/xmtpd#578
File: pkg/blockchain/blockchainPublisher.go:0-0
Timestamp: 2025-03-03T17:22:41.099Z
Learning: Go-ethereum defines a specific error constant `core.ErrNonceTooLow` for nonce too low errors. Instead of string comparison, use `errors.Is(err, core.ErrNonceTooLow)` for more robust error handling in blockchain transaction code.
Learnt from: mkysel
PR: xmtp/xmtpd#578
File: pkg/blockchain/blockchainPublisher.go:0-0
Timestamp: 2025-03-03T17:22:41.099Z
Learning: In go-ethereum, there's no specific error type or constant for "nonce too low" errors - they're returned as string-based errors. A more robust approach than direct string comparison is to use a helper function that checks for multiple error message patterns.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test (Node)
🔇 Additional comments (35)
pkg/blockchain/registryAdmin_test.go (1)
3-11: Import management looks goodThe addition of the time package is appropriate for supporting the new retry mechanism using require.Eventually.
pkg/blockchain/migrator/migrator_test.go (1)
3-14: Import management looks goodThe addition of the time package is appropriate for supporting the new retry mechanism.
go.mod (2)
71-71:Details
✅ Verification successful
Validate new indirect dependencies
These dependencies appear to have been added as indirect dependencies, likely as requirements of the blockchain libraries used by the nonce manager. Since they are indirect, they don't directly impact your code but should be reviewed for any security concerns.
Also applies to: 80-80, 89-89, 96-96, 106-106
🏁 Script executed:
#!/bin/bash # Check if these libraries are used directly in the codebase echo "Checking for direct imports:" rg "import.*snappy|import.*bloomfilter|import.*runewidth|import.*tablewriter|import.*uniseg" --type go # Check if these are recent versions echo "Checking versions of new dependencies:" go list -m github.com/golang/snappy github.com/holiman/bloomfilter/v2 github.com/mattn/go-runewidth github.com/olekukonko/tablewriter github.com/rivo/unisegLength of output: 668
Validate Indirect Dependencies – Security Check Passed
The new indirect dependencies (e.g.,
github.com/golang/snappy v0.0.5-0.20220116011046-fa5810519dcb, along with similar entries at lines 80, 89, 96, and 106) were verified as follows:
- Direct Imports: A search for direct usage of these libraries in our Go codebase returned no matches.
- Version Verification: The versions reported by
go list -mmatch the ones listed ingo.mod.Since these libraries are included indirectly (likely pulled in via the blockchain libraries used by the nonce manager), they don’t directly impact our code. However, please keep an eye on any security advisories related to transitive dependencies.
47-47:Details
❓ Verification inconclusive
Verify if this cache library is needed for the nonce manager
This new indirect dependency might be related to the nonce management implementation. Consider checking if it's actually needed or if it was added automatically.
🏁 Script executed:
#!/bin/bash # Check where fastcache is used in the codebase rg -A 2 -B 2 "fastcache" --type goLength of output: 34
Action: Verify Fastcache Dependency for Nonce Manager
After re-running the search for any direct import or usage of
github.com/VictoriaMetrics/fastcachein the Go codebase (using both context around "fastcache" and its full import path), no results were found. This indicates that the dependency appears only as an indirect reference, and there’s no immediate evidence that it’s directly used by the nonce manager.However, since the nonce manager might rely on this library indirectly or through transitive dependencies, please manually verify that none of its functionality depends on fastcache before deciding to remove it or conclude its necessity.
- Confirm that the nonce manager implementation does not implicitly depend on fastcache.
- Double-check transitive dependencies that might pull in fastcache.
pkg/db/queries.sql (4)
205-214: Excellent concurrent-safe nonce retrieval designThe use of
FOR UPDATE SKIP LOCKEDis perfect for a high-availability environment where multiple threads need to consume nonces. This allows concurrent access without blocking, which is exactly what's needed for the HA payer implementation.
- Ordering by nonce ASC ensures we use the lowest available nonce
- SKIP LOCKED prevents threads from blocking each other
- FOR UPDATE prevents the same nonce from being used by multiple threads
215-218: Clean nonce deletion after consumptionThis query provides a straightforward way to delete a nonce after it has been used in a transaction, keeping the nonce table clean and well-maintained.
219-228: Well-designed obsolete nonce cleanup with concurrency handlingThis implementation is excellent for cleaning up obsolete nonces:
- The CTE (deletable) identifies nonces that are less than the given nonce
- The FOR UPDATE SKIP LOCKED ensures that this cleanup doesn't interfere with other operations
- The JOIN-based deletion is efficient for bulk removal
This approach supports the requirement mentioned in the PR to handle nonce management properly, even in failure scenarios.
202-204:Details
✅ Verification successful
Efficient batch nonce generation with fill_nonce_gap function
This implementation aligns with the PR objective of generating nonces in batches (10,000 as mentioned) and ensures gaps in the nonce sequence are filled appropriately.
🏁 Script executed:
#!/bin/bash # Check the implementation of the fill_nonce_gap function fd -t f "migrations" -x grep -l "fill_nonce_gap" {} \; # Show the function definition find . -type f -name "*.sql" -exec grep -l "CREATE.*FUNCTION.*fill_nonce_gap" {} \; -exec cat {} \;Length of output: 831
Review Approved: Efficient Batch Nonce Generation
The query inpkg/db/queries.sql(lines 202-204) now correctly invokes thefill_nonce_gapfunction. Verification confirms that the implementation inpkg/migrations/00008_payer-nonces.up.sqluses a robust approach:
- It generates a series of nonces from
pending_noncetopending_nonce + num_elements - 1.- It conditionally inserts nonces into
nonce_table, skipping any that already exist.- The use of
ON CONFLICT DO NOTHINGensures duplicate entries are avoided.This aligns well with the PR objective of generating nonces in large batches (10,000) while appropriately filling any gaps in the sequence. Great work!
pkg/testutils/contracts.go (1)
108-164: Validate loop exit condition and error handling.The retry logic correctly increments on each error and ultimately fails if attempts exceed
retryMax. The use ofrequire.NoError(t, err)after the loop will catch any unresolved error. This approach looks good overall; just ensure the client is properly closed or reused if needed and keep an eye on potential transient failures.pkg/db/sequences_test.go (13)
16-28: Straightforward test for filling nonces.The
TestFillRowstest is simple and ensures thatFillNonceSequenceruns without errors. No concerns here.
30-48: Good verification of empty and refilled states.
TestEmptyRowsconfirms the error state when no nonces exist, then verifies successful filling and retrieval of the first nonce. This helps validate initialization behaviors.
50-68: Potential concurrency caution in transaction function.
getNextPayerSequencesleeps for 10ms after acquiring a nonce, which is fine for tests. In high-load scenarios, consider verifying that short sleeps don’t cause race conditions or flakiness. Otherwise, the approach is valid.
70-83: Intentional failure path well-structured.
failNextPayerSequencetests error-handling in a transaction, which helps ensure correctness under partial failures. The structure is concise and effective.
85-136: Robust concurrency test for nonce uniqueness.
TestConcurrentReadssets up multiple goroutines, each acquiring a distinct nonce. This thoroughly checks for duplicates or missed IDs. The short sleep and logging is fine for demonstration; for large-scale concurrency, you might consider more robust synchronization or a wait-based approach.
138-163: Testing at-least-once semantics with repeated calls.
TestRequestsUnusedconfirms the same nonce is acquired multiple times when not deleted. This is consistent with the at-least-once approach.
164-189: Proper coverage for used nonce increments.
TestRequestsUsedensures once the nonce is “used” (deleted), subsequent calls retrieve the next value. This aligns well with the gapless requirement.
191-218: Failure scenario coverage.
TestRequestsFailedchecks that a failed transaction reverts the nonce usage. The final retrieval ensures the next new nonce is allocated. This is a significant test of resilience.
220-256: Concurrent fill with an open transaction.
TestFillerCanProceedWithOpenTxnshows that filling can succeed even when another transaction holds locks. This is important for HA usage.
258-276: Re-running the fill logic.
TestFillerRerunensures multiple calls toFillNonceSequenceappend or fill additional nonces without error. The coverage looks good.
278-299: Abandoning lower nonces.
TestAbandonNonceschecks partial deletion of lower-range nonces. The retrieval confirms that only newer nonces remain available. Straightforward and effective.
301-339: Deleting nonces during an open transaction.
TestAbandonCanProceedWithOpenTxnconfirms we can abandon (delete) older nonces while another transaction still references them. The coverage is excellent for concurrency correctness.
341-384: Validation of at-least-once semantics post-abandon.
TestAbandonSkipsOpenTxnconfirms a rolled-back transaction might still yield a “too low” nonce in subsequent calls, consistent with at-least-once. Great boundary testing.pkg/db/queries/queries.sql.go (2)
15-26: Efficient targeted deletion of a single nonce.
deleteAvailableNonceis appropriately straightforward. Ensure there's an index onnonce_table.noncefor optimal performance.
61-73: Gap-filling approach for nonce sequence.
fill_nonce_gapfosters a gapless sequence. Make sure the underlying function is well-tested for edge cases (e.g., negative or extremely large nonce values).pkg/blockchain/blockchainPublisher.go (8)
7-7: Good addition for proper error handling.Adding the import for
github.com/ethereum/go-ethereum/coreallows for proper error handling using the predefined error constants likecore.ErrNonceTooLowrather than relying solely on string comparison.
31-32: Good structural change for nonce management.The introduction of the
nonceManagerfield andreplenishCancelfunction properly separates concerns, allowing for more maintainable code and easier testing. ThereplenishCancelfunction is essential for proper resource cleanup.
41-41: Appropriate function signature update.Adding the
nonceManagerparameter to theNewBlockchainPublisherfunction enables dependency injection, making the code more testable and flexible.
69-95: Correctly implemented periodic nonce replenishment.The implementation now uses a ticker for periodic replenishment as previously recommended, which is the correct approach for recurring tasks. The goroutine properly handles context cancellation and error logging.
I notice that on error, the replenishment continues to retry on the next tick, which is appropriate for this use case.
104-105: Properly initialized the new fields.The newly added fields are correctly initialized in the returned struct.
240-241: Robust implementation for handling nonce errors.Using both
errors.Is(err, core.ErrNonceTooLow)and string comparison provides a belt-and-suspenders approach to error handling, which is more robust than either method alone.This implementation addresses the previous discussion about error handling approaches for "nonce too low" errors, incorporating the feedback from previous reviews.
222-277: Well-designed helper function for nonce management.The
withNoncefunction properly encapsulates the nonce retrieval, transaction creation, and error handling logic. The implementation:
- Uses proper error handling and retries for nonce issues
- Ensures resource cleanup with deferred calls
- Provides a clean interface for the caller
This is a significant improvement over the previous implementation.
279-282: Essential cleanup method added.The
Closemethod is necessary to properly cancel the goroutine that replenishes nonces, preventing resource leaks. This is an important addition to the lifecycle management of theBlockchainPublisher.pkg/blockchain/nonceManager_test.go (3)
14-22: Simple and effective test implementation.The
TestNonceManagerstruct provides a basic implementation for testing purposes. It includes proper mutex locking for thread safety, which is essential for concurrent tests.
24-40: Thread-safe nonce generation with appropriate logging.The
GetNoncemethod correctly implements mutex locking to ensure thread safety. The returnedNonceContextuses no-op functions which is appropriate for the test implementation.
42-48: Correctly implements FastForwardNonce with proper locking.The method properly uses mutex locking to ensure thread safety when updating the nonce value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/blockchain/blockchainPublisher.go (2)
77-95: Well-structured goroutine for nonce replenishment.The implementation properly cancels the goroutine when the context is done, and includes appropriate error handling. Consider adding backoff or throttling if consecutive errors occur.
go func() { + consecutiveErrors := 0 + maxConsecutiveErrors := 5 + backoffDuration := 10 * time.Second for { select { case <-replenishCtx.Done(): return case <-ticker.C: nonce, err := client.PendingNonceAt(replenishCtx, signer.FromAddress()) if err != nil { logger.Error("error getting pending nonce", zap.Error(err)) + consecutiveErrors++ + if consecutiveErrors > maxConsecutiveErrors { + logger.Warn("too many consecutive errors, backing off", + zap.Duration("backoff", backoffDuration), + zap.Int("consecutiveErrors", consecutiveErrors)) + time.Sleep(backoffDuration) + } continue } err = nonceManager.Replenish(replenishCtx, *new(big.Int).SetUint64(nonce)) if err != nil { logger.Error("error replenishing nonce", zap.Error(err)) + consecutiveErrors++ + if consecutiveErrors > maxConsecutiveErrors { + logger.Warn("too many consecutive errors, backing off", + zap.Duration("backoff", backoffDuration), + zap.Int("consecutiveErrors", consecutiveErrors)) + time.Sleep(backoffDuration) + } + } else { + consecutiveErrors = 0 } } } }()
222-282: Well-structured withNonce helper function.The withNonce function effectively encapsulates the common pattern of nonce management, making the code more maintainable. The loop and error handling logic is robust, ensuring proper retry behaviors for nonce-related errors.
However, consider moving the error message string checks to a separate helper function to improve readability and maintainability:
+func isNonceTooLowError(err error) bool { + if errors.Is(err, core.ErrNonceTooLow) { + return true + } + errMsg := err.Error() + return strings.Contains(errMsg, "nonce too low") || + strings.Contains(errMsg, "replacement transaction underpriced") +} func withNonce[T any](ctx context.Context, logger *zap.Logger, nonceManager NonceManager, create func(context.Context, big.Int) (*types.Transaction, error), wait func(context.Context, *types.Transaction) (*T, error), ) (*T, error) { // ... if err != nil { - if errors.Is(err, core.ErrNonceTooLow) || - strings.Contains( - err.Error(), - "nonce too low", - ) || - strings.Contains(err.Error(), "replacement transaction underpriced") { + if isNonceTooLowError(err) { // ... } } // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/blockchain/blockchainPublisher.go(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
pkg/blockchain/blockchainPublisher.go (2)
Learnt from: mkysel
PR: xmtp/xmtpd#578
File: pkg/blockchain/blockchainPublisher.go:0-0
Timestamp: 2025-03-03T17:22:41.099Z
Learning: Go-ethereum defines a specific error constant `core.ErrNonceTooLow` for nonce too low errors. Instead of string comparison, use `errors.Is(err, core.ErrNonceTooLow)` for more robust error handling in blockchain transaction code.
Learnt from: mkysel
PR: xmtp/xmtpd#578
File: pkg/blockchain/blockchainPublisher.go:0-0
Timestamp: 2025-03-03T17:22:41.099Z
Learning: In go-ethereum, there's no specific error type or constant for "nonce too low" errors - they're returned as string-based errors. A more robust approach than direct string comparison is to use a helper function that checks for multiple error message patterns.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
🔇 Additional comments (6)
pkg/blockchain/blockchainPublisher.go (6)
7-7: Good addition of go-ethereum/core import.The addition of the core package import supports the direct comparison with
core.ErrNonceTooLow, which is a more robust error checking approach than string comparisons alone.
31-32: Good restructuring of the BlockchainPublisher struct.The changes remove the atomic nonce handling in favor of the NonceManager interface, which better encapsulates the nonce management logic and improves maintainability. The addition of replenishCancel is a good practice for managing the lifecycle of the background goroutine.
74-75: Properly implemented ticker for periodic nonce replenishment.The change from a one-shot timer to a ticker ensures continuous replenishment of nonces at regular intervals, which is essential for the proper functioning of the high-availability payer system.
240-250: Robust error handling for nonce-related errors.The code correctly checks for
core.ErrNonceTooLowusingerrors.Is()while also maintaining string comparisons as fallbacks. This dual approach ensures compatibility with different error representations that might be returned by various Ethereum clients.
265-269: Well-implemented cleanup with defer.The defer block ensures that the nonce context is properly canceled if any error occurs during the transaction wait process, preventing resource leaks.
284-287:Details
✅ Verification successful
Good addition of Close method for proper cleanup.
The Close method properly terminates the background goroutine, which is essential for clean shutdown. Consider adding more cleanup logic if the NonceManager interface supports a Close method.
🏁 Script executed:
#!/bin/bash # Check if NonceManager interface has a Close method that should be called # Look for the NonceManager interface definition echo "Searching for NonceManager interface definition:" rg -A 10 "type NonceManager interface" --type go # Look for any Close method on concrete implementations echo "Searching for Close methods on NonceManager implementations:" rg "func \(\w+ \*\w+.*\) Close\(\)" --type go | grep -v BlockchainPublisherLength of output: 6175
Verified Cleanup in Close Method – No Additional Action Needed
The implemented Close method correctly logs its shutdown process and terminates the background goroutine via
m.replenishCancel(). Our verification confirms that theNonceManagerinterface (defined inpkg/blockchain/nonceManager.go) does not expose aClosemethod. Therefore, there’s no need to add further cleanup logic at this time. If in the future theNonceManagerinterface is extended with aClosemethod, you may consider updating theClosemethod inBlockchainPublisheraccordingly.
| contractAddress common.Address | ||
| topics []common.Hash | ||
| eventChannel chan<- types.Log | ||
| type ContractConfig struct { |
There was a problem hiding this comment.
nit: I'm failing to see why we need to export ContractConfig, can you explain to me what I'm missing?
There was a problem hiding this comment.
the tests access it and they are located in their separate _test package
Create a SQL backed nonce manager so we can have HA payer using the same wallet.
This uses SQL. We know we can't trust PostgreSQL sequences to generate gapless sequences, which is a requirement for nonces. So, this generates a table of nonces that the various threads can consume.
SQL guarantees that if the payer fails, or Cancel() gets called, the nonce is released back into the pool. A future transaction can grab that nonce again.
There is a lot of SQL magic here using
FOR UPDATE SKIP LOCKED.The nonce release/consume is kinda convoluted so its extracted into its own
withNoncefunction.This manager has a at-least-once semantic. It will never skip a nonce and ideal cases will generate each nonce exactly once. But during failure, it might return a nonce more than once. This might lead to "underpriced" and "nonce too low" errors. We can internally handle those without exposing them to the client. If "nonce too low" is detected, we just attempt to use the next one until we get something usable.
The payer now needs a DB. I am just using the same one... In our environments, we can point the payer at the same DB instance and migration will take care of things. In other environments, a separate DB can be provided. I think that it is a lot of work to introduce a completely separate DB with separate migrations, so I opted for this quick and dirty solution.
There are a lot of unit tests, but I have to deliver this in stages.
I have raised the bar on test flakiness. I am trying to get the tests to pass for 10+ min of running continuously. We are not there yet, but at least the blockchain related tests don't fail. There are lots of changes to tests in this PR that came from that effort. Mostly retries.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Chores
Tests