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.

Sign up for GitHub

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

Jump to bottom

Add height to importpubkey for rescan #3102

Merged
merged 1 commit into from
Oct 26, 2022
Merged

Add height to importpubkey for rescan #3102

merged 1 commit into from
Oct 26, 2022

Conversation

chromatic
Copy link
Member

No description provided.

@patricklodder patricklodder requested a review from a team August 22, 2022 18:00
@patricklodder patricklodder added this to the 1.14.7 milestone Aug 22, 2022
@chromatic chromatic force-pushed the add-height-to-importpubkey-rescan branch 2 times, most recently from 015ce1c to ba5379e Compare October 2, 2022 22:25
@chromatic chromatic marked this pull request as draft October 3, 2022 05:30
@chromatic
Copy link
Member Author

I'm moving this to draft for now, because I want to squash a bunch of commits before merge. When this design meets the approval of others, I'll squash and mark ready for final review.

@patricklodder
Copy link
Member

When this design meets the approval of others, I'll squash and mark ready for final review.

I like this design 👍

@chromatic chromatic force-pushed the add-height-to-importpubkey-rescan branch from 1102310 to f3dbf6f Compare October 8, 2022 18:28
@chromatic chromatic marked this pull request as ready for review October 8, 2022 18:28
@chromatic chromatic requested a review from a team October 8, 2022 18:29
@chromatic chromatic force-pushed the add-height-to-importpubkey-rescan branch from f3dbf6f to 20727f1 Compare October 8, 2022 18:29
@xanimo
Copy link
Member

xanimo commented Oct 9, 2022

I understand that I should attempt to import a pubkey for rescan but would be your best suggestion for evaluation/testing of this enhancement @chromatic? If this does what I think it does, this is a very cool feature to have! ❤️

@chromatic
Copy link
Member Author

I should add some automated tests for this. I think the best way to test this manually is to:

  • find an address that's received a transaction
  • import the public key, asking for a rescan from a height after the transaction's block height
  • confirm that the transaction is not visible
  • rescan from a height before the transaction's block height
  • confirm that the transaction is visible
  • find a second address that's received a transaction
  • import the second public key, asking for a rescan from a height before the second transaction's block height
  • confirm that the second public key's transaction is visible

@chromatic chromatic force-pushed the add-height-to-importpubkey-rescan branch 2 times, most recently from 4aa3ada to 2e7ba9e Compare October 16, 2022 21:15
@chromatic
Copy link
Member Author

Rebased and finished off the tests. I'm happy to squash the tests into the implementation commit or leave them separate; no preference.

@xanimo
Copy link
Member

xanimo commented Oct 18, 2022

Built and tested on x86_64-pc-linux-gnu (Ubuntu Jammy). Still reading through debug.logs for each node and it looks like it's doing what you've described above and in the comments found in import-rescan.py. I'll keep you posted if I find anything abnormal but so far this all looks good to me. 👍

patricklodder
patricklodder reviewed Oct 18, 2022
View reviewed changes
src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved
qa/rpc-tests/import-rescan.py Outdated Show resolved Hide resolved
@chromatic chromatic force-pushed the add-height-to-importpubkey-rescan branch 2 times, most recently from 50c9baf to 563370d Compare October 21, 2022 23:51
patricklodder
patricklodder reviewed Oct 22, 2022
View reviewed changes
Copy link
Member

@patricklodder patricklodder left a comment

Choose a reason for hiding this comment

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

Code review ACK
Tested ACK - on Ubuntu 22.04

Needs a squash.

@chromatic
Add height to importpubkey for rescan
7a00fb1 
This adds two helper functions. One function gets a height parameter from the
incoming RPC request. The other performs the scanning. We can use both
functions for reducing code in other RPC calls that can/should take height
parameters and perform rescanning.
@chromatic chromatic force-pushed the add-height-to-importpubkey-rescan branch from 563370d to 7a00fb1 Compare October 23, 2022 00:07
@chromatic
Copy link
Member Author

Squashed and rebased against 1.14.7-dev.

patricklodder
patricklodder approved these changes Oct 23, 2022
View reviewed changes
Copy link
Member

@patricklodder patricklodder left a comment

Choose a reason for hiding this comment

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

reACK 563370d..7a00fb1 (clean squash)

@patricklodder patricklodder merged commit f01e36c into dogecoin:1.14.7-dev Oct 26, 2022
@chromatic chromatic deleted the add-height-to-importpubkey-rescan branch April 29, 2023 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patricklodder patricklodder patricklodder approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
1.14.7
Development

Successfully merging this pull request may close these issues.
3 participants
@chromatic @patricklodder @xanimo