Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor ScanTask in zebra-scan #8250

Open
Tracked by #7728
arya2 opened this issue Feb 8, 2024 · 2 comments
Open
Tracked by #7728

Refactor ScanTask in zebra-scan #8250

arya2 opened this issue Feb 8, 2024 · 2 comments
Labels
A-blockchain-scanner Area: Blockchain scanner of shielded transactions A-concurrency Area: Async code, needs extra work to make it work properly. C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement C-tech-debt Category: Code maintainability issues

Comments

@arya2
Copy link
Contributor

arya2 commented Feb 8, 2024

Motivation

The scan task in zebra-scan is could be simplified and more maintainable if it were refactored, possibly into a new tower service or by adding some of its logic to the existing ScanService.

This would make it easier to:

  • Implement efficient batch scanning
  • Register keys immediately (it's currently only checking for new registered keys before scanning a block in the root scan task, but not while it's scanning a block)
  • Subscribe to results for a block height while it's still being scanned (so that simultaneous Results and SubscribeResults requests will catch any results for a given key without extra logic)

Possible Design

A design meeting could be worthwhile to discuss possible designs. We may want to consider the Downloads service in the inbound service and/or the QueuedBlocks struct in the state service.

Example:

We could replace the scan_task field on the ScanService with scan_tasks

struct ScanTasks {
    /// Watch channel of senders for subscribed clients
    result_senders: tokio::sync::watch::Sender<HashMap<String, Sender<()>>>,

    /// Scan tasks by id
    tasks_by_id: HashMap<ScanTaskId, ScanTask>,

    /// tasks scanning transactions for a given key
    by_key: HashMap<String, Vec<ScanTaskId>>,

    /// tasks by the next block height that they will scan
    by_height: BTreeMap<Height, ScanTaskId>,
}
@arya2 arya2 added C-enhancement Category: This is an improvement C-cleanup Category: This is a cleanup A-concurrency Area: Async code, needs extra work to make it work properly. C-tech-debt Category: Code maintainability issues A-blockchain-scanner Area: Blockchain scanner of shielded transactions P-High 🔥 labels Feb 8, 2024
@arya2 arya2 removed the P-High 🔥 label Feb 8, 2024
@arya2
Copy link
Contributor Author

arya2 commented Feb 12, 2024

We may want to do #7934 in the same PR as this one to avoid changing a lot of the scan task twice, we can do #7927 first to prepare for #7934.

@upbqdn
Copy link
Member

upbqdn commented Feb 14, 2024

There's a discussion about the new design in #8271.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-blockchain-scanner Area: Blockchain scanner of shielded transactions A-concurrency Area: Async code, needs extra work to make it work properly. C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement C-tech-debt Category: Code maintainability issues
Projects
Status: Product Backlog
Development

No branches or pull requests

2 participants