[v0.16.x] More UI + PrivateSend/MN-Sync improvements #3705
Merged
Conversation
* qt: Replaced checkbox images with new ones * qt: Replaced radiobutton images with new ones * qt: Redefine arrow usage - Removed "hover" arrows - Rename normal -> light - Rename pressed -> dark - Use light as hover in dark theme and vice versa * qt: Update icon set * qt: Update sync spinner * qt: Add new ThemedColor's - ThemedColor::ORANGE - ThemedColor::ICON_ALTERNATIVE_COLOR * qt: Add GUIUtil::getIcon and GUIUtil::setIcon * qt: Use themed icons where required * qt: Make sure icons in SendCoinsEntry are updated on style changes * qt: Make sure status bar icons are updated on style changes * qt: Make sure icons in RPCConsole are updated on style changes * qt: Remove icon/iconsSize in modaloverlaydialog.ui * qt: Remove obsolete qproperty-iconSize entries from general.css * qt: Use QToolButton's in SendVerifyMessageDialog and RPCConsole Instead of QPushButton. This is to have the same alignment of the iconized buttons by default like in SendCoinsEntry * qt: Revert arrow icons * qt: Revert HD icons
…CollateralAmounts (dashpay#3657) * wallet: Add m_dust_feerate to CCoinControl / Use it in CreateTransaction * privatesend: Introduce CTransactionBuilder/CTransactionBuilderOutput Builder classes for transactions from type Inputs: Defined by CompactTallyItem Outputs: Simple outputs with lose reserve keys This takes fully takes care of fee calculations and makes sure calculations are the same like those happening when actually create the transaction with CreateTransaction. * privatesend: Improve amount/fee calculation in CreateDenominated * privatesend: Improve amount/fee calculation in MakeCollateralAmounts * qt: Fix decomposeTransaction's MakeCollateralAmounts detection Align it with the three cases in CPrivateSendClientSession::MakeCollateralAmounts * Refactor GetFee The fee rate is always coinControl.m_feerate, also it's not used outside so should be a private method * Simplify nBytesOutput calculations * Drop unused GetBytesOutput() * Make Clear(), GetBytesTotal() and GetAmountUsed() private * Drop unused GetCoinControl() * Make ToString() const * Refactor `CTransactionBuilder::Commit()` * Reorder cases in decomposeTransaction * Fix "finished" conditions in CreateDenominated * Fix typo * wallet|privatesend: Refactor CCoinControl's m_dust_feerate -> m_discard_feerate * privatesend: Drop unused member CTransactionBuilder::dustFeeRate * privatesend: Improve CTransactionBuilder's readability * privatesend: Make the static CTransactionBuilder::GetAmountLeft private * wallet: Recover previous code style * Update src/privatesend/privatesend-util.cpp Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> * Tweak CTransactionBuilder to respect potential compact size diffs * Tweak GetFee param type * Trivial log/comments tweaks * privatesend: Fix countPossibleOutputs - Fix off by one issue - Respect max outputs threshold * privatesend: Use GetSizeOfCompactSizeDiff in CTransactionBuilder * Apply suggestions from code review Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> * privatesend: Rename TryAdd to CouldAdd in CTransactionBuilder * wallet: Reset m_discard_feerate in CCoinControl::SetNull * Respect `-paytxfee` and `settxfee` * privatesend: Check for denominated amount only where really required * qt: Remove obsolete IsCollateralAmount() checks * privatesend: Don't accept negative amounts in CTransactionBuilder * privatesend: Remove unused CConnman parameter * use emplace_back instead of push_back Signed-off-by: pasta <pasta@dashboost.org> * fix typos Signed-off-by: pasta <pasta@dashboost.org> * make GetAmount const Signed-off-by: pasta <pasta@dashboost.org> * privatesend: Explicit capture __func__ in needMoreOutputs lambda * privatesend: Update CTransactionBuilder::UpdateAmount * remove const on parameter in declaration Signed-off-by: pasta <pasta@dashboost.org> * handle unsigned int -> int conversions a bit better Signed-off-by: pasta <pasta@dashboost.org> * explicitly cast to int from unsigned int. estimateSmartFee handles it if negative Signed-off-by: pasta <pasta@dashboost.org> * Make CTransactionBuilderOutput::GetScript const Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> * privatesend: Update comments to follow doxygen * privatesend: Add class descriptions Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Co-authored-by: pasta <pasta@dashboost.org>
* test: Implement unit tests for CWallet::CreateTransaction * test: Reset minRelayTxFee in CreateTransactionTest At this point its modified from an other test when running all unit tests which lets this test fail unexpectedly. * test: Lock cs_main/cs_wallet when required in CreateTransactionTest * test: Add new test in CreateTransactionTest Test if the wallet creation fails properly with the correct error messages. * test: Fixed expected test results for some CreateTransactionTest cases * test: Fail if CreateTransaction runs into "Exceed max tries" case * test: Adjust last return in CreateTransaction * test: Drop dust tests
* wallet: Remove unused vecTxDSInTmp in CWallet::CreateTransaction * wallet: Calculate fees earlier and respect them in change determination * wallet: Cleanup obsolete code in CreateTransaction This parts were needed before when the fee was calculated after the change was assigned. But now with the previous commit the fee is calculated upfront and respected properly from the begining. So there is no longer a need of increasing or decreasing the change depending on the fee as it has the correct value directly after its added. * wallet: Try to pick other inputs if the selected ones are too small If nChange is negative in this cases it means that the selected inputs can't cover the amount to send and the required transaction fee. So we just add the missing amount to nFeeRet. This leads to the algo trying to pick larger inputs in the next loop (with nFeeRet more duffs than in the previous loop). This process gets repeated until the selected amount is enough to cover all the costs or until the requested amount can't be selected anymore (not enough utxos to cover it). * wallet: Break the loop if the transaction is ready * wallet: Respect additional amount from previous cycles * wallet: Respect available in coin selection, try all coins in last round * wallet: Avoid potential infinite loop, just in case.. * wallet: Fix signing in CreateTransaction Prior it was messed because of 60d96a1. Set coins isn't sorted the same way as txNew.vin is so it sometimes may pick wrong coins for signing the input. * wallet: Fix change calculation if "subtract fee from amount" is enabled * wallet: Return after fee calc if no or not enough amount available Return the proper fail reason. Prior to this commit it run into "Exceeded max tried". Note: Only return if not enough amount is available if we can't subtract fee from amount. * wallet: Fix break logic if available amount is not enough * Revert "wallet: Fix signing in CreateTransaction" This reverts commit 5fcdc0f00e7b961ebb62c94d17d585537e911309. * Use a vector of coins instead of a set in CreateTransaction and sort it in a BIP69 compliant way when needed * wallet: Adjust comment * Cleaner usage of nChangePosRequest/InOut * Simplify some fail/try-again conditions (fixed) * Loop through outputs to update change output position only when outputs are sorted for BIP69 No need to do this for non-bip69 cases (i.e. when a specific change output position was requested and assigned) * Avoid implicit conversions of int-s into bool-s * Move `nAmountLeft == nFeeRet` check higher, tweak comments * wallet: Fix some formatting * wallet: Improve CTxIn creation in CreateTransaction Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* makefile.test.include: Let privatesend_tests.cpp depend on ENABLE_WALLET * test: Implement unit tests for CTransactionBuilder * Check that we can decrease the amount and GetAmountLeft() is updated accordingly * Check if resulting tx has a change output when expected * Avoid pushing nullptr into vecOutputs * Add few notes about size calculations * nit: better readability (imo) Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* qt: Adjust green color * qt: Don't disable the HD icon Disabling it leads to incorrect colors due to opacity. * qt: Move the HD icon to the left Reason: Its the only one not turning green by default if all is set properly. So this lets is look better imo. * qt: Increase statusbar icon size * qt: Show lock icon in status bar also if wallet is not encrypted Just as signal to the user! * qt: Refine connection icon usage * qt: Use state related colors in statusbar * qt: Hide the HD icon for non-HD wallets * qt: Update HD icon
* masternode: Replace sync states INITIAL and WAITING with BLOCKCHAIN * masternode: Peer dependent "assume tip" timeout I would say its enough to only wait 1 tick if we have more than 3 peers before we move over to governance sync. * masternode: Notify the UI instantly if switched to governance sync Without this it takes one iteration more for the UI to receive the update. * masternode: Notify the UI about CMasternodeSync::Reset calls * masternode: Don't instantly reset the sync process Give it MASTERNODE_SYNC_RESET_SECONDS (600) seconds time after the last UpdateBlockTip call. * rpc: Don't switch to next asset in "mnsync reset" * rpc: Force the reset in "mnsync reset" * net: Make sure the sync gets a reset if required after network changes This will reset the sync process if its outdated in the following cases: - If the connections dropped to zero - If the connections went from zero to one - If the network has been enabled or disabled * Apply suggestions from code review Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> * net: Only open masternode connections if the blockchain is synced In general it doesn't make sense to connect to masternodes before due to MNAUTH requires blockchain sync. This could lead to failing quorum connections/failing masternode probing.. if a just restarted node/a out of sync node would hit a dkg block.. Then they would not try to open those llmq/probing connections for the next 60s (nLLMQConnectionRetryTimeout). Thats basically what happens in tests right now and they fail without this commit. * test: Make sure nodes are synced when they get restored after isolation Their sync might be out of date otherwise due to bigger mocktime bumps Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
- Add BitcoinGUI::updateWalletStatus() - Add WalletModel::getWalletModel() - Make WalletFrame::currentWalletView public
…#3698) * qt: Increase number of spinner frames * qt: Add and use BitcoinGUI::startSpinner and BitcoinGUI::stopSpinner * qt: Handle CMasternodeSync::Reset calls * qt: Make sure the statusbar always reflects the sync state/progress * qt: Add BitcoinGUI::updateProgressBarVisibility * qt: Animate connection icon while connecting * qt: Refactor check in BitcoinGUI::updateProgressBarVisibility * qt: Rename some variables * Update src/qt/bitcoingui.cpp Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
* qt: Align warning icon in ModalOverlay properly with the text * qt: Center form layout in modaloverlay
xdustinface
changed the title
PastaPastaPasta
approved these changes
Sep 13, 2020
Member
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK, changes match expected
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For this PR i added #3689 to the 16 milestone/added backport label + I partially backported #3604 in 9e425d6 without adding it to the 16 milestone, any concerns about this?
Also,
llmq-signing.py --spork21might fail on CI since we don't havetimeoutscaleinv.0.16.x. At least it failed in my gitlab shared runner setup. Passes locally though..