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

Add wallet/setAccountHead RPC #4951

Merged
merged 9 commits into from
May 30, 2024
Merged

Add wallet/setAccountHead RPC #4951

merged 9 commits into from
May 30, 2024

Conversation

dguenther
Copy link
Member

@dguenther dguenther commented May 8, 2024

Summary

Implements an RPC for manually updating an account's head. The user can pass in hashes of transactions known to be owned by the account, and the wallet will decrypt and apply only those transactions to the account's state.

Needs Review

  • Should the name of the RPC route be different? Renamed from wallet/addKnownTransactions to wallet/setAccountHead
  • Should this logic live somewhere other than an RPC route, e.g. integrated with the wallet? We might move this out into the Scanner once it's refactored out of the wallet.
  • Should the route have wallet context instead of fullnode? Decided this is fine for now since it's similar to a service command anyway.
  • What should we do if someone cancels an RPC request before updateHead finishes? Added early-exit to the loops by checking request.closed.

Testing Plan

Added automated tests for the RPC.

Manual testing

  1. Run wallet:scanning:off to disable scanning for an account
  2. Run wallet:transactions on the account. Save all transaction hashes.
  3. Run wallet:status. Save the account's head hash as end
  4. Run wallet:rescan to reset the account (Add wallet/resetAccount RPC #4985 will allow you to do this for one account). Cancel the scan
  5. Run wallet:status again. Save the account's head hash as start
  6. Run start on the directory.
  7. In a separate prompt, run repl on the directory. Execute the command with the values you previously saved.
client = await sdk.connectRpc()
(await client.wallet.addKnownTransactions({ account: "account", start: "startHash", end: "endHash", transactions: [{ hash: "ceddfc8c0978db964f8fb3ad66f4d127235dcd25385401e2cd092a7c088c8992" }] })).content

After running wallet:status again, you should see the account restored to its former state.

Documentation

Does this change require any updates to the Iron Fish Docs (ex. the RPC API
Reference
)? If yes, link a
related documentation pull request for the website.

[x] Yes

iron-fish/website#706

Breaking Change

Is this a breaking change? If yes, add notes below on why this is breaking and label it with breaking-change-rpc or breaking-change-sdk.

[ ] Yes

@dguenther dguenther requested a review from a team as a code owner May 8, 2024 20:32
@dguenther dguenther marked this pull request as draft May 8, 2024 20:32
@NullSoldier NullSoldier force-pushed the start-stop-syncing branch from 3c8f732 to 29b54d0 Compare May 22, 2024 21:34
@dguenther dguenther force-pushed the add-known-transactions branch from e532a4f to 540e75a Compare May 23, 2024 02:31
Base automatically changed from start-stop-syncing to staging May 23, 2024 21:31
@dguenther dguenther force-pushed the add-known-transactions branch from 540e75a to 152bf87 Compare May 24, 2024 19:21
@dguenther dguenther changed the title WIP: Add addKnownTransactions RPC Add addKnownTransactions RPC May 24, 2024
@dguenther dguenther marked this pull request as ready for review May 24, 2024 23:02
@dguenther dguenther changed the title Add addKnownTransactions RPC Add wallet/addKnownTransactions RPC May 24, 2024
@NullSoldier
Copy link
Contributor

NullSoldier commented May 29, 2024

This PR looks extremely well written to me. The tests are very easy to verify. My only comment here would be, we should abort eagerly when we can if the request is closed so we don't continue using up CPU if the client disconnects. Especially because we don't have an account lock. It'll prevent the client re-trying the request and causing havoc if they disconnect.

@dguenther
Copy link
Member Author

I added early-exit to both of the loops. Tested it by curling wallet/addKnownTransactions with start at block 1 and an account with head at block 500k, then closed the request after a few seconds. Verified that the account head was no longer rolling back to block 1.

@dguenther dguenther changed the title Add wallet/addKnownTransactions RPC Add wallet/setAccountHead RPC May 30, 2024
@dguenther dguenther merged commit 2c0d518 into staging May 30, 2024
11 checks passed
@dguenther dguenther deleted the add-known-transactions branch May 30, 2024 19:45
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.

2 participants