feat: merge-train/fairies-v5#24632
Merged
Merged
Conversation
waitForNode called retryUntil without a timeout, and retryUntil treats a
zero timeout as the never-time-out sentinel, so an unreachable node caused
the call to poll getNodeInfo forever with no way for the caller to bound it.
Add an optional { timeout, interval } argument and default it to
DefaultWaitOpts (300s / 1s), matching the sibling waitForTx. An unreachable
node now rejects with a TimeoutError; pass timeout: 0 to opt into the
previous infinite-wait behavior.
Fixes #24602. ## Problem `waitForNode` polls `getNodeInfo()` forever when the node is unreachable, with no way for the caller to bound the wait: - It calls `retryUntil(fn, 'RPC Get Node Info')` — passing no `timeout`. - `retryUntil` defaults `timeout = 0`, and `0` is the documented "never time out" sentinel, so the timeout branch is dead. - `waitForNode` exposed no parameter to override this, so a caller could not cap the wait at all. The result: a dApp/CLI/service pointed at a wrong URL or a down node hangs silently at startup, indistinguishable from "node still starting." This is also inconsistent with the sibling `waitForTx` in the same file, which forwards `opts?.timeout ?? DefaultWaitOpts.timeout` (300s). ## Fix Add an optional `{ timeout, interval }` argument to `waitForNode` and default it to `DefaultWaitOpts` (300s / 1s), mirroring `waitForTx`. An unreachable node now rejects with a `TimeoutError`. ## Behavioral change The default wait changes from infinite to 300s. Callers that relied on waiting indefinitely can restore the old behavior by passing `{ timeout: 0 }`. 300s is generous for a reachable node and matches the existing `waitForTx` / `DefaultWaitOpts` default. ## Tests Added to `node.test.ts` (reusing the existing `jest-mock-extended` style): - resolves once the node becomes reachable - rejects with a `TimeoutError` when the node stays unreachable (bounded)
First part of integrating the enhancements explored at aztec-labs-eng/oxide#257 Closes F-808
) Fixes [F-680](https://linear.app/aztec-labs/issue/F-680/migrate-pxe-tagging-stores-to-prefixed-apptaggingsecret-keys) ## What changed Migrates PXE tagging-store keys off the legacy two-part `AppTaggingSecret` string format (`secret:app`) so every persisted key uses the uniform self-describing `kind:secret:app` form. - `AppTaggingSecret.toString()` now always emits `kind:secret:app` (the unconstrained special case that emitted the legacy two-part key is gone). - `AppTaggingSecret.fromString()` only accepts the three-part form; a two-part legacy key now throws instead of parsing. - Bumped `PXE_DATA_SCHEMA_VERSION` 12 → 13. ## Migration strategy (breaking) Since the general PXE migration framework does not exist, and per the issue discussion, this cuts over via the schema-version bump rather than a dual-read / self-healing path. `initStoreForRollupAndSchemaVersion` wipes any DB whose stored schema version differs from the current one on open, so a DB written with legacy keys is cleared before the new parser ever reads it. **Blast radius:** the wipe resets the *entire* PXE DB (addresses, notes, contracts, key store, L2 tips, facts, and tagging), not just tagging data, because they share one backing KV store. Existing wallets re-sync from genesis after upgrade. This is inherent to any `PXE_DATA_SCHEMA_VERSION` bump. ## Tests - `app_tagging_secret.test.ts`: pins the new three-part unconstrained `toString()`, adds a "rejects the legacy two-part format" case, drops the now-redundant kind-prefixed-unconstrained test. - `pxe_db_compatibility.test.ts`: adds a pre-migration wipe test that writes a raw legacy two-part key at schema v12, reopens at v13, and asserts the raw map is empty (it asserts raw map contents rather than a high-level getter, which would false-pass since a new three-part `toString()` never reconstructs a legacy key).
Adds an oracle to check whether a collection of block hashes is present in the archiver tree as of a given block. An upstream port of: aztec-labs-eng/oxide#257 Note: I considered making this batch call return something more "interesting" than existence booleans (such as MembershipWitnesses), but that would potentially be heavy over the wire and there's no current evidence of it being needed. We can add a new oracle for that if turns out to be useful in the future. Closes F-810
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
1 similar comment
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BEGIN_COMMIT_OVERRIDE
fix(aztec.js): give waitForNode a bounded default timeout (#24627)
refactor: cache Aztec node reads per execution (#24630)
feat(pxe)!: Add AppTaggingSecret kinds to keys in tagging stores (#24604)
feat: add batch is block in archive oracle (#24634)
END_COMMIT_OVERRIDE