-
Notifications
You must be signed in to change notification settings - Fork 53
fix: Prevent issues with fnames being associated with multiple fids #442
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
4ebd026
6c69ad2
1223212
b756046
fa61636
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,6 +73,7 @@ pub fn get_fname_proof_by_fid(db: &RocksDB, fid: u64) -> Result<Option<UserNameP | |
pub fn put_username_proof_transaction( | ||
txn: &mut RocksDbTransactionBatch, | ||
username_proof: &UserNameProof, | ||
existing_fid: Option<u64>, | ||
) { | ||
let buf = username_proof.encode_to_vec(); | ||
|
||
|
@@ -81,6 +82,12 @@ pub fn put_username_proof_transaction( | |
|
||
let secondary_key = make_fname_username_proof_by_fid_key(username_proof.fid); | ||
txn.put(secondary_key, primary_key); | ||
|
||
// If a username is being transferred, remove the existing secondary key. | ||
if let Some(existing_fid) = existing_fid { | ||
let secondary_key = make_fname_username_proof_by_fid_key(existing_fid); | ||
txn.delete(secondary_key); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, have to consider how to handle existing bad data. This doesn't break consensus since it's just an index change. We might need a migration to fix, but we don't have any migration harness code. Will ticket this, let me know if you're interested in taking that on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feels like it's probably something to deal with outside of this PR |
||
} | ||
} | ||
|
||
#[inline] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, this technically affects consensus. But we might be ok if nodes upgrade at different times since this logic is only hit in the proposer? Other nodes will behave the same regardless of version, as long as the transfer is in the transactions list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it will affect consensus in practice since it only changes where the message is stored tbh.