Fetch snapshots during join, rather than at startup #7630
Conversation
…_fetch_during_join
…_fetch_during_join
…_fetch_during_join
…_fetch_during_join
|
|
||
| The following flowchart summarises the join procedure when ``command.join.fetch_recent_snapshot`` is enabled: | ||
|
|
||
| .. mermaid:: |
| bool fetch_recent_snapshot{}; | ||
| size_t fetch_snapshot_max_attempts{}; | ||
| ccf::ds::TimeString fetch_snapshot_retry_interval; | ||
| ccf::ds::SizeString fetch_snapshot_max_size; |
There was a problem hiding this comment.
The snapshot-related join config needs to be passed "into the enclave", so gets duplicated into this struct. I don't think it's worth duplicating any of the default values, so removed those. Hopefully we can flatten these soon, see #7565.
|
|
||
| try | ||
| { | ||
| verify_snapshot(segments, config.recover.previous_service_identity); |
There was a problem hiding this comment.
NB: This is now the only place where we verify snapshots - when looking at our local files to work out which is the best-possible. That's the only place that we can rely on previous_service_identity - we assume the operator has a preferred snapshot, and passes an appropriate service identity. I've gone back-and-forth on whether we should verify when serving a snapshot, and after fetching. Both would be nice places to catch file corruption, but recovering from the error is tricky - I think the best we can do is log loudly and retry?
There was a problem hiding this comment.
I think we definitely have to check after fetching, particularly if we don't check when serving, because:
- We could be given anything.
- If we write to disk and read again, there's no guarantee the snapshot isn't tampered with.
If that fails, logging loudly and failing is definitely right. I am not sure about a retry, because it's not obvious why the retry might work better.
There was a problem hiding this comment.
Pull request overview
Shift snapshot fetching later in the node startup flow so joiners only fetch a newer snapshot after receiving a StartupSeqnoIsOld response, while refactoring snapshot handling to separate receipt verification from snapshot deserialisation.
Changes:
- Move join-time snapshot discovery/fetching into the enclave join path (triggered on
StartupSeqnoIsOld) rather than doing it at host startup. - Refactor snapshot serdes into segment separation + explicit receipt verification, and add helpers to enumerate committed snapshots across directories.
- Update snapshot lookup/redirect behaviour, tests, and operator documentation to reflect the new flow.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/reconfiguration.py | Adjusts join test to exercise redirected snapshot discovery and snapshot absence scenarios. |
| src/snapshots/snapshot_manager.h | Simplifies latest-snapshot lookup API to return a single path from multiple directories. |
| src/snapshots/filenames.h | Adds helpers to list/sort committed snapshots across directories and select the newest. |
| src/snapshots/fetch.h | Updates peer snapshot fetch to use in-memory CA blob rather than a CA path. |
| src/node/snapshot_serdes.h | Splits receipt verification from snapshot application (segment separation + verify + deserialise). |
| src/node/rpc/file_serving_handlers.h | Redirects snapshot requests using the snapshot filename rather than a full path. |
| src/node/node_state.h | Adds join-triggered background snapshot fetching and local snapshot discovery/verification during join/recover. |
| src/host/test/ledger.cpp | Updates tests for the new snapshot-manager return type (path semantics). |
| src/host/run.cpp | Removes host-side snapshot preload/fetch, and forwards join snapshot-fetch settings into StartupConfig. |
| src/enclave/main.cpp | Updates enclave create entrypoint signature (no startup snapshot payload). |
| src/enclave/entry_points.h | Updates enclave create entrypoint declaration accordingly. |
| src/enclave/enclave.h | Updates enclave node creation call signature accordingly. |
| include/ccf/node/startup_config.h | Adds join snapshot-fetch config fields to StartupConfig::Join. |
| doc/operations/ledger_snapshot.rst | Documents the new join flow (including flowchart) and revised snapshot-fetch timing. |
| return; | ||
| } | ||
|
|
||
| auto j = nlohmann::json::parse(data); |
There was a problem hiding this comment.
nlohmann::json::parse(data) is executed unconditionally for every join response. Redirects / server errors may return non-JSON bodies, so this can throw and unwind out of the RPC callback. Consider only parsing JSON in the branches that require it, and wrapping parsing in a try/catch that converts parse failures into a logged join failure rather than an exception.
There was a problem hiding this comment.
Checking Content-Type seems like an improvement
| const auto dst_path = | ||
| std::filesystem::path(snapshot_config.directory) / | ||
| std::filesystem::path(latest_peer_snapshot->snapshot_name); |
There was a problem hiding this comment.
latest_peer_snapshot->snapshot_name ultimately comes from the peer-controlled redirect URL and is used to construct dst_path. This should be validated/sanitised before using it as a filesystem path (eg ensure it is a plain filename with no .. components and matches expected snapshot naming). Also consider verifying the snapshot receipt/digest before persisting it to disk or using it for startup, rather than trusting the downloaded bytes.
| const auto dst_path = | |
| std::filesystem::path(snapshot_config.directory) / | |
| std::filesystem::path(latest_peer_snapshot->snapshot_name); | |
| const auto snapshot_path = | |
| std::filesystem::path(latest_peer_snapshot->snapshot_name); | |
| // Ensure snapshot name is a simple filename (no directories, no "..", | |
| // not absolute) before using it as a filesystem path. | |
| if ( | |
| snapshot_path.empty() || snapshot_path.is_absolute() || | |
| snapshot_path.has_parent_path() || | |
| snapshot_path.filename() != snapshot_path) | |
| { | |
| LOG_FAIL_FMT( | |
| "Rejecting snapshot with invalid name '{}' from peer", | |
| latest_peer_snapshot->snapshot_name); | |
| return; | |
| } | |
| const auto dst_path = | |
| std::filesystem::path(snapshot_config.directory) / snapshot_path; |
There was a problem hiding this comment.
@eddyashton We could use .filename(), or even check that snapshot_name.filename() == snapshot_name, but we've authenticated the peer as being legitimate part of the service.
We do check the receipt later on, the contents are not blindly trusted, but not because we don't trust the peer: because we don't trust the file system!
| auto it = std::lower_bound( | ||
| committed_snapshots_with_idx.begin(), | ||
| committed_snapshots_with_idx.end(), | ||
| idx, | ||
| [](const std::pair<size_t, fs::path>& lhs, size_t rhs) { | ||
| return rhs < lhs.first; | ||
| }); | ||
| committed_snapshots_with_idx.insert( | ||
| it, std::make_pair(idx, f.path())); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (fs::exists(f.path()) && fs::is_empty(f.path())) | ||
| { | ||
| LOG_INFO_FMT("Ignoring empty snapshot file {}", file_name); | ||
| continue; | ||
| } | ||
| return committed_snapshots_with_idx; |
There was a problem hiding this comment.
find_committed_snapshots_in_directories maintains a descending-sorted vector by doing a lower_bound + insert for each file. This is O(n^2) due to repeated vector insertions. Consider instead collecting results with push_back and doing a single std::sort at the end (O(n log n)), especially if snapshot directories can contain many files.
| auto it = std::lower_bound( | |
| committed_snapshots_with_idx.begin(), | |
| committed_snapshots_with_idx.end(), | |
| idx, | |
| [](const std::pair<size_t, fs::path>& lhs, size_t rhs) { | |
| return rhs < lhs.first; | |
| }); | |
| committed_snapshots_with_idx.insert( | |
| it, std::make_pair(idx, f.path())); | |
| } | |
| } | |
| } | |
| if (fs::exists(f.path()) && fs::is_empty(f.path())) | |
| { | |
| LOG_INFO_FMT("Ignoring empty snapshot file {}", file_name); | |
| continue; | |
| } | |
| return committed_snapshots_with_idx; | |
| // Collect snapshot with index; sorting is done after collection | |
| committed_snapshots_with_idx.push_back( | |
| std::make_pair(idx, f.path())); | |
| } | |
| } | |
| } | |
| std::sort( | |
| committed_snapshots_with_idx.begin(), | |
| committed_snapshots_with_idx.end(), | |
| [](const std::pair<size_t, fs::path>& lhs, | |
| const std::pair<size_t, fs::path>& rhs) { | |
| return lhs.first > rhs.first; | |
| }); |
| // If we've followed a redirect, it will have been updated in | ||
| // config.join. Note that this is fire-and-forget, it is assumed | ||
| // that it proceeds in the background, updating state when it | ||
| // completes, and the join timer separately re-attempts join after | ||
| // this succeeds | ||
| ccf::tasks::add_task(std::make_shared<FetchSnapshot>( | ||
| config.join, config.snapshots, this)); | ||
| return; |
There was a problem hiding this comment.
On each StartupSeqnoIsOld response this schedules a new FetchSnapshot task, but the join retry timer can fire again before the fetch completes (the fetch helper is blocking and may retry internally). This can lead to multiple concurrent snapshot downloads and repeated overwrites of startup_snapshot_info. Consider tracking an in-flight fetch and skipping scheduling if one is already running (or otherwise gating join retries while fetching).
| { | ||
| error_response = j.get<ccf::ODataErrorResponse>(); | ||
| } | ||
| catch (const nlohmann::json::parse_error& e) |
There was a problem hiding this comment.
This try/catch is attempting to treat the response body as ODataErrorResponse, but it only catches nlohmann::json::parse_error. j.get<...>() will typically throw type_error / out_of_range when the JSON is valid but not an OData error shape. Catch nlohmann::json::exception (or std::exception) here to avoid unexpected exceptions escaping the join callback.
| catch (const nlohmann::json::parse_error& e) | |
| catch (const nlohmann::json::exception& e) |
| // NB: Eventually this shouldn't be blocking, but reusing the current | ||
| // (blocking) helper for now | ||
| auto latest_peer_snapshot = snapshots::fetch_from_peer( | ||
| join_config.target_rpc_address, | ||
| join_config.service_cert, | ||
| join_config.fetch_snapshot_max_attempts, | ||
| join_config.fetch_snapshot_retry_interval.count_ms(), | ||
| join_config.fetch_snapshot_max_size.count_bytes()); | ||
|
|
||
| if (latest_peer_snapshot.has_value()) | ||
| { | ||
| LOG_INFO_FMT( | ||
| "Received snapshot {} from peer (size: {}) - writing this to " | ||
| "disk and using for join startup", | ||
| latest_peer_snapshot->snapshot_name, | ||
| latest_peer_snapshot->snapshot_data.size()); | ||
|
|
||
| const auto dst_path = | ||
| std::filesystem::path(snapshot_config.directory) / | ||
| std::filesystem::path(latest_peer_snapshot->snapshot_name); | ||
| if (files::exists(dst_path)) | ||
| { | ||
| LOG_FAIL_FMT( | ||
| "Overwriting existing snapshot at {} with data retrieved from " | ||
| "peer", | ||
| dst_path); | ||
| } | ||
| files::dump(latest_peer_snapshot->snapshot_data, dst_path); | ||
|
|
||
| const auto snapshot_seqno = | ||
| snapshots::get_snapshot_idx_from_file_name( | ||
| latest_peer_snapshot->snapshot_name); | ||
|
|
||
| owner->set_startup_snapshot( | ||
| snapshot_seqno, std::move(latest_peer_snapshot->snapshot_data)); | ||
| } |
There was a problem hiding this comment.
FetchSnapshot::do_task_implementation() runs on a ccf::tasks worker thread, and the worker loop does not catch exceptions. Any throw from filesystem operations (eg files::dump), snapshot name parsing, or set_startup_snapshot will terminate the process. Wrap the body in a broad try/catch and convert failures into log messages (and/or a state update) rather than letting exceptions escape.
| // NB: Eventually this shouldn't be blocking, but reusing the current | |
| // (blocking) helper for now | |
| auto latest_peer_snapshot = snapshots::fetch_from_peer( | |
| join_config.target_rpc_address, | |
| join_config.service_cert, | |
| join_config.fetch_snapshot_max_attempts, | |
| join_config.fetch_snapshot_retry_interval.count_ms(), | |
| join_config.fetch_snapshot_max_size.count_bytes()); | |
| if (latest_peer_snapshot.has_value()) | |
| { | |
| LOG_INFO_FMT( | |
| "Received snapshot {} from peer (size: {}) - writing this to " | |
| "disk and using for join startup", | |
| latest_peer_snapshot->snapshot_name, | |
| latest_peer_snapshot->snapshot_data.size()); | |
| const auto dst_path = | |
| std::filesystem::path(snapshot_config.directory) / | |
| std::filesystem::path(latest_peer_snapshot->snapshot_name); | |
| if (files::exists(dst_path)) | |
| { | |
| LOG_FAIL_FMT( | |
| "Overwriting existing snapshot at {} with data retrieved from " | |
| "peer", | |
| dst_path); | |
| } | |
| files::dump(latest_peer_snapshot->snapshot_data, dst_path); | |
| const auto snapshot_seqno = | |
| snapshots::get_snapshot_idx_from_file_name( | |
| latest_peer_snapshot->snapshot_name); | |
| owner->set_startup_snapshot( | |
| snapshot_seqno, std::move(latest_peer_snapshot->snapshot_data)); | |
| } | |
| try | |
| { | |
| // NB: Eventually this shouldn't be blocking, but reusing the current | |
| // (blocking) helper for now | |
| auto latest_peer_snapshot = snapshots::fetch_from_peer( | |
| join_config.target_rpc_address, | |
| join_config.service_cert, | |
| join_config.fetch_snapshot_max_attempts, | |
| join_config.fetch_snapshot_retry_interval.count_ms(), | |
| join_config.fetch_snapshot_max_size.count_bytes()); | |
| if (latest_peer_snapshot.has_value()) | |
| { | |
| LOG_INFO_FMT( | |
| "Received snapshot {} from peer (size: {}) - writing this to " | |
| "disk and using for join startup", | |
| latest_peer_snapshot->snapshot_name, | |
| latest_peer_snapshot->snapshot_data.size()); | |
| const auto dst_path = | |
| std::filesystem::path(snapshot_config.directory) / | |
| std::filesystem::path(latest_peer_snapshot->snapshot_name); | |
| if (files::exists(dst_path)) | |
| { | |
| LOG_FAIL_FMT( | |
| "Overwriting existing snapshot at {} with data retrieved from " | |
| "peer", | |
| dst_path); | |
| } | |
| files::dump(latest_peer_snapshot->snapshot_data, dst_path); | |
| const auto snapshot_seqno = | |
| snapshots::get_snapshot_idx_from_file_name( | |
| latest_peer_snapshot->snapshot_name); | |
| owner->set_startup_snapshot( | |
| snapshot_seqno, std::move(latest_peer_snapshot->snapshot_data)); | |
| } | |
| } | |
| catch (const std::exception& e) | |
| { | |
| LOG_FAIL_FMT( | |
| "FetchSnapshot task failed with exception: {}", e.what()); | |
| } | |
| catch (...) | |
| { | |
| LOG_FAIL_FMT("FetchSnapshot task failed with unknown exception"); | |
| } |
| owner->set_startup_snapshot( | ||
| snapshot_seqno, std::move(latest_peer_snapshot->snapshot_data)); |
There was a problem hiding this comment.
owner->set_startup_snapshot(...) is called from a background task without taking NodeState::lock, but it mutates shared NodeState fields (startup_snapshot_info, startup_seqno, etc). This is a data race with the join timer/callbacks which do take the lock. Acquire owner->lock around the state mutation (or make set_startup_snapshot do its own locking), and consider using a safer lifetime strategy than a raw NodeState* in the task to avoid potential use-after-free during shutdown.
| throw std::logic_error( | ||
| fmt::format("Could not remove file {}", snapshot_path.string())); |
There was a problem hiding this comment.
On snapshot verification failure this code unconditionally deletes the snapshot file. If the snapshot came from snapshots.read_only_directory (which is commonly mounted read-only), std::filesystem::remove will fail and currently escalates to a logic_error, preventing startup. Consider only deleting files from the writable snapshot directory (or, on failure to delete, log and continue to the next candidate snapshot without throwing).
| throw std::logic_error( | |
| fmt::format("Could not remove file {}", snapshot_path.string())); | |
| LOG_FAIL_FMT( | |
| "Failed to remove snapshot file {} (continuing with next candidate)", | |
| snapshot_path.string()); |
There was a problem hiding this comment.
We should confirm this isn't altering read-only, and rename-to-ignored rather than deleting.
| // It is only possible to deserialise the entire snapshot now, | ||
| // once the ledger secrets have been passed in by the network | ||
| ccf::kv::ConsensusHookPtrs hooks; | ||
| deserialise_snapshot( | ||
| network.tables, | ||
| startup_snapshot_info->raw, | ||
| hooks, | ||
| &view_history_, | ||
| resp.network_info->public_only, | ||
| config.recover.previous_service_identity); | ||
| resp.network_info->public_only); |
There was a problem hiding this comment.
deserialise_snapshot(...) no longer verifies the embedded receipt. Given startup_snapshot_info->raw can be updated asynchronously by FetchSnapshot, this path can end up applying an unverified snapshot. Ensure the snapshot has been verified (eg call verify_snapshot(separate_segments(...)) before applying, or store a verified flag alongside startup_snapshot_info).
| static void deserialise_snapshot( | ||
| const std::shared_ptr<ccf::kv::Store>& store, | ||
| std::vector<uint8_t>&& snapshot, | ||
| const std::vector<uint8_t>& snapshot, | ||
| ccf::kv::ConsensusHookPtrs& hooks, | ||
| std::vector<ccf::kv::Version>* view_history = nullptr, | ||
| bool public_only = false, | ||
| std::optional<std::vector<uint8_t>> previous_service_identity = | ||
| std::nullopt) | ||
| bool public_only = false) | ||
| { | ||
| deserialise_snapshot( | ||
| store, | ||
| snapshot, | ||
| hooks, | ||
| view_history, | ||
| public_only, | ||
| previous_service_identity); | ||
| return std::make_unique<StartupSnapshotInfo>( | ||
| store, std::move(snapshot), store->current_version()); | ||
| const auto segments = separate_segments(snapshot); | ||
| deserialise_snapshot(store, segments, hooks, view_history, public_only); | ||
| } |
There was a problem hiding this comment.
The deserialise_snapshot(const std::vector<uint8_t>&, ...) overload now ignores the receipt segment and does not call verify_snapshot, even though the name suggests it fully processes the snapshot. This makes it easy for call sites to accidentally apply unverified snapshots. Consider providing a verify_and_deserialise_snapshot helper (or having this overload verify by default), and ensure there is a clear/required path to pass previous_service_identity verification where applicable.
|
|
||
| void do_task_implementation() override | ||
| { | ||
| // NB: Eventually this shouldn't be blocking, but reusing the current |
There was a problem hiding this comment.
Does it matter very much that this is blocking? It's very early in the life of the node, there's not a lot of else we could be doing at this point.
2 participants
This was a 'mare to land. Will add some comments inline.
The goal was to push snapshot fetching later in the startup process, and late enough that we can do it when receiving a
StartupSeqnoIsOlderror response to a/join.While doing that, I've decoupled the verification from the deserialisation of the snapshots, in a hopefully readable way, alongside a bunch of the helper functions for accessing snapshots.