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

Mempool refactor #291

Merged
merged 36 commits into from
Mar 17, 2024
Merged

Mempool refactor #291

merged 36 commits into from
Mar 17, 2024

Conversation

Duddino
Copy link
Member

@Duddino Duddino commented Jan 22, 2024

Abstract

This PR aims to better define the role of the Mempool class, and its relationship to other parts of the code. It's worthwhile to note that the name 'Mempool' has been a misnomer for quite a while; I've decided to interpret its role as a Transaction Manager, implementing the following functionalities:

  • Storing and managing transactions
  • Outpoint management, including the ability to tell which one are unspent, locked and their type (pk2ph/p2cs)
  • UTXO retrival
  • Calculating and caching balance

The circular dependency with the Wallet class was removed. Now the Wallet class holds a private instance of the Mempool, and all public references to the global mempool object have been removed.

Why not merge this with the Wallet class?

I believe the Wallet class already has a lot of complexity, and it may be more beneficial to abstract some behavior to other classes, rather than having one big class.

Other minor improvements:

  • Cache management of the different balances has been simplified, by mapping filter->balance.
  • Added unit tests for the new mempool class
  • The getUTXOs method was changed. It now takes a filter, which should usually be left as default = OutpointState.SPENT. filter removes any UTXO that matches the OutpointState.
    A new parameter has been added, requirement. This does the opposite of filter, and only returns UTXOs that match the given state. For example getUTXOs({requirement: OutpointState.P2PKH}) would only return regular P2PKH UTXOs

TODO

  • Fix masternodes
  • Improve mempool unit tests
  • Readd tx db
  • Change mempool name?

Testing

  • Test that sending/receiving transaction works as expected
  • Test that txs get locked as expected (Right now the only locked txs are masternode ones)
  • Test that balances are correct

Copy link

netlify bot commented Jan 22, 2024

Deploy Preview for cheery-moxie-4f1121 ready!

Name Link
🔨 Latest commit bfde392
🔍 Latest deploy log https://app.netlify.com/sites/cheery-moxie-4f1121/deploys/65f0d5904c6a7b00087ca990
😎 Deploy Preview https://deploy-preview-291--cheery-moxie-4f1121.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Duddino Duddino self-assigned this Jan 22, 2024
@Duddino Duddino added Refactor A PR or suggestion for rewriting existing code. Test Addition of unit or functional test labels Jan 22, 2024
@Duddino Duddino marked this pull request as ready for review February 27, 2024 13:57
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.

overall the PR is very good, I'm leaving a partial review about algorithms and data structure used.
Most of them can be done as future PRs since the old system did the same and this is a refactor PR

scripts/dashboard/Activity.vue Show resolved Hide resolved
scripts/mempool.js Show resolved Hide resolved
scripts/mempool.js Outdated Show resolved Hide resolved
scripts/mempool.js Show resolved Hide resolved
scripts/wallet.js Outdated Show resolved Hide resolved
scripts/network.js Outdated Show resolved Hide resolved
scripts/wallet.js Show resolved Hide resolved
scripts/__mocks__/mempool.js Show resolved Hide resolved
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.

nits/triviliaties

scripts/dashboard/Dashboard.vue Outdated Show resolved Hide resolved
scripts/wallet.js Outdated Show resolved Hide resolved
scripts/wallet.js Outdated Show resolved Hide resolved
@JSKitty JSKitty added Awaiting Review This PR and/or issue is awaiting reviews before continuing. Review Reward: 20 PIV Reviewers of this Pull Request will receive a 20 PIV reward labels Mar 7, 2024
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 bfde392

Looking stable in every way I can test: HD and non-HD wallets, Staking and Unstaking, Shielding and Deshielding, synchronising, all appear to be perfectly stable and reliable.

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 and merging

@panleone panleone merged commit 8fe783c into PIVX-Labs:master Mar 17, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Review This PR and/or issue is awaiting reviews before continuing. Refactor A PR or suggestion for rewriting existing code. Review Reward: 20 PIV Reviewers of this Pull Request will receive a 20 PIV reward Test Addition of unit or functional test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants