fix: add account closure tracking to prevent mainnet refetch (#402)#409
Conversation
|
@MicaiahReid hi ser, can you review this pr ? thank you |
MicaiahReid
left a comment
MicaiahReid
left a comment
There was a problem hiding this comment.
Hi @AvhiMaz, thanks for the PR!
Sadly I don't think this PR is really hitting the crux of the issue.
Your PR tracks "closed accounts" and adds the close_account fn to update this index of closed accounts.
However, the close_account method is never actually called. Closing an account isn't meant to be something a use has to manually do - by closed accounts I'm referring to one that is automatically garbage collected by the VM. When an account sends a transaction such that it no longer has any lamports (i.e. transfers all of its funds away), it will automatically be closed.
So, when a transaction is closed in this way, we need to be sure that it isn't fetched from remote again.
I haven't been able to implement/test this, but I think the way to do this is to update our update_account_registries function to push to the closed_accounts list if the accound being passed to the function is empty (aka Account::default). This empty account will be garbage collected, so it's closed, but since we know it's been fetched before we know we don't want to fetch it again.
You're welcome to try this, or I can take over if you'd like.
Let me know!
|
hi @MicaiahReid, i tried to implement what you said. if you think i'm on the right path, let me know, otherwise you can take it from here. thank you. |
MicaiahReid
force-pushed
the
fix/account-closure-tracking
branch
from
43bcb3a to
b146654
Compare
MicaiahReid
left a comment
MicaiahReid
left a comment
There was a problem hiding this comment.
I think this is looking good! I've added a quick test to this PR :)
And a few small comments
crates/core/src/surfnet/locker.rs
Outdated
| /// | ||
| /// Once marked as closed, this account will not be fetched from mainnet even if it doesn't exist | ||
| /// in the local cache. This prevents closed accounts from being inadvertently restored. | ||
| pub fn close_account(&self, pubkey: Pubkey) -> SurfpoolResult<()> { |
There was a problem hiding this comment.
I think we can remove this method, it's not used currently, and I don't think we'll need to
crates/core/src/surfnet/locker.rs
Outdated
|
|
||
| // Collect missing pubkeys (local_results is already in correct order from pubkeys) | ||
| // Get the closed accounts set | ||
| let closed_accounts = self.with_svm_reader(|svm_reader| svm_reader.closed_accounts.clone()); |
There was a problem hiding this comment.
I think you can just used your self. get_closed_accounts() helper here instead of with_svm_reader
crates/core/src/surfnet/locker.rs
Outdated
| Ok(result.with_new_value(remote_account)) | ||
| // Check if the account has been explicitly closed - if so, don't fetch from remote | ||
| let is_closed = | ||
| self.with_svm_reader(|svm_reader| svm_reader.closed_accounts.contains(pubkey)); |
There was a problem hiding this comment.
I think you can just used your self. get_closed_accounts() helper here instead of with_svm_reader
MicaiahReid
left a comment
MicaiahReid
left a comment
There was a problem hiding this comment.
Awesome work, thanks @AvhiMaz!!
you can run the ci/cd. if there are any errors, i’m ready to fix them 🫡 |
AvhiMaz
force-pushed
the
fix/account-closure-tracking
branch
from
91b98c4 to
e57b2c1
Compare
|
@MicaiahReid there was a formatting issue. i rebased with the main branch and pushed it. hopefully the issue is resolved now. |
|
thanks @AvhiMaz ! do you think you could refresh your branch with latest |
…foundation#402) Distinguish between user-initiated closure and reset/stream operations. - Add closed_accounts HashSet to SurfnetSvm - Check closed accounts before mainnet fetch in retrieval logic - Add close_account/unclose_account methods - Update reset_account to unclose before reset Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
AvhiMaz
force-pushed
the
fix/account-closure-tracking
branch
from
e57b2c1 to
6899317
Compare
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
|
hi @lgalabru
ig its ready to merge, thanks! |
3 participants
Distinguish between user-initiated closure and reset/stream operations.
Fixes #402