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

Switched to /utxo API #65

Merged
merged 15 commits into from
Jan 5, 2023
Merged

Switched to /utxo API #65

merged 15 commits into from
Jan 5, 2023

Conversation

Duddino
Copy link
Member

@Duddino Duddino commented Nov 29, 2022

Follow up to #62.

  • Edited by JSKitty for LMP (Labs Micro Proposal) context.

What does this PR address?

This PR fixes the previous slow, heavy, over-complex balance synchronisation, which caused the wallet to slow down for users with a large TX history, as well as simply using more RAM, Battery, and Bandwidth, all of which benefits from this PR.

What features or improvements were added?

The classic "UTXO only" sync has returned to MPW, now with fixes that allow both MPW and the Explorers to register stakes and delegations much better than before, the UTXO-only sync is extremely lightweight, as it bears no history, only returning the latest balances held by the wallet.

How does this benefit users?

Massively improved synchronistion speed, lower battery consumption, less bandwidth usage, and less overall RAM/Memory use as well, given MPW now processes far less data to achieve identical results.

JSKitty
JSKitty previously approved these changes Dec 2, 2022
Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

tACK so far, this method syncs much faster than the old (for long-history wallets), with much less bandwidth use, which is great, and I've not noticed any issues in a quick 30m testing session.

@JSKitty JSKitty requested a review from panleone December 2, 2022 02:31
Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

apart from the optimization the code works fine (I couldn't test with rewards since I have never won anything lol)

panleone
panleone previously approved these changes Dec 3, 2022
Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

tACK

Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

I've added a bunch of fixes as there was a lot of leftover code (like non-existent Mempool Statuses or named-variable typos), simplified the UTXO management a bit for Coin Control, and removed redundant net code.

After some testing this generally works well, however after some transactions (stake <---> unstake a few times), the balances end up inconsistent after new blocks, and the app needs a reload to properly calculate balances again, could you take a look at this @Duddino? I believe this is the only issue preventing the merge.

Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

tACK, I've tested on mainnet with over 100+ transactions, stakes, delegations, undelegations, send-to-self, and it seems totally solid.

Nice to have an instant sync now, dropping the heavy one saves us a ton of bandwidth, memory, general battery consumption... win/win! 💪 💜

Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

tACK

Copy link

@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 final testing passes with @JSKitty

Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

re-tACK, fully tested on Testnet and Mainnet with various wallets and comparisons against a master branch deployment.

@JSKitty JSKitty merged commit 2bf4eda into PIVX-Labs:master Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

4 participants