fix(factory): extend persistent TTL on stream_address read to prevent registry archival - #499
Open
scarface-dev1 wants to merge 1 commit into
Open
Conversation
… registry archival The bounded TTL walker (bump_persistent_bucket) can only bump 8 StreamAddr(id) entries per maintenance call. For a factory with thousands of streams, this is insufficient — entries archive before they can be bumped, causing stream_address(id) to silently return None for streams that provably exist. Fix by extending the persistent TTL on every stream_address read, matching the pattern used by index page reads (extend_page_ttls). Frequently-resolved streams stay alive without relying solely on the bounded walker. Also fixes pre-existing build errors: - stream/errors.rs: added missing InvalidRecipient, BackdatedStream, StreamUnderfunded error variants - factory/index.rs: fixed streams_by_sender return type (Vec<u64> -> StreamPage) and captured read_index result as ids - factory/tests.rs: updated assertions to use page.ids.len() / page.ids.get() Closes conduit-protocol#407
|
@scarface-dev1 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
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.
What this fixes
stream_address(id)silently returnsNonefor streams that provably exist (stream_id < stream_count). TheStreamAddr(id)persistent entries — the id → deployed-address registry — archive because the bounded TTL walker (bump_persistent_bucket) can only bump 8 entries per maintenance call, which is insufficient for any factory with more than a few hundred streams.Root cause
The
StreamAddr(id)persistent entries have a TTL that must be periodically extended or they archive (get deleted from the live ledger). There are two mechanisms for extending them:create_stream— extends the TTL of the newly created entry only. Does nothing for existing entries.bump_persistent_bucket— the bounded walker, called frompause/unpause/upgrade_stream_wasm. WalksBATCH_LIMIT = 8entries per call, wrapping moduloStreamCount.For a factory with N streams, keeping all entries alive within one TTL window (~200k ledgers) requires roughly N/8 maintenance calls. Nobody makes thousands of
pause/unpause/upgrade_stream_wasmcalls per window, so entries inevitably archive.The index pages (
BySenderPage,ByRecipientPage) solved this exact problem withextend_page_ttls— a function that refreshes every populated page's TTL on every read/append. The primaryStreamAddrregistry had no equivalent.The fix and why
Extend the persistent TTL on every
stream_addressread, matching the pattern used by index page reads:Why this works:
stream_addressis the primary read path — every frontend/SDK resolution goes through it. Frequently-resolved streams stay alive naturally.has/is_some()check is defensive:extend_ttlon a missing entry is a no-op, but guarding it avoids paying for a storage read + TTL extension on non-existent entries (e.g., during pagination of IDs that haven't been created yet).extend_page_ttlsin the index module — read-through TTL extension for persistent entries.Why the bounded walker alone is insufficient:
BATCH_LIMIT = 8, a factory with 10,000 streams needs 1,250 walker calls per TTL window. Even if everypause/unpause/upgradecall drove the walker, that's far fewer than 1,250.How it was tested
cargo clippy -p drip-factory -- -D warnings— zero warnings.cargo test -p drip-factory— all 35 tests pass (including legacy migration, TTL refresh, cancel batch, protocol fee, upgrade, and pause tests).cargo test -p drip-stream— all 93 stream tests pass (verifying the error variant additions don't break existing behavior).The TTL extension is a Soroban host-level operation (
extend_ttl) that cannot be directly unit-tested in the Soroban test VM without setting up specific ledger TTL parameters. The fix follows the exact same pattern validated across the index module'sextend_page_ttls.Follow-up worth filing separately
env.ledger().set()with specificmin_persistent_entry_ttl/max_entry_ttlvalues andenv.as_contract()to read back the TTL afterstream_addresscalls, confirming the extension actually took effect.stream_addresses(batch resolver) — the batch version callsstream_addressper ID, so it already benefits from this fix. But if the batch resolver is ever refactored to bypassstream_address, it would need its own TTL extension.bump_persistent_bucketcost — with the read-path fix, the walker's role shifts to idle-period maintenance. Consider whetherBATCH_LIMITcan be increased (or the walker removed entirely) now that active reads keep entries alive.Closes #407