Implement generating and using for State Sync#738
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates protocol artifact lifecycle tracking to align with State Sync by renaming/reframing the in-memory states from reserved/used to generating/using, and removing presignature recycling behavior.
Changes:
- Replace
reserve/usedsemantics withcreate_slot+generating/usingtracking in protocol storage. - Remove presignature recycling paths and update signature flow to discard unusable presignatures.
- Update integration tests/benchmarks/fixtures to use
create_slotand validateusingmarkers.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-tests/tests/cases/store.rs | Updates persistence tests to use create_slot and assert using markers. |
| integration-tests/tests/cases/helpers.rs | Updates helper insertion utilities to use create_slot. |
| integration-tests/src/mpc_fixture/builder.rs | Updates fixture seeding logic to use create_slot. |
| integration-tests/src/containers.rs | Updates Redis preloading path to use create_slot. |
| integration-tests/benches/store.rs | Updates benchmarks to use create_slot. |
| chain-signatures/node/src/storage/protocol_storage.rs | Reworks storage state tracking to generating/using, removes recycling/reservation APIs, adjusts pruning script. |
| chain-signatures/node/src/protocol/triple.rs | Switches triple generation to create_slot; adds throttled warnings when not enough active participants. |
| chain-signatures/node/src/protocol/signature.rs | Removes presignature recycling and changes behavior to discard on inactivity/REJECTs/timeouts. |
| chain-signatures/node/src/protocol/presignature.rs | Switches presignature generation to create_slot; uses contains_generating; adds throttled warnings. |
Comments suppressed due to low confidence (1)
chain-signatures/node/src/storage/protocol_storage.rs:345
- The Lua script in
insert()usesHSETunconditionally, so if an artifact id already exists it will be overwritten. Previouslyreserve()guarded against this by checkingHEXISTSand refusing the reservation. With the new API, please add an explicit non-overwrite guarantee (e.g., checkHEXISTS/useHSETNXin the script and treat collisions as an error) so state sync/restarts can't silently corrupt stored artifacts.
const SCRIPT: &str = r#"
local artifact_key = KEYS[1]
local owner_keys = KEYS[2]
local owner_key = KEYS[3]
local artifact_id = ARGV[1]
local artifact = ARGV[2]
local num_holders = tonumber(ARGV[3])
redis.call("SADD", owner_key, artifact_id)
redis.call("SADD", owner_keys, owner_key)
redis.call("HSET", artifact_key, artifact_id, artifact)
-- Store holders in a dedicated Redis set
local holders_key = artifact_key .. ':holders:' .. artifact_id
redis.call("DEL", holders_key)
if num_holders > 0 then
redis.call("SADD", holders_key, unpack(ARGV, 4, 3 + num_holders))
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
|
@ChaoticTempest @jakmeier I've added tests and a couple of fixes. For State Sync to work properly, we need to distinguish owned/not_owned generations and in-use artifacts. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.contains(id).await { | ||
| self.reserved.write().await.generating.remove(&id); | ||
| tracing::error!(id, "cannot create slot: artifact already exists in storage"); | ||
| return None; | ||
| } |
There was a problem hiding this comment.
create_slot() uses self.contains(id).await to check Redis for collisions, but contains() returns false on Redis connection/command errors. That fails open and can allow creating a slot for an ID that already exists, and insert() then does an unconditional HSET, potentially overwriting an existing artifact. Consider performing the HEXISTS check inside create_slot() and treating any Redis error as a hard failure (return None and remove the generating entry), or change contains() to return a Result<bool, StorageError> and propagate it here.
| if self.contains(id).await { | |
| self.reserved.write().await.generating.remove(&id); | |
| tracing::error!(id, "cannot create slot: artifact already exists in storage"); | |
| return None; | |
| } | |
| let Some(mut conn) = self.connect().await else { | |
| self.reserved.write().await.generating.remove(&id); | |
| tracing::error!(id, "cannot create slot: failed to connect to storage"); | |
| return None; | |
| }; | |
| let exists: bool = match conn.hexists(&self.artifacts, id).await { | |
| Ok(exists) => exists, | |
| Err(err) => { | |
| self.reserved.write().await.generating.remove(&id); | |
| tracing::error!(id, ?err, "cannot create slot: failed to check storage"); | |
| return None; | |
| } | |
| }; | |
| if exists { | |
| self.reserved.write().await.generating.remove(&id); | |
| tracing::error!(id, "cannot create slot: artifact already exists in storage"); | |
| return None; | |
| } |
There was a problem hiding this comment.
I would propose to allow each Storage function to fail (return Result) for consistency and robustness. But that deserves a separate PR.
There was a problem hiding this comment.
Specifically:
- len_generated
- contains
- contains_by_owner
- len_by_owner
- is_empty
They return 0 or false by default, which is not great
jakmeier
left a comment
There was a problem hiding this comment.
Looking good! But I guess we still have to wait for other PRs first
| #[test(tokio::test(flavor = "multi_thread"))] | ||
| async fn test_sync_matrix() { |
3b80a36 to
0d78c0d
Compare
Relies on #697, let's merge it first, cc @ChaoticTempest
It would be great to merge and extend #735 before merging this PR
I've replaced
reservedandusedwithgeneratingand `using. It is closer to the design and required by StteSync.