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

[RPC] Fix Invalidateblock #2856

Merged
merged 3 commits into from
Oct 3, 2023
Merged

Conversation

panleone
Copy link

@panleone panleone commented Apr 25, 2023

Invalidateblock has not been working since sapling came out. This happens because the current maximum witness cache has a depth of only 100 blocks. With this PR I want to fix this issue by manually building the witness cache if the block has a depth bigger than the cache size.
Since this operation can become pretty lengthy, the RPC will just return error if your oldest sapling note is older than 1 month (43200 blocks behind the chain tip).

Note: After using invalidateblock the user might need to do
-> debug ->wallet repair -> recover transactions in order to recover the correct balance. This is not a limit of this PR and you can test that it already happens with the current implementation of invalidateblock.
Anyways "recover transactions" also cleans the wallet GUI from "conflicted" txs that you get after invalidating a block so it would be useful regardless.

Overall invalidating + eventually using recover transactions is faster than the current rescanning the blockchain from 0
Solves #2605

@panleone panleone self-assigned this Apr 25, 2023
@panleone panleone added this to the 6.0.0 milestone Apr 25, 2023
@panleone panleone force-pushed the invalidateblock branch 9 times, most recently from caf0f85 to 859ae93 Compare April 26, 2023 14:12
Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

I think it would be better if you put the max sapling note age in the RPC, because for wallets with sapling notes older than 1 month, the alternative is to rescan everything from scratch, which is going to take longer anyways

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/sapling/saplingscriptpubkeyman.cpp Outdated Show resolved Hide resolved
src/sapling/saplingscriptpubkeyman.cpp Outdated Show resolved Hide resolved
src/sapling/saplingscriptpubkeyman.cpp Outdated Show resolved Hide resolved
@panleone
Copy link
Author

thanks for the review Duddino! I rebased with your suggestion...

I think it would be better if you put the max sapling note age in the RPC, because for wallets with sapling notes older than 1 month, the alternative is to rescan everything from scratch, which is going to take longer anyways

I don't totally agree on this, from PR #2820 users will also be able to rollback to the last checkpoint which is quite fast. So I'd keep the limit of 1 month or in case extend it to 2 months

@panleone
Copy link
Author

Update: found an optimization that makes the rpc call much faster so I decided to remove the upper bound limit of 1 month.

On my regtest it took 15 seconds to invalidate a block and in the wallet I had 15 sapling notes each with a depth of around 70k blocks.

@panleone
Copy link
Author

panleone commented Jun 3, 2023

rebased on master

Copy link
Member

@Liquid369 Liquid369 left a comment

Choose a reason for hiding this comment

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

tACK be49720

@Fuzzbawls Fuzzbawls changed the title Fix Invalidateblock [RPC] Fix Invalidateblock Oct 3, 2023
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK be49720

@Fuzzbawls Fuzzbawls merged commit f02ab52 into PIVX-Project:master Oct 3, 2023
@Fuzzbawls Fuzzbawls modified the milestones: 6.0.0, 5.6.0 Feb 6, 2024
@panleone panleone deleted the invalidateblock branch March 23, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants