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

feat(node_framework): Support for preconditions and oneshot tasks #1398

Merged
merged 17 commits into from
Mar 18, 2024

Conversation

popzxc
Copy link
Member

@popzxc popzxc commented Mar 11, 2024

This PR consists of three parts in one (sorry for that).

Part 1. Preconditions

An alternative take on #1290

In short:

  • Adds two new concepts to the framework: Precondition and UnconstrainedTask.
  • Any precondition is a future that must successfully resolve before tasks can be started. An example of future precondition may be the readiness of storage after snapshot sync.
  • Any Task will wait until all of the added preconditions successfully resolves.
  • An UnconstrainedTask is a way to opt-out from waiting for preconditions to complete. These may be useful in two cases:
    • If you have a stateless component that should start ASAP (e.g. healthcheck server).
    • If your task is supposed to ensure that precondition is met (e.g. task that does state recovery).

The last point is important, btw: imagine that the node runs as a set of microservices on different machines. The precondition should be present on all machines (e.g. simply wait until it's ensured), but the corresponding unconstrained task should be run on just one machine (since it alters state).

In the current impl there is no way to subscribe to a certain set of preconditions or opt-out from a single precondition. You either have to wait for them all or may rely on none. This is done on purpose to not overcomplicate the publicly facing interface. The expectation is that Task is a working default that people may just pick up and it'll be guaranteed to work. You only need to meet extra complexity when a Task is not enough for you.

Part 2. Oneshot tasks

Adds a notion of oneshot tasks and adds support of running the service in an oneshot mode (e.g. when we only add oneshot tasks and expect the node to exit as soon as all of them resolve).

It comes in two flavors:

  • OneshotTask. An oneshot analog of Task.
  • UnconstrainedOneshotTask. An oneshot analog of UnconstrainedTask. May be useful, for example, to enforce some Precondition and then exit.

This way the logical flow of the node is as follows:

  1. Start all the unconstrained tasks, unconstrained oneshot tasks, and preconditions.
  2. Wait for all the preconditions to resolve.
  3. Start all the tasks and oneshot tasks.
  4. Wait for any of long-running tasks (i.e. not oneshot) to exit.

Part 3. Refactoring

There was a lot of new logic added, so the ZkSyncService::run became pretty messy.
The most notable part here is that now all flavors of tasks are managed by the new Runnables collection that encapsulates the logic of turning different tasks into futures that can be launched by the service.

Copy link
Contributor

@slowli slowli left a comment

Choose a reason for hiding this comment

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

Conceptually looks good. I wonder whether it could make sense to generalize preconditions to oneshot tasks that could be used for programmatic migrations etc. Can be done later, obviously.

core/node/node_framework/src/service/mod.rs Outdated Show resolved Hide resolved
core/node/node_framework/src/service/mod.rs Outdated Show resolved Hide resolved
core/node/node_framework/src/service/mod.rs Outdated Show resolved Hide resolved
core/node/node_framework/src/service/mod.rs Show resolved Hide resolved
@popzxc
Copy link
Member Author

popzxc commented Mar 13, 2024

I wonder whether it could make sense to generalize preconditions to oneshot tasks that could be used for programmatic migrations etc. Can be done later, obviously.

@slowli Yup, that's exactly my plan, but indeed I want to tackle it in a separate PR.

@popzxc popzxc changed the title feat(node_framework): Support for preconditions feat(node_framework): Support for preconditions and oneshot tasks Mar 13, 2024
@popzxc popzxc marked this pull request as ready for review March 13, 2024 13:56
@popzxc
Copy link
Member Author

popzxc commented Mar 18, 2024

@slowli I'm going to merge this to not block any further progress, but if you have any comments -- I'll be happy to work on them in follow-up PRs.

@popzxc popzxc added this pull request to the merge queue Mar 18, 2024
Merged via the queue into main with commit 65ea881 Mar 18, 2024
29 checks passed
@popzxc popzxc deleted the popzxc-node-setup-round-2 branch March 18, 2024 06:50
github-merge-queue bot pushed a commit that referenced this pull request Mar 22, 2024
🤖 I have created a release *beep* *boop*
---


##
[22.0.0](core-v21.1.0...core-v22.0.0)
(2024-03-21)


### ⚠ BREAKING CHANGES

* Use protocol version v22 as latest
([#1432](#1432))

### Features

* add docs for Dal
([#1273](#1273))
([66ceb0b](66ceb0b))
* adds debug_traceBlockByNumber.callFlatTracer
([#1413](#1413))
([d2a5e36](d2a5e36))
* **api:** introduce mempool cache
([#1460](#1460))
([c5d6c4b](c5d6c4b))
* **commitment-generator:** `events_queue` shadow mode
([#1138](#1138))
([9bb47fa](9bb47fa))
* **contract-verifier:** Allow sc code reverification
([#1455](#1455))
([5a37b42](5a37b42))
* **database:** add an optional master replica max connections settings
([#1472](#1472))
([e5c8127](e5c8127))
* **db:** Configurable maximum number of open RocksDB files
([#1401](#1401))
([b00c052](b00c052))
* **en:** Check recipient contract and function selector in consistency
checker ([#1367](#1367))
([ea5c684](ea5c684))
* Follow-up for DAL split
([#1464](#1464))
([c072288](c072288))
* **node_framework:** Ergonomic improvements
([#1453](#1453))
([09b6887](09b6887))
* **node_framework:** Only store each wiring layer once
([#1468](#1468))
([4a393dc](4a393dc))
* **node_framework:** Support for preconditions and oneshot tasks
([#1398](#1398))
([65ea881](65ea881))
* **node-framework:** Add eth sender layer
([#1390](#1390))
([0affdf8](0affdf8))
* **node-framework:** Add housekeeper layer
([#1409](#1409))
([702e739](702e739))
* Remove batch dry_run
([#1076](#1076))
([b82d093](b82d093))
* Separate Prover and Server DAL
([#1334](#1334))
([103a56b](103a56b))
* **storage:** Make the storage caches task cancellation aware
([#1430](#1430))
([ab532bb](ab532bb))
* support running consensus from snapshot (BFT-418)
([#1429](#1429))
([f9f4d38](f9f4d38))
* **tx-sender:** Limit concurrent tx submissions
([#1473](#1473))
([4bdf3ca](4bdf3ca))
* Use protocol version v22 as latest
([#1432](#1432))
([1757412](1757412))
* **vm:** Prestate tracer implementation
([#1306](#1306))
([c36be65](c36be65))


### Bug Fixes

* **core:** drop correct index of proof_generation_details table during
database migration
([#1199](#1199))
([76a6821](76a6821))
* **dal:** correct `tx_index_in_l1_batch` in
`get_vm_events_for_l1_batch`
([#1463](#1463))
([8bf37ac](8bf37ac))
* **node_framework:** Fix main node example
([#1470](#1470))
([ac4a744](ac4a744))
* **protocol:** Remove verifier address from protocol upgrade
([#1443](#1443))
([90dee73](90dee73))
* **reth:** use reth instead of geth
([#1410](#1410))
([bd98dc7](bd98dc7))
* **verified sources fetcher:** Use correct `contact_name` for
`SolSingleFile`
([#1433](#1433))
([0764227](0764227))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
montekki pushed a commit that referenced this pull request Mar 22, 2024
🤖 I have created a release *beep* *boop*
---


##
[22.0.0](core-v21.1.0...core-v22.0.0)
(2024-03-21)


### ⚠ BREAKING CHANGES

* Use protocol version v22 as latest
([#1432](#1432))

### Features

* add docs for Dal
([#1273](#1273))
([66ceb0b](66ceb0b))
* adds debug_traceBlockByNumber.callFlatTracer
([#1413](#1413))
([d2a5e36](d2a5e36))
* **api:** introduce mempool cache
([#1460](#1460))
([c5d6c4b](c5d6c4b))
* **commitment-generator:** `events_queue` shadow mode
([#1138](#1138))
([9bb47fa](9bb47fa))
* **contract-verifier:** Allow sc code reverification
([#1455](#1455))
([5a37b42](5a37b42))
* **database:** add an optional master replica max connections settings
([#1472](#1472))
([e5c8127](e5c8127))
* **db:** Configurable maximum number of open RocksDB files
([#1401](#1401))
([b00c052](b00c052))
* **en:** Check recipient contract and function selector in consistency
checker ([#1367](#1367))
([ea5c684](ea5c684))
* Follow-up for DAL split
([#1464](#1464))
([c072288](c072288))
* **node_framework:** Ergonomic improvements
([#1453](#1453))
([09b6887](09b6887))
* **node_framework:** Only store each wiring layer once
([#1468](#1468))
([4a393dc](4a393dc))
* **node_framework:** Support for preconditions and oneshot tasks
([#1398](#1398))
([65ea881](65ea881))
* **node-framework:** Add eth sender layer
([#1390](#1390))
([0affdf8](0affdf8))
* **node-framework:** Add housekeeper layer
([#1409](#1409))
([702e739](702e739))
* Remove batch dry_run
([#1076](#1076))
([b82d093](b82d093))
* Separate Prover and Server DAL
([#1334](#1334))
([103a56b](103a56b))
* **storage:** Make the storage caches task cancellation aware
([#1430](#1430))
([ab532bb](ab532bb))
* support running consensus from snapshot (BFT-418)
([#1429](#1429))
([f9f4d38](f9f4d38))
* **tx-sender:** Limit concurrent tx submissions
([#1473](#1473))
([4bdf3ca](4bdf3ca))
* Use protocol version v22 as latest
([#1432](#1432))
([1757412](1757412))
* **vm:** Prestate tracer implementation
([#1306](#1306))
([c36be65](c36be65))


### Bug Fixes

* **core:** drop correct index of proof_generation_details table during
database migration
([#1199](#1199))
([76a6821](76a6821))
* **dal:** correct `tx_index_in_l1_batch` in
`get_vm_events_for_l1_batch`
([#1463](#1463))
([8bf37ac](8bf37ac))
* **node_framework:** Fix main node example
([#1470](#1470))
([ac4a744](ac4a744))
* **protocol:** Remove verifier address from protocol upgrade
([#1443](#1443))
([90dee73](90dee73))
* **reth:** use reth instead of geth
([#1410](#1410))
([bd98dc7](bd98dc7))
* **verified sources fetcher:** Use correct `contact_name` for
`SolSingleFile`
([#1433](#1433))
([0764227](0764227))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants