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

Fixes inaccurate round count in CoinControlDialog (Issue #2068) #2137

Conversation

PastaPastaPasta
Copy link
Member

@PastaPastaPasta PastaPastaPasta commented Jun 19, 2018

The CoinControlDialog previously displayed either the actual round count, or the maxRounds set through the options, whichever is lower (used GetOutputPrivateSendRounds). However, there is no reason why the CoinControlDialog should not always display the actual round count of an input (using GetRealOutputPrivateSendRounds). Since GetOutputPrivateSendRounds is defined as:

    int realPrivateSendRounds = GetRealOutpointPrivateSendRounds(outpoint, 0);
    return realPrivateSendRounds > privateSendClient.nPrivateSendRounds ? privateSendClient.nPrivateSendRounds : realPrivateSendRounds;

This should not break anything, while making it a little bit better and more accurate. Fixes #2068

@UdjinM6 UdjinM6 added this to the 12.4 milestone Jun 20, 2018
@UdjinM6
Copy link

UdjinM6 commented Jun 20, 2018

You are fixing it in the wrong place, should be the one in CoinControlDialog::updateView(). And ironically, it does break the build - there is a second argument there that you missed.

Anyway, I'm not convinced that #2068 is really an issue in the first place, so postponing this till 12.4 for now.

@PastaPastaPasta
Copy link
Member Author

Yup, you're 100% right... That's what I get for not paying enough attention. Just updated that. I remember in my testing before that I was surprised that there wasn't a function where the second arg was assumed to be 0 if nothing was passed.

12.4 is fine, not a major change by any regard, but good in general as it makes it more accurate.

@UdjinM6
Copy link

UdjinM6 commented Jul 12, 2018

Needs rebase after #2149

@PastaPastaPasta PastaPastaPasta force-pushed the inaccurate-coincontrol-rounds-issue-2068 branch from 855cfd9 to 964d153 Compare July 13, 2018 03:00
@PastaPastaPasta PastaPastaPasta force-pushed the inaccurate-coincontrol-rounds-issue-2068 branch from 964d153 to 2209630 Compare July 13, 2018 03:06
@PastaPastaPasta
Copy link
Member Author

I was having difficulties, but was able to figure it out, I think... Please ended up doing git rebase upstream/develop -ff and then resolving the conflicts and then git push -f

Seems to be correct. Is this how I should have done it @UdjinM6 ?

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

I usually rebase like this:

git checkout develop
git pull
git checkout mybranch
git rebase -i develop
git mergetool
<fix conflicts>
git rebase --contunue
git push MyRepo HEAD -f

Anyway, looks good imo.

ACK

@UdjinM6 UdjinM6 merged commit d7e2103 into dashpay:develop Jul 16, 2018
@PastaPastaPasta PastaPastaPasta deleted the inaccurate-coincontrol-rounds-issue-2068 branch July 18, 2018 01:49
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Apr 25, 2019
* fixes dashpay#2068

* Revert prior and update in correct location
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants