Skip to content

[Wallet] Prefer coins that have fewer ancestors, sanity check txn before ATMP and rebroadcast functional test #2601

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
Merged
furszy merged 7 commits into from
Nov 22, 2021
Merged

[Wallet] Prefer coins that have fewer ancestors, sanity check txn before ATMP and rebroadcast functional test #2601

furszy merged 7 commits into from
Nov 22, 2021

Conversation

@furszy
Copy link

@furszy furszy commented Oct 14, 2021

Work originated from a third party service hitting the mempool ancestors count limit.

Focused on two main topics (plus some extra cleanup):

  1. The wallet's utxo selection process will now predilect coins that have fewer ancestors.
  2. Add functional test for the wallet transactions rebroadcast.

Based on the following backports:

Important Note:

This PR is not meant to be included in the shortly coming v5.3.2.1. Which is good enough as is, the sendmany min depth arg fix is already by-passing the issue automatically without human interaction (mempool utxo will not be selected by default).
This is a follow up work to fully prevent the issue from happening in other flows and not only in sendmany.
Merge target is v5.4.

@furszy furszy self-assigned this Oct 14, 2021
@furszy furszy added this to the 5.4.0 milestone Oct 14, 2021
@random-zebra
Copy link

random-zebra commented Nov 11, 2021

I think that the introduction of -walletrejectlongchains argument is redundant, due to our current behavior (which diverges from upstream's).
The wallet should always reject long chains, as any transaction that fails to commit to a node's mempool would be abandoned by the node's wallet (#1695).
Due to this, the last extra run of SelectCoinsMinConf is also redundant.

@furszy furszy force-pushed the 2021_wallet_ancestors_selection branch 2 times, most recently from 4f9e6d2 to bf74449 Compare November 12, 2021 14:14
@furszy
Copy link
Author

furszy commented Nov 12, 2021 •
edited
Loading

good catch zebra ☕, true that is redundant. Squashed changes in 503ce57 and 0c2b921

@furszy furszy force-pushed the 2021_wallet_ancestors_selection branch 2 times, most recently from 29288f8 to 92db52d Compare November 16, 2021 13:27
@furszy
[wallet] Refactor to use new MIN_CHANGE
117ec84 
* Introduce new constant MIN_CHANGE and use it instead of the
hardcoded "CENT"
* Add test case for MIN_CHANGE
* Introduce new constant for -mintxfee default:
 DEFAULT_TRANSACTION_MINFEE = 1000
@furszy furszy force-pushed the 2021_wallet_ancestors_selection branch from 92db52d to 26dcd16 Compare November 19, 2021 13:17
@furszy
Copy link
Author

furszy commented Nov 19, 2021

rebased on master.

Fuzzbawls
Fuzzbawls previously approved these changes Nov 22, 2021
View reviewed changes
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 26dcd16c7b353b9921a427f3178392be30d1575c

random-zebra
random-zebra reviewed Nov 22, 2021
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Need to add the new test wallet_resendwallettransactions.py to the test runner file (in the "less than 60s" list).

@furszy furszy force-pushed the 2021_wallet_ancestors_selection branch from 26dcd16 to 9deb391 Compare November 22, 2021 12:42
@furszy
Copy link
Author

furszy commented Nov 22, 2021

yes good, done, new test added to the test runner. Squashed in 5cd487e

random-zebra
random-zebra approved these changes Nov 22, 2021
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

ACK 9deb391

@furszy furszy merged commit 162d76a into PIVX-Project:master Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fuzzbawls Fuzzbawls Fuzzbawls left review comments

+1 more reviewer

@random-zebra random-zebra random-zebra approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@furszy furszy

Labels

Upstream Wallet

Projects

None yet

Milestone

5.4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@furszy @random-zebra @Fuzzbawls @instagibbs