Page MenuHomePhabricator
Differential D2614

Remove unused param from txmempool::estimateFee
ClosedPublic
Actions

Authored by jasonbcox on Feb 26 2019, 19:32.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
Commits
rSTAGINGcf9a8488eb62: Remove unused param from txmempool::estimateFee
rABCcf9a8488eb62: Remove unused param from txmempool::estimateFee
Summary

Cleanup to help simplify the coincontrol logic being backported in T521.
Depends on D2613, D2641, D2662
Split out from D2543 for better review

Test Plan

ninja check
test_runner.py rpc_estimatefee

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D2614
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5167
Build 8397: Bitcoin ABC Buildbot (legacy)
Build 8396: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Feb 26 2019, 19:32
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 26 2019, 19:32
Herald added a subscriber: schancel. · View Herald Transcript
teamcity added a subscriber: teamcity.Feb 26 2019, 19:33

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Feb 26 2019, 19:46

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5079: Diff 7490.Feb 26 2019, 19:46
jasonbcox added a child revision: D2615: Deprecate nblocks from estimatefee RPC.Feb 26 2019, 19:48
jasonbcox mentioned this in D2543: Remove unused param from txmempool::estimateFee and estimatefee RPC.Feb 26 2019, 19:50
jasonbcox edited the summary of this revision. (Show Details)
jasonbcox mentioned this in D2602: Cleanup unused confirm target in GetMinimumFee().Feb 26 2019, 20:11
jasonbcox added a child revision: D2602: Cleanup unused confirm target in GetMinimumFee().Feb 27 2019, 06:02
Fabien added inline comments.Feb 28 2019, 07:40
src/qt/sendcoinsdialog.cpp
806 ↗(On Diff #7490)

Even if this is correct, it can look like a bug for the user. If the target number of block selected is changed, the labels keeps displaying that the target is the next block (and the fee amount won't change).
Maybe this label should keep updating for now, and get removed together with this useless block target selection box ?

deadalnix added inline comments.Mar 1 2019, 00:39
src/qt/sendcoinsdialog.cpp
806 ↗(On Diff #7490)

Screenshot would be nice.

Fabien added a comment.Mar 1 2019, 08:04

Here are the screenshots

Before: fees not updating but block number matches the user selection

before.PNG (596×890 px, 67 KB)

After: neither fee nor block number gets updated

after.PNG (596×890 px, 67 KB)

deadalnix requested changes to this revision.Mar 2 2019, 00:43

Look like removing the option from the GUI is the first step here.

This revision now requires changes to proceed.Mar 2 2019, 00:43
jasonbcox mentioned this in D2641: Removed unused confirmation target dropdown from the UI.Mar 4 2019, 18:40
jasonbcox planned changes to this revision.Mar 4 2019, 18:42
jasonbcox edited the summary of this revision. (Show Details)
jasonbcox added a parent revision: D2641: Removed unused confirmation target dropdown from the UI.

Done: D2641
Still need to rebase this diff.

jasonbcox updated this revision to Diff 7574.Mar 4 2019, 18:53

Rebase

teamcity added a comment.Mar 4 2019, 18:54

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Mar 4 2019, 19:07

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5128: Diff 7574.Mar 4 2019, 19:07
Fabien accepted this revision.Mar 4 2019, 20:09
jasonbcox mentioned this in rABC43d3860f4c0b: Removed unused confirmation target dropdown from the UI.Mar 4 2019, 21:37
jasonbcox updated this revision to Diff 7585.Mar 4 2019, 21:39

Rebase

teamcity added a comment.Mar 4 2019, 21:39

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Mar 4 2019, 21:52

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5136: Diff 7585.Mar 4 2019, 21:52
deadalnix added inline comments.Mar 4 2019, 21:59
src/qt/sendcoinsdialog.cpp
749 ↗(On Diff #7585)

Changing this requires to figure out the whole translation business. Maybe we do not want to translate it, but we need to at least update the translations files to reflect that change.

jasonbcox planned changes to this revision.Mar 6 2019, 17:20
jasonbcox added inline comments.
src/qt/sendcoinsdialog.cpp
749 ↗(On Diff #7585)

hm I thought those files got auto generated as part of make but obviously that isn't true since there are none in this diff.

jasonbcox updated this revision to Diff 7623.Mar 6 2019, 18:20

Updated translation strings

teamcity added a comment.Mar 6 2019, 18:21

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

jasonbcox added a comment.Mar 6 2019, 18:31

Going to split out refreshing the translation strings for unrelated changes since it's rather large.

jasonbcox planned changes to this revision.Mar 6 2019, 18:31

Going to split out refreshing the translation strings for unrelated changes since it's rather large.

Harbormaster completed remote builds in B5167: Diff 7623.Mar 6 2019, 18:36
teamcity added a comment.Mar 6 2019, 18:36

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

jasonbcox edited the summary of this revision. (Show Details)Mar 6 2019, 18:37
jasonbcox added a parent revision: D2662: Regenerate out-of-date translation strings.
jasonbcox updated this revision to Diff 7626.Mar 6 2019, 18:38

Rebase on D2662 to cleanly update translation strings

teamcity added a comment.Mar 6 2019, 18:39

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B5170: Diff 7626.Mar 6 2019, 18:53
teamcity added a comment.Mar 6 2019, 18:53

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

deadalnix accepted this revision.Mar 7 2019, 17:18
This revision is now accepted and ready to land.Mar 7 2019, 17:18
Closed by commit rABCcf9a8488eb62: Remove unused param from txmempool::estimateFee (authored by jasonbcox). · Explain WhyMar 7 2019, 18:14
This revision was automatically updated to reflect the committed changes.
jasonbcox mentioned this in rSTAGING43d3860f4c0b: Removed unused confirmation target dropdown from the UI.Sep 18 2019, 22:37
jasonbcox added a commit: rSTAGINGcf9a8488eb62: Remove unused param from txmempool::estimateFee.

Revision Contents
Changeset List

PathSize
src/
qt/
locale/
1282 lines
6 lines
rpc/
7 lines
test/
13 lines
2 lines
2 lines
wallet/
2 lines

Diff 7623

View Options

src/qt/locale/bitcoin_en.ts

Loading...
View Options

src/qt/sendcoinsdialog.cpp

Loading...
View Options

src/rpc/mining.cpp

Loading...
View Options

src/test/policyestimator_tests.cpp

Loading...
View Options

src/txmempool.h

Loading...
View Options

src/txmempool.cpp

Loading...
View Options

src/wallet/fees.cpp

Loading...