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.

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

[GUI] Automatically set the lowest possible Custom Fee when user provided fee is too low #2215

Merged
merged 1 commit into from
Apr 9, 2021
Merged

[GUI] Automatically set the lowest possible Custom Fee when user provided fee is too low #2215

merged 1 commit into from
Apr 9, 2021

Conversation

MishaPozdnikin
Copy link

@MishaPozdnikin MishaPozdnikin commented Feb 25, 2021 •
edited
Loading

What does it do?

Automatically set the lowest possible Custom Fee when user type in fee that is too low.
More details: #1836

Demonstration:

LowestFeeDemonstration

random-zebra
random-zebra approved these changes Mar 20, 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.

Thanks for the contribution! LGTM. utACK 60bc468

@random-zebra random-zebra added GUI Feature Request Needs Backport Placeholder tag for anything needing a backport to prior version branches labels Mar 20, 2021
@random-zebra random-zebra added this to the 5.1.0 milestone Mar 20, 2021
furszy
furszy reviewed Mar 20, 2021
View reviewed changes
Copy link

@furszy furszy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code review ACK 60bc468.

Same as was what zebra said, welcome :). Left a minor comment that can be easily tackled.

src/qt/pivx/sendcustomfeedialog.cpp
@@ -126,6 +126,9 @@ void SendCustomFeeDialog::accept()
inform(tr("Fee too high. Must be below: %1").arg(
BitcoinUnits::formatWithUnit(walletModel->getOptionsModel()->getDisplayUnit(), insaneFee)));
} else if (customFee < CWallet::GetRequiredFee(1000)) {
CAmount nFee;
walletModel->getWalletCustomFee(nFee);
Copy link

Choose a reason for hiding this comment

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

Would be good to check for the getWalletCustomFee return boolean value or use the hasWalletCustomFee method before getting the wallet custom fee, so in case of having the wallet custom fee disabled (CWallet::fUseCustomFee), the GUI use the minimum required fee and not the stored value.

In other words, something like:

CAmount nFee = 0;
if (walletModel->hasWalletCustomFee()) {
     walletModel->getWalletCustomFee(nFee);
} else {
     nFee = CWallet::GetRequiredFee(1000);
}

Side topic, a future good work on this area would be to encapsulate every CWallet related function call into the wallet model. None of the widgets should have access to the backend related files/classes, only connect to the models.

Copy link

Choose a reason for hiding this comment

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

Maybe we can merge this PR, and fix this thing in a quick follow-up PR?

As for the side topic... check #2293 :)

Copy link

Choose a reason for hiding this comment

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

Cool for #2293 👌 .
Ok, np. let's merge this one and will do a quick follow-up PR to fix the problem.

Guess that @MishaPozdnikin isn't around anymore.

@Fuzzbawls Fuzzbawls changed the title Qt: Automatically set the lowest possible Custom Fee when user type in fee that is too low [GUI] Automatically set the lowest possible Custom Fee when user provided fee is too low Mar 22, 2021
Fuzzbawls
Fuzzbawls approved these changes Mar 24, 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 60bc468

@furszy furszy mentioned this pull request Apr 8, 2021
Merged
4 tasks
@random-zebra random-zebra requested a review from furszy April 9, 2021 10:18
@furszy furszy merged commit a9e9169 into PIVX-Project:master Apr 9, 2021
@furszy furszy mentioned this pull request Apr 9, 2021
Merged
furszy referenced this pull request in furszy/bitcoin-core Apr 9, 2021
@MishaPozdnikin @furszy
Automatically set the lowest possible Custom Fee when user type in fe…
1fc145c 
…e that is too low.

Github-Pull: bitcoin#2215
Rebased-From: 60bc468
random-zebra added a commit that referenced this pull request Apr 10, 2021
@random-zebra
Merge #2306: [GUI] if the custom fee is disabled, use minimum require…
3908535 
…d fee

0e182e3 GUI: if the custom fee is disabled, use the minimum required fee and not the stored value. (furszy)

Pull request description:

  Implementing the comment #2215 (comment)

ACKs for top commit:
  Fuzzbawls:
    ACK 0e182e3
  random-zebra:
    utACK 0e182e3 and merging...

Tree-SHA512: b25c118d6068a56f063879a4efbf349ee613e275fecf3b82df520a30cc21c3612819632511c7c668e8d8bcb1293937d820caf631caa9028e68d5eb2f0ef9cc57
Fuzzbawls added a commit that referenced this pull request Apr 12, 2021
@Fuzzbawls
Merge #2303: [Backport] 5.1.0 backports
a0f768a 
50a5e84 GUI: if the custom fee is disabled, use the minimum required fee and not the stored value. (furszy)
1fc145c Automatically set the lowest possible Custom Fee when user type in fee that is too low. (MishaPozdnikin)
0d2555e net_processing: missing cs_main lock for chainActive.GetLocator() call (furszy)
64bd400 validation: Remove redundant request sync from ProcessNewBlock(). (furszy)
c44b431 validation: missing cs_main lock for `CheckBlock` in `ProcessNewBlock` (furszy)
1f38b90 Removal of the `fAlreadyChecked` flag from the entire `ActivateBestChain` flow. (furszy)
f53c824 Refactor: remove redundant `fAlreadyCheckedBlock` argument from `AcceptBlock` (furszy)
374f6c9 Refactor: move `CheckBlockSignature` function call inside `CheckBlock`. (furszy)
a78dfbe Validation: Remove `CheckBlockSignature` now unneeded enableP2PKH flag. (furszy)
7f244b1 [Tests] Check last_processed_block in getwalletinfo Github-Pull: #2283 Rebased-From: a700fdf (random-zebra)
b3a12b5 [RPC] `getwalletinfo`: Add last_processed_block return value. (furszy)
e51dcee Add unit tests for signals generated by ProcessNewBlock() (furszy)
c4dd07f Validation: rename one of the two instances using "bad-prevblk" to its correct description of "prevblk-not-found" (furszy)
39f6eaf Fix concurrency-related bugs in ActivateBestChain (Jesse Cohen)
fb19c3a Do not unlock cs_main in ABC unless we've actually made progress. (Matt Corallo)
fd79bb7 Optimize ActivateBestChain for long chains (Pieter Wuille)
c2ae5ff Fix fast-shutdown crash if genesis block was not loaded (Matt Corallo)
7ab7112 Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip (Jesse Cohen)
c202bc1 Update ValidationInterface() documentation to explicitly specify threading and memory model (Jesse Cohen)
f499e6e Update documentation for SingleThreadedSchedulerClient() to specify the memory model (Jesse Cohen)
9a9425f Add Unit Test for SingleThreadedSchedulerClient (Jesse Cohen)
4903f31 Removing blocksizenotify abomination. (furszy)
c865148 [validation] Do not actively wait for cs_main lock in `ActivateBestChain()` (furszy)
a582560 docs: add reduce-memory.md (fanquake)
f806584 test: move sync_blocks and sync_mempool functions to test_framework.py (random-zebra)
d902128 [Test] Fix intermittent sync_blocks failures (random-zebra)
a1d6c0c Move g_is_mempool_loaded into CTxMemPool::m_is_loaded (Ben Woosley)
584bbba rpc: Expose g_is_mempool_loaded via getmempoolinfo and /rest/mempool/info.json (Ben Woosley)
e10c1b6 Set fee to highest possible if input is too big #fixes 2234 (dnchk)
775e532 Fixes double fade-in animation when clicking the question mark next to the 'Available' label in the top bar (Volodia)
0cf4064 Make box of PIVX address return to purple when it's empty (Volodia)

Pull request description:

  Backport the following PRs to the 5.1 branch:

  * #2237
  * #2247
  * #2249
  * #2254
  * #2262
  * #2284
  * #2290

  Missing open PRs that need to get merged to move forward with v5.1.0:

  * [x] #2215
  * [x] #2283
  * [x] #2295
  * [x] #2306

  When every PR get merged, we can move forward with a new release candidate for the v5.1.0 release (rc3). Then one or two weeks of testing (depending on how many testers join the efforts) and we are ready for the production release.

  The heart of v5.1.0 has been already battle tested this past month with the v5.1.0rc2 testing phase. The only remaining point that needs a more broadly usage/testing is #2290 that it's solving a reported issue in the recent release candidate.

ACKs for top commit:
  random-zebra:
    utACK 50a5e84
  Fuzzbawls:
    ACK 50a5e84

Tree-SHA512: 1f77e1601c51f280b3de0d1de9e0b084871701c1af35ad817760ad55ec28c02e2423494b3d189273aacd367b41ba826d63beb73b0259a1c17f019d05711d54ea
@Fuzzbawls Fuzzbawls removed the Needs Backport Placeholder tag for anything needing a backport to prior version branches label Apr 12, 2021
@furszy furszy mentioned this pull request Jul 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@Fuzzbawls Fuzzbawls Fuzzbawls approved these changes

@furszy furszy Awaiting requested review from furszy

Assignees

@MishaPozdnikin MishaPozdnikin

Labels
Feature Request GUI
Projects
None yet
Milestone
5.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@MishaPozdnikin @furszy @Fuzzbawls @random-zebra