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

fix: added triples and presigs back to storage and fix EXPIRE on used #92

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

ChaoticTempest
Copy link
Contributor

  • Adds back triple and presignatures to storage if they do not meet the threshold criteria. A new parameter back is added for cases where we are sure we want to add these back to storage. Mainly added back to circumvent the cases of inconsistency when one node decides to drop a triple and presignature but others do not or were not aware of it. In the current code, they would never drop these as they do not own them and will purely sit in storage. It's better to have the chance later to use them otherwise we'd still be at the same problem of taking up maximum amount of triples/presignatures we can have per the config.
  • Updates to redis 7.4.2 in tests for HEXPIRE where EXPIRE was deleting the whole set before, which isn't ideal for when we want to expire per triple id or presignature id. Especially when a new triple or presignature gets used, the time to live on this set would get updated and effectively never get deleted.

Copy link
Contributor

@volovyks volovyks left a comment

Choose a reason for hiding this comment

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

Let's make sure we revert when we have the solution for consistency.

-- Add the triples to the used set and set expiration time. Note, HSET is used so
-- we can expire on each field instead of the whole hash set.
redis.call("HSET", KEYS[3], id1, "1", id2, "1")
redis.call("HEXPIRE", KEYS[3], ARGV[1], "FIELDS", 2, id1, id2)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch

For M1 you may want to pull the following image instead:

```BASH
docker pull ghcr.io/near/sandbox:latest-aarch64
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this still relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, we don't need sandbox at all

Copy link
Contributor

@ppca ppca left a comment

Choose a reason for hiding this comment

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

It seems like if we can check threshold first, then do take_mine(), then we do not need to insert anything back.

if redis.call("SISMEMBER", used_key, triple_id) == 1 then
if back == "true" then
redis.call("HDEL", used_key, triple_id)
elseif redis.call("HEXISTS", used_key, triple_id) == 1 then
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference between HEXISTS and SISMEMBER?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Theyre methods for different types. One is for set and one is for map

@ppca
Copy link
Contributor

ppca commented Jan 9, 2025

And just for my own clarification: our current code does not treat back=true/false differently?

@ChaoticTempest
Copy link
Contributor Author

And just for my own clarification: our current code does not treat back=true/false differently?

before this PR, there was no back at all because we never added back triples/presignatures. The back is so we ignore the if exists in used set check

@ChaoticTempest ChaoticTempest merged commit 5a628ea into develop Jan 9, 2025
2 of 3 checks passed
@ChaoticTempest ChaoticTempest deleted the phuong/fix/inconsistent-drop branch January 9, 2025 00:55
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.

3 participants