Skip to content

[wallet] Refactor to use new MIN_CHANGE #6669

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
merged 3 commits into from
Nov 4, 2015
Merged

[wallet] Refactor to use new MIN_CHANGE #6669

merged 3 commits into from
Nov 4, 2015

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Sep 13, 2015

Introduce a new constant MIN_CHANGE and use it in appropriate places.

@maflcko maflcko force-pushed the MarcoFalke-2015-walletCleanup branch 2 times, most recently from 12ff907 to d266b80 Compare September 18, 2015 13:22
@maflcko
Copy link
Member Author

maflcko commented Sep 18, 2015

I will add some more tests later.

@maflcko maflcko mentioned this pull request Sep 18, 2015
Closed
@maflcko
Copy link
Member Author

maflcko commented Sep 20, 2015

ping @dooglus: As you are one of the authors of the coin selection code and tests, would you mind giving a concept ACK or concept NACK on this pull?

@dooglus
Copy link
Contributor

dooglus commented Sep 20, 2015

Looks good to me. It makes sense to have the minimum change not be hardcoded to 0.01.

@laanwj laanwj added the Wallet label Sep 23, 2015
@btcdrak
Copy link
Contributor

btcdrak commented Sep 30, 2015

utACK

sipa
sipa reviewed Sep 30, 2015
View reviewed changes
src/wallet/wallet.h
//! -maxtxfee default
static const CAmount DEFAULT_TRANSACTION_MAXFEE = 0.1 * COIN;
//! minimum change amount
static const CAmount nMinChange = CENT;
Copy link
Member

Choose a reason for hiding this comment

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

If it's a constant, please use an all-upper-case name to make that clear, like MINIMUM_CHANGE.

If you plan to turn it into a runtime variable later, it's probably better to make it a runtime variable immediately now, and initialize it with a constant, for example DEFAULT_MINIMUM_CHANGE.

Copy link
Member Author

Choose a reason for hiding this comment

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

No plans yet to turn it into a runtime variable.

@maflcko maflcko force-pushed the MarcoFalke-2015-walletCleanup branch from baab750 to c3c98ff Compare October 9, 2015 08:23
@maflcko
Copy link
Member Author

maflcko commented Oct 9, 2015

Addressed nit by @sipa in follow up commit c3c98ffcb6c27fb836754d54954d2f273928c164.

@maflcko maflcko force-pushed the MarcoFalke-2015-walletCleanup branch from c3c98ff to bb3ed07 Compare October 24, 2015 21:30
@maflcko maflcko changed the title [wallet] Refactor to use new nMinChange [wallet] Refactor to use new MIN_CHANGE Oct 24, 2015
@wtogami
Copy link
Contributor

wtogami commented Oct 27, 2015

Add mention of the addition of DEFAULT_TRANSACTION_MINFEE to the commit message?

@maflcko
Copy link
Member Author

maflcko commented Oct 27, 2015

@wtogami I think you intended to put the comment over at #6696?

@maflcko
Copy link
Member Author

maflcko commented Oct 27, 2015

Anything holding this back? Does this need more review or additional test cases?

@wtogami
Copy link
Contributor

wtogami commented Oct 27, 2015

@MarcoFalke oh? it appeared that the commits in this PR added it.

@sipa
Copy link
Member

sipa commented Oct 28, 2015

Concept & light code review ACK.

[wallet] Refactor to use new MIN_CHANGE
6b0e622 
* 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
@maflcko maflcko force-pushed the MarcoFalke-2015-walletCleanup branch from bb3ed07 to 5190276 Compare October 28, 2015 09:55
@maflcko maflcko force-pushed the MarcoFalke-2015-walletCleanup branch from 5190276 to a9c73a1 Compare October 28, 2015 10:16
@maflcko
Copy link
Member Author

maflcko commented Oct 28, 2015

@wtogami Done. (Fixed commit msg)

@laanwj
Copy link
Member

laanwj commented Oct 29, 2015

Concept ACK. After this we could completely get rid of CENT.

laanwj
laanwj reviewed Oct 29, 2015
View reviewed changes
src/wallet/wallet.h
@@ -41,8 +41,12 @@ extern bool fPayAtLeastCustomFee;
static const CAmount DEFAULT_TRANSACTION_FEE = 0;
//! -paytxfee will warn if called with a higher fee than this amount (in satoshis) per KB
static const CAmount nHighTransactionFeeWarning = 0.01 * COIN;
//! -mintxfee default
static const CAmount DEFAULT_TRANSACTION_MINFEE = 1000;
Copy link
Member

Choose a reason for hiding this comment

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

Should use this new default constant in init.cpp as well (e.g. HelpMessage)

Copy link
Member Author

Choose a reason for hiding this comment

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

@laanwj Done

@dcousens
Copy link
Contributor

utACK

@laanwj laanwj merged commit 6342a48 into bitcoin:master Nov 4, 2015
laanwj added a commit that referenced this pull request Nov 4, 2015
@laanwj
Merge pull request #6669
8a95a18 
6342a48 Init: Use DEFAULT_TRANSACTION_MINFEE in help message (MarcoFalke)
a9c73a1 [wallet] Add comments for doxygen (MarcoFalke)
6b0e622 [wallet] Refactor to use new MIN_CHANGE (MarcoFalke)
@maflcko maflcko deleted the MarcoFalke-2015-walletCleanup branch November 4, 2015 11:31
@str4d str4d mentioned this pull request Mar 29, 2017
Draft
@str4d str4d mentioned this pull request Dec 17, 2019
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Dec 18, 2019
@zkbot
Auto merge of #4268 - str4d:2074-cleanup-2, r=str4d
577f7ef 
Bitcoin 0.12 cleanup PRs 2

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6631
- bitcoin/bitcoin#6664
  - Only the first commit (we already had the second through bitcoin/bitcoin#6825).
- bitcoin/bitcoin#6669
- bitcoin/bitcoin#6887
  - Only the non-QT parts.
- bitcoin/bitcoin#6962
- bitcoin/bitcoin#6822
  - Only first and third commits (we already had the second through an earlier PR).
- bitcoin/bitcoin#7136
  - Excludes Travis CI changes, and fixes to documents we don't have anymore.
- bitcoin/bitcoin#7084
- bitcoin/bitcoin#7509
- bitcoin/bitcoin#7617
- bitcoin/bitcoin#7726

Part of #2074.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@maflcko @dooglus @btcdrak @wtogami @sipa @laanwj @dcousens