wallet: Fix and improve CWallet::CreateTransaction #3668
Merged
Conversation
This file contains 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
|
Ready for review! |
|
Soo, i hope thats it now.. Seems like i confused myself a bit to believe it would be good to accept too high amounts as recipient and then send less fee than actually wanted/needed 🙈 |
|
Pls see https://github.com/UdjinM6/dash/commits/pr3668 for some suggestions |
|
Added 7837b3b and e732b94. Didn't add 679fb9c as it introduces bugs leading to So, if you want those simplifications you should rework them. Im currently not in the mood to find those bugs, im really totally over debugging/fixing the logic of this method for now.. it was just too much the last days 😆 |
|
Pls see https://github.com/UdjinM6/dash/commits/pr3667 starting with b9947ee (I merged 3667 into this branch and applied new fixes on top - all tests pass locally for me now) |
|
Looks good now yeah, picked it with the rest 👍 |
PastaPastaPasta
requested changes
Aug 30, 2020
PastaPastaPasta
previously approved these changes
Aug 31, 2020
|
Needs rebase |
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.
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).
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.
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.
…ts 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)
xdustinface
dismissed stale reviews from PastaPastaPasta and UdjinM6
via
eaf9da4
xdustinface
force-pushed
the
pr-wallet-createtransaction
branch
from
fdf3829 to
eaf9da4
Compare
|
Rebased and added a059633 |
|
I don't think d5a5311 belongs here... |
xdustinface
force-pushed
the
pr-wallet-createtransaction
branch
from
eaf9da4 to
a059633
Compare
nah, all good 😆 but it does not really longer match upstream because i moved it around anyways |
xdustinface
commented
Sep 1, 2020
| // nLockTime set above actually works. | ||
| vecTxDSInTmp.clear(); | ||
| for (const auto& coin : setCoins) { | ||
| CTxIn txin = CTxIn(coin.outpoint,CScript(), |
There was a problem hiding this comment.
@PastaPastaPasta look, it was here before. Now its 100 lines above
There was a problem hiding this comment.
Yeah... But git can be surprising good at resolving conflicts like this
There was a problem hiding this comment.
Come on, this method is already so far from upstream due to segwit, privatesend and now this bugfixing that this single line which is already 100 lines above the origin being a bit more like upstream won't bring any merge improvement, would it? This will probably just be a big conflict block no matter if a059633 is added or not 😅
PastaPastaPasta
approved these changes
Sep 3, 2020
xdustinface
added a commit
to xdustinface/dash
that referenced
this pull request
Sep 12, 2020
* 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 5fcdc0f. * 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>
xdustinface
added a commit
to xdustinface/dash
that referenced
this pull request
Sep 12, 2020
* 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 5fcdc0f. * 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>
xdustinface
added a commit
to xdustinface/dash
that referenced
this pull request
Sep 12, 2020
* 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 5fcdc0f. * 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>
xdustinface
added a commit
to xdustinface/dash
that referenced
this pull request
Sep 12, 2020
* 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 5fcdc0f. * 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>
gades
pushed a commit
to cosanta/cosanta-core
that referenced
this pull request
Mar 10, 2022
* 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 5fcdc0f. * 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>
5 tasks
PastaPastaPasta
pushed a commit
that referenced
this pull request
Feb 9, 2024
…hify help text, undashify code comments (#5852) ## Issue being fixed or feature implemented This pull request is a follow-up to [some](#5834 (comment)) [feedback](#5834 (comment)) received on [dash#5834](#5834) as the patterns highlighted were present in different parts of the codebase and hence not corrected within the PR itself but addressed separately. This is that separate PR 🙂 (with some additional cleanup of my own) ## What was done? * This pull request will remain a draft until [dash#5834](#5834) as it will introduce more changes that will need to be corrected in this PR. * Code introduced that is unique to Dash Core (CoinJoin, InstantSend, etc.) has been excluded from un-Dashification as the purpose of it is to reduce backport conflicts, which don't apply in those cases. * `CWallet::CreateTransaction` and the `CreateTransactionTest` fixture have been excluded as the former originates from [dash#3668](#3668) and the latter from [dash#3667](#3667) and are distinct enough to be unique to Dash Core. * There are certain Dashifications and SegWit-removals that prove frustrating as it would break compatibility with programs that rely on the naming of certain keys * `getrawmempool`, `getmempoolancestors`, `getmempooldescendants` and `getmempoolentry` return `vsize` which is currently an alias of `size`. I have been advised to retain `vsize` in lieu of potential future developments. (this was originally remedied in 219a1d0 but has since been dropped) * `getaddressmempool`, `getaddressutxos` and `getaddressdeltas` all return a value with the key `satoshis`. This is frustrating to rename to `duffs` for compatibility reasons. * `decodepsbt` returns (if applicable) `non_witness_utxo` which is frustrating to rename simply to `utxo` for the same reason. * `analyzepsbt` returns (if applicable) `estimated_vsize` which frustrating to rename to `estimated_size` for the same reason. ## How Has This Been Tested? ## Breaking Changes None ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Feb 12, 2025
(wallet backports: part 1) 153bdc2 merge bitcoin#22155: Add test for subtract fee from recipient behavior (Kittywhiskers Van Gogh) 0185847 fix: correct fee calculations in `CreateTransactionInternal` (Kittywhiskers Van Gogh) 445f489 merge bitcoin#17331: Use effective values throughout coin selection (Kittywhiskers Van Gogh) 7e54bd9 wallet: copy and sort `vecSend` if BIP69 sorting enabled, enable sorting (Kittywhiskers Van Gogh) 9e9e66f partial bitcoin#17331: Use effective values throughout coin selection (Kittywhiskers Van Gogh) 66fe2d4 merge bitcoin#25497: more accurate target for large transactions (Kittywhiskers Van Gogh) 6e4d789 wallet: add back missing `CoinSelectionParams` assignments (Kittywhiskers Van Gogh) bd35042 wallet: move `CoinSelectionParams` to positions expected upstream (Kittywhiskers Van Gogh) 0711e67 wallet: shuffle transaction inputs if we aren't using BIP69 (Kittywhiskers Van Gogh) 1cf1c58 refactor: move selected coin and txin sorting to the end of the scope (Kittywhiskers Van Gogh) ab756ba wallet: Fail if maximum weight is too large (Kittywhiskers Van Gogh) 05c319e refactor: move oversized transaction check to tail end of scope (Kittywhiskers Van Gogh) 6ca51df wallet: Use existing feerate instead of getting a new one (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6543 * Dependency for #6529 * [bitcoin#17331](bitcoin#17331) logically partially reverts [dash#3368](#3668) as Dash Core implemented a calculate-before approach (compared to _then_ Bitcoin Core's calculate-and-adjust approach) and it is being replaced with the current upstream calculate-after approach done in a single-pass instead of iteratively (like the former two). * As the changes are non-trivial, they have been split into a "partial" and a "merge" commit, the first half dedicated just to the logical partial revert and the latter half dedicated to using effective values in coin selection. * BIP69 sorting is disabled in the former half to allow the fix to be in a separate commit while allowing validation of the remaining set of changes. The fix re-enables BIP69 sorting. * Due to the changes introduced in [dash#3368](#3668), a lot of then-redundant code was removed and changes to it upstream were not mirrored in Dash Core. To allow [bitcoin#17331](bitcoin#17331) to work properly, a lot of that unmirrored code was reintroduced and existing code readjusted to match upstream. * `coin_selection_params.tx_noinputs_size` is said to have a size (sans output count) of `9` instead of `10` as we don't have a SegWit field (referred to as `1 witness overhead (dummy, flag, stack size)` in a code comment) on account of not having SegWit. * To allow for backporting [bitcoin#17331](bitcoin#17331), portions of [bitcoin#21083](bitcoin#21083) (1a6a0b0) and [bitcoin#20536](bitcoin@51e2cd3) (3e69939) were backported. * [bitcoin#17331](bitcoin#17331) seems to have silently broken `CreateTransactionInternal` as functional tests fail (see below) despite the backport not intending to change behavior. This was caught due to unit tests introduced in [dash#3667](#3667). The aberration seems be remedied by portions of [bitcoin#25647](bitcoin#25647) and [bitcoin#26643](bitcoin#26643) and they have been incorporated into this pull request in a separate commit. **Special thanks to UdjinM6 for figuring this out!** 🎉 <details> <summary>Error log:</summary> ``` dash@479e0aa4ebbf:/src/dash$ ./src/test/test_dash -t coinselector_tests,wallet_tests Running 21 test cases... wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 2 - 5 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 4 - 4 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 4 - 5 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 6 - 0 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 6 - 2 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 6 - 4 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 6 - 5 *** 7 failures are detected in the test module "Dash Core Test Suite" ``` </details> ## How Has This Been Tested? 153bdc2 was tested on Debian 12 (`bookworm`) mixing ~2 tDASH on default settings.  ## Breaking Changes * If a transaction isn't shuffled using BIP69 (i.e. if an explicit position for the change txout is specified), it will be randomly shuffled (mirroring upstream behavior, [source](https://github.com/bitcoin/bitcoin/blob/51a3ac242c92e69b59df26f8f9e287b31e5c3b0f/src/wallet/wallet.cpp#L3048)). This deviates from earlier behavior where no shuffling would be done at all if BIP69 isn't applied. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 153bdc2 PastaPastaPasta: utACK 153bdc2 Tree-SHA512: 709b77dce3cea2bbf09eab42c7e70171c3bc6527ff4c387a4db75994da5c0d59025b641d90f734b87a62cdfa8e422d09513697a6e875635de2765a1c9141233e
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR fixes/improves fee calculation and with that reasons for the failing test case 2/4 of
CreateTransactionTestunit tests introduced in #3667.@UdjinM6 Test 2/4 is the issue we discovered in #3657 aka the previous logic did not subtract fees from change before checking if it’s dust or not. If fees are much higher than the discard/dust rate and the change after fee was actually dust, it still tried to add a change output which then caused it to fail if only given inputs were allowed.