rollmint feat: implement multi-account support with round-robin address selection

Overview

Closes #2627

Oct 27 '25 19:10 randygrok

The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
`✅ passed`	`⏩ skipped`	`✅ passed`	`✅ passed`	Nov 10, 2025, 10:41 AM

Oct 27 '25 19:10 github-actions[bot]

Claude finished @randygrok's task —— View job

Code Review Complete ✅

I've completed a comprehensive code review of the multi-account DA submission feature. Overall, this is a well-designed and well-implemented feature that addresses a real pain point with sequence mismatches in high-throughput DA submissions. The code quality is high, with excellent test coverage and clear documentation.

Summary

This PR introduces multi-account support with round-robin address selection for DA submissions to prevent sequence/nonce conflicts when submitting multiple transactions concurrently to Cosmos SDK-based DA layers like Celestia. The implementation is solid with good separation of concerns, thorough testing, and proper thread-safety.

Strengths:

Clean abstraction with AddressSelector interface
Thread-safe round-robin implementation using atomic.Uint64
Comprehensive test coverage including concurrency tests
Well-documented configuration options
Defensive panic checks in constructor and Next() method

Areas for improvement: See detailed feedback below.

Detailed Findings

1. Code Quality & Architecture ⭐ Excellent

Strengths:

Clean interface design: The AddressSelector interface (pkg/da/selector.go:8-12) provides excellent abstraction, allowing easy extension with new selection strategies
Proper separation of concerns: Address selection logic is isolated from submission logic
Idiomatic Go: Code follows Go conventions well (naming, package structure, error handling)
Good use of atomic operations: atomic.Uint64 is correctly used for thread-safe counter (pkg/da/selector.go:19,46)

Observations:

The NoOpSelector pattern is elegant for the zero-addresses case
Configuration integration is clean with backward compatibility maintained

Minor suggestions:

Consider adding a comment explaining why Add(1) - 1 is used instead of Load() + Add(1) in RoundRobinSelector.Next() (line 46) - it's for atomicity, but worth documenting
The NewRoundRobinSelector constructor panics on empty slice (line 26), which is documented but could also return an error for more graceful handling in some contexts

2. Error Handling & Resilience ✅ Good

Strengths:

Proper error wrapping in mergeSubmitOptions (da_submitter.go:390)
Graceful handling of invalid JSON in mergeSubmitOptions (lines 372-375)
Good defensive checks: nil map handling after JSON unmarshal (lines 379-381)

Critical Issue Found (Already Addressed): ✅ FIXED: The Gemini Code Assist review identified a potential panic with JSON null literal, and I can see this has been addressed with the nil check at lines 379-381. A test was also added (da_submitter_options_test.go:126-132).

Suggestions:

Context cancellation: Consider checking ctx.Done() before selecting an address in the retry loop to avoid selecting addresses unnecessarily when the context is already cancelled
Logging improvement: When mergeSubmitOptions fails, the error is logged and returned (da_submitter.go:456), but consider adding the original base options (truncated) to the log for debugging

3. Security 🔒 Good

Strengths:

No hardcoded credentials or secrets
Addresses are treated as configuration, not sensitive data
Proper input validation in mergeSubmitOptions

Observations:

Signer address exposure: Signing addresses are logged at DEBUG level (da_submitter.go:461). This is appropriate since addresses are public information
JSON injection: The mergeSubmitOptions function properly validates JSON and doesn't execute arbitrary code
Configuration: Addresses come from trusted sources (config file/flags), not user input

Recommendations:

Consider adding validation for address format in config validation (config.go:236-266) to catch configuration errors early
Document that signing_addresses should contain valid Celestia addresses in the config comments

4. Performance & Resource Efficiency ⚡ Excellent

Strengths:

Lock-free concurrency: Uses atomic.Uint64 instead of mutex for counter
Zero allocations on hot path: RoundRobinSelector.Next() has no allocations in steady state
Efficient modulo operation: Uses modulo on atomic counter for index calculation
Minimal overhead: NoOpSelector returns empty string with no computation

Concurrency test validates scalability:

Tests with 100 goroutines × 100 calls (10,000 total) show proper distribution (selector_test.go:57-105)
Fair distribution verified within 10% tolerance

Minor optimization opportunity:

In mergeSubmitOptions, the JSON unmarshal/marshal cycle happens on every submission. Since base options are typically static, consider caching the parsed base options map if profiling shows this is a hot path

5. Testing & Reliability ✅ Excellent

Strengths:

Comprehensive unit tests: 11 test cases for selectors, 8 for merge options
Concurrency testing: Dedicated tests for race conditions and fairness
Edge case coverage: Empty lists, single address, nil inputs, invalid JSON, null JSON
Mock integration: DA submitter tests properly integrate with existing test infrastructure

Test quality highlights:

TestRoundRobinSelector_Concurrent (selector_test.go:57-105) - validates thread safety AND fairness
TestMergeSubmitOptions_NullJSON (da_submitter_options_test.go:126-132) - catches the panic case
Panic tests use assert.Panics correctly (selector_test.go:11-25)

Coverage:

Codecov reports 72.34% patch coverage with 13 lines missing
This is reasonable for a feature with retry logic and error handling paths

Suggestions:

Consider adding integration test with actual concurrent DA submissions (if not already covered by E2E)
Test behavior when all addresses fail vs. when round-robin helps (could be part of E2E)

6. Documentation & Developer Experience 📚 Good

Strengths:

Clear config comments explaining purpose (config.go:167)
Interface documentation explains return values (selector.go:10)
Test names are descriptive and follow Go conventions
CLAUDE.md instructions are followed

Documentation updates:

✅ Config documentation updated (docs/learn/config.md)
✅ CLI flag help text added (config.go:334)
✅ YAML comments added to config struct (config.go:167)

Suggestions for improvement:

Add migration guide: Document how existing single-address users can benefit from this feature

# Example in docs:
# Before (single address, potential sequence issues):
da:
  submit_options: '{"signer_address": "celestia1abc..."}'

# After (multiple addresses, no sequence conflicts):
da:
  signing_addresses:
    - "celestia1abc..."
    - "celestia1def..."
    - "celestia1ghi..."
  # submit_options: can now omit signer_address

Clarify interaction with submit_options: Document that signer_address in submit_options will be overridden when signing_addresses is configured (this is the intended behavior but should be explicit)
Add performance note: Document expected throughput improvement (e.g., "Using 3 addresses can support ~3x throughput before hitting sequence issues")

Example configuration: Add to config.md:

da:
  signing_addresses:
    - "celestia1account1..."
    - "celestia1account2..."
    - "celestia1account3..."

Specific Code Issues

Issue 1: Potential confusion with NoOpSelector behavior

Location: pkg/da/selector.go:10

Description: The interface comment says "Implementations may return empty string (NoOpSelector) or panic (RoundRobinSelector with no addresses)" but doesn't explain when each behavior is appropriate.

Recommendation: Clarify the contract:

// AddressSelector defines the interface for selecting a signing address from a list.
type AddressSelector interface {
    // Next returns the next address to use for signing.
    // Returns empty string when no addresses are configured (NoOpSelector),
    // indicating the DA layer should use its default signer.
    // Implementations MUST be safe for concurrent access.
    Next() string
}

Issue 2: Missing defensive check in DASubmitter initialization

Location: block/internal/submitting/da_submitter.go:156-164

Description: If config.DA.SigningAddresses contains empty strings or invalid addresses, the selector will still be created and will cycle through invalid addresses.

Recommendation: Add validation:

// Create address selector based on configuration
var addressSelector pkgda.AddressSelector
if len(config.DA.SigningAddresses) > 0 {
    // Filter out empty addresses
    validAddresses := make([]string, 0, len(config.DA.SigningAddresses))
    for _, addr := range config.DA.SigningAddresses {
        if addr != "" {
            validAddresses = append(validAddresses, addr)
        }
    }
    
    if len(validAddresses) > 0 {
        addressSelector = pkgda.NewRoundRobinSelector(validAddresses)
        daSubmitterLogger.Info().
            Int("num_addresses", len(validAddresses)).
            Msg("initialized round-robin address selector for multi-account DA submissions")
    } else {
        addressSelector = pkgda.NewNoOpSelector()
    }
} else {
    addressSelector = pkgda.NewNoOpSelector()
}

Issue 3: Test naming inconsistency

Location: pkg/da/selector_test.go:126

Description: Test function TestMergeSubmitOptions_NullJSON is in the selector_test file but tests mergeSubmitOptions which is in da_submitter.go.

Wait, checking again...

Actually, I see this test is correctly in block/internal/submitting/da_submitter_options_test.go:126-132, so this is not an issue. ✅

Config Flag Documentation Issue

Location: docs/learn/config.md (based on review comments)

Issue: Review comment from @tac0turtle suggests the flag format should be:

`--evnode.da.signing_addresses <string>`

However, looking at the flag definition in config.go:334, it uses StringSlice:

cmd.Flags().StringSlice(FlagDASigningAddresses, def.DA.SigningAddresses, "Comma-separated list...")

Recommendation: The docs should show the comma-separated format:

`--evnode.da.signing_addresses "addr1,addr2,addr3"`

Or if using multiple flags:

`--evnode.da.signing_addresses addr1 --evnode.da.signing_addresses addr2`

Review Comments Assessment

I reviewed the existing PR comments and here's my assessment:

@gemini-code-assist's panic concern (da_submitter.go:376): ✅ RESOLVED - The nil map check has been added at lines 379-381
@tac0turtle's comments:
- Config comment improvements: ✅ ADDRESSED
- Question about celestia-node behavior with empty signer_address: ✅ CLARIFIED - Uses default (line 454 discussion)
- Defensive checks request: ✅ ADDED (per @randygrok's comment)

Final Recommendations

High Priority

✅ Done: Fix nil map panic (already addressed)
⚠️ Consider: Add address validation in config validation phase
⚠️ Consider: Document flag format clearly (comma-separated vs. multiple flags)

Medium Priority

Add examples to docs showing before/after configuration
Document the override behavior with submit_options
Consider adding metrics for address selection distribution

Low Priority

Add integration/E2E test demonstrating throughput improvement
Consider making panic behavior configurable (panic vs. error return)
Add performance benchmarks for selector operations

Conclusion

This is a high-quality implementation that solves a real problem. The code is clean, well-tested, and properly documented. The main suggestion is to enhance documentation with examples and migration guidance.

Recommendation: ✅ Approve with minor documentation improvements suggested above.

The feature is production-ready and the existing test coverage provides good confidence in its correctness. The round-robin strategy is appropriate for the use case, and the implementation is efficient and thread-safe.

Oct 27 '25 19:10 claude[bot]

Summary of Changes

Hello @randygrok, I'm Gemini Code Assist[^1]! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the Data Availability (DA) submission process by introducing multi-account support with a round-robin address selection strategy. This feature is crucial for maintaining stability and efficiency when interacting with high-throughput DA layers, such as Celestia, by mitigating common issues like sequence mismatches that arise from concurrent transactions. The changes involve new configuration options, a flexible address selection mechanism, and dynamic merging of signing addresses into submission parameters, ensuring a robust and scalable DA submission pipeline.

Highlights

Multi-Account DA Submission Support: Introduced the ability to configure multiple signing addresses for Data Availability (DA) layer submissions, primarily to address sequence mismatches in high-throughput Cosmos SDK-based DA layers like Celestia.
Round-Robin Address Selection: Implemented a thread-safe round-robin mechanism (RoundRobinSelector) to distribute DA submissions across multiple configured signing addresses, ensuring fair usage and preventing nonce/sequence conflicts.
Dynamic Submit Options Merging: Added logic to automatically merge the selected signing address into the DA submit options. If the base options are valid JSON, the signer_address field is added; otherwise, a new JSON object is created with just the signing address.
Configuration Updates: A new configuration option, DA.SigningAddresses, has been added to allow users to specify a list of addresses for DA submissions via YAML or command-line flags. Documentation has been updated accordingly.
Refactored DASubmitter: The DASubmitter now utilizes an AddressSelector interface, allowing for flexible address selection strategies, with RoundRobinSelector and NoOpSelector as initial implementations.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with :thumbsup: and :thumbsdown: on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

[^1]: Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Oct 27 '25 19:10 gemini-code-assist[bot]

PR Preview Action v1.6.2 :---: Preview removed because the pull request was closed. 2025-11-10 10:57 UTC

Oct 27 '25 19:10 github-actions[bot]

Codecov Report

:x: Patch coverage is 73.46939% with 13 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 64.72%. Comparing base (3d49dfc) to head (82a1303). :warning: Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
block/internal/submitting/da_submitter.go	58.06%	8 Missing and 5 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2785      +/-   ##
==========================================
+ Coverage   64.69%   64.72%   +0.03%     
==========================================
  Files          80       81       +1     
  Lines        7177     7224      +47     
==========================================
+ Hits         4643     4676      +33     
- Misses       1999     2009      +10     
- Partials      535      539       +4

Flag	Coverage Δ
combined	`64.72% <73.46%> (+0.03%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

:snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Oct 29 '25 13:10 codecov[bot]