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

Set() to return a value indicating whether the key already existed #75

Closed
anorth opened this issue Nov 23, 2020 · 2 comments · Fixed by #77
Closed

Set() to return a value indicating whether the key already existed #75

anorth opened this issue Nov 23, 2020 · 2 comments · Fixed by #77
Assignees
Labels
kind/enhancement A net-new feature or an improvement to an existing feature need/triage Needs initial labeling and prioritization

Comments

@anorth
Copy link
Member

anorth commented Nov 23, 2020

The Set method is implemented as blind upsert and does not return any indication of what value, if any, previously existed for the key in question. Callers that wish to leave any existing value intact, or save or report it elsewhere after it is overwritten, must first perform a Get or Has in order to establish if one exists. Set could instead return an indication of whether the key previously existed.

Some options:

  • Set returns a boolean if the key existed
  • Set accepts a nillable out parameter like Get, which receives any previous value (combined with the boolean)
  • Set or a variant of it accepts a callback receiving the bytes previously stored for the key
  • [Edit] A new SetIfAbsent writes a key only if not previously present, and returns a boolean indicating whether it did so

The concrete motivating case is safety checks in the Miner actor. It contains a HAMT of pre-committed sectors by sector number. The PreCommitSector method wants to abort if a pre-commit collides with a sector number already pre-committed or proven. If it does, the method will abort and roll back all state changes. This method is expensive and we wish to remove all unnecessary state access. The usual case is no collision.

A single boolean return value is sufficient here: even if the HAMT is internally modified by the Set, the modification will be thrown away. The more complicated options just provide more flexibility for future use cases.

As it happens, the Miner actor has a separate bitfield of all allocated sector numbers so the HAMT check is redundant and will probably be removed (in filecoin-project/specs-actors#1241). But it would be more robust to keep it, if it were free in terms of state blocks accessed.

@ZenGround0 @Stebalien

@anorth anorth added kind/enhancement A net-new feature or an improvement to an existing feature need/triage Needs initial labeling and prioritization labels Nov 23, 2020
@Stebalien
Copy link
Member

This is also an issue when creating new deals and recording the proposal CID. Unfortunately, that's a check we can't just remove.

@anorth
Copy link
Member Author

anorth commented Nov 24, 2020

Added the new option of a SetIfAbsent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or an improvement to an existing feature need/triage Needs initial labeling and prioritization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants