Use CCoinControl selection in CWallet::FundTransaction #7506
Conversation
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
|
@laanwj is this acceptable for the 0.12 release cycle? |
|
Unless it fixes a critical bug, no. This will be reviewed and merged for 0.13. |
|
utACK 8310baf |
|
Matt was here |
|
utACK 8310baf |
| } | ||
| } | ||
| if (!found) | ||
| if (!coinControl.IsSelected(txin.prevout)) |
There was a problem hiding this comment.
Is the old and new code equivalent here?
To me it looks like coinControl.IsSelected and the old code check for different things: IsSelected checks coincontrol.setSelected, whereas the old code just looks at wtx and tx.
Can you explain?
There was a problem hiding this comment.
Yes they are. The only difference is that the old code performs a linear search and reimplements the COutPoint::operator== inline.
coinControl.setSelected is filled with the current inputs, which are the ones to search when the inputs are added, otherwise the same input would be duplicated.
There was a problem hiding this comment.
Thanks for the explanation, makes sense to me. I missed the part where setSelected was prepopulated.
|
Do I understand this correctly: this only make sense if you use the GUI (CoinControl enabled) and Although looking at this PR from an angle where CCoinControl is a core-class (not tied to the GUI) I think this would make sense. What real-world use cases would solve this? |
|
@jonasschnelli this is only an enhancement of the |
|
@promag: yes. I saw that this only affects fundrawtransaction. But IIRC, CoinControl can only be used over the GUI. So does that mean, that this change only affects users who uses the GUI together with fundrawtransaction (console/RPC)? |
|
|
|
@jonasschnelli maybe CCoinControl should be considered core/wallet and not gui? |
|
@promag: but there is no functionality/possibility to use CoinControl in core/wallet (non gui)? |
|
Maybe I'm not understanding what you're saying. I'm just saying |
|
utACK |
|
@promag: needs a (force)push for a travis re-trigger. |
8310baf to
d6cc6a1
Compare
|
utACK |
|
@laanwj bump. |
laanwj
added a commit
that referenced
this pull request
Mar 24, 2016
d6cc6a1 Use CCoinControl selection in CWallet::FundTransaction (João Barbosa)
codablock
pushed a commit
to codablock/dash
that referenced
this pull request
Sep 7, 2017
…ction d6cc6a1 Use CCoinControl selection in CWallet::FundTransaction (João Barbosa)
UdjinM6
pushed a commit
to dashpay/dash
that referenced
this pull request
Sep 9, 2017
* Merge bitcoin#7506: Use CCoinControl selection in CWallet::FundTransaction d6cc6a1 Use CCoinControl selection in CWallet::FundTransaction (João Barbosa) * Merge bitcoin#7732: [Qt] Debug window: replace "Build date" with "Datadir" fc737d1 [Qt] remove unused formatBuildDate method (Jonas Schnelli) 4856f1d [Qt] Debug window: replace "Build date" with "Datadir" (Jonas Schnelli) * Merge bitcoin#7707: [RPC][QT] UI support for abandoned transactions 8efed3b [Qt] Support for abandoned/abandoning transactions (Jonas Schnelli) * Merge bitcoin#7688: List solvability in listunspent output and improve help c3932b3 List solvability in listunspent output and improve help (Pieter Wuille) * Merge bitcoin#8006: Qt: Add option to disable the system tray icon 8b0e497 Qt: Add option to hide the system tray icon (Tyler Hardin) * Merge bitcoin#8073: qt: askpassphrasedialog: Clear pass fields on accept 02ce2a3 qt: askpassphrasedialog: Clear pass fields on accept (Pavel Vasin) * Merge bitcoin#8231: [Qt] fix a bug where the SplashScreen will not be hidden during startup b3e1348 [Qt] fix a bug where the SplashScreen will not be hidden during startup (Jonas Schnelli) * Merge bitcoin#8257: Do not ask a UI question from bitcoind 1acf1db Do not ask a UI question from bitcoind (Pieter Wuille) * Merge bitcoin#8463: [qt] Remove Priority from coincontrol dialog fa8dd78 [qt] Remove Priority from coincontrol dialog (MarcoFalke) * Merge bitcoin#8678: [Qt][CoinControl] fix UI bug that could result in paying unexpected fee 0480293 [Qt][CoinControl] fix UI bug that could result in paying unexpected fee (Jonas Schnelli) * Merge bitcoin#8672: Qt: Show transaction size in transaction details window c015634 qt: Adding transaction size to transaction details window (Hampus Sjöberg) \-- merge fix for s/size/total size/ fdf82fb Adding method GetTotalSize() to CTransaction (Hampus Sjöberg) * Merge bitcoin#8371: [Qt] Add out-of-sync modal info layer 08827df [Qt] modalinfolayer: removed unused comments, renamed signal, code style overhaul (Jonas Schnelli) d8b062e [Qt] only update "amount of blocks left" when the header chain is in-sync (Jonas Schnelli) e3245b4 [Qt] add out-of-sync modal info layer (Jonas Schnelli) e47052f [Qt] ClientModel add method to get the height of the header chain (Jonas Schnelli) a001f18 [Qt] Always pass the numBlocksChanged signal for headers tip changed (Jonas Schnelli) bd44a04 [Qt] make Out-Of-Sync warning icon clickable (Jonas Schnelli) 0904c3c [Refactor] refactor function that forms human readable text out of a timeoffset (Jonas Schnelli) * Merge bitcoin#8805: Trivial: Grammar and capitalization c9ce17b Trivial: Grammar and capitalization (Derek Miller) * Merge bitcoin#8885: gui: fix ban from qt console cb78c60 gui: fix ban from qt console (Cory Fields) * Merge bitcoin#8821: [qt] sync-overlay: Don't block during reindex fa85e86 [qt] sync-overlay: Don't show estimated number of headers left (MarcoFalke) faa4de2 [qt] sync-overlay: Don't block during reindex (MarcoFalke) * Support themes for new transaction_abandoned icon * Fix constructor call to COutput * Merge bitcoin#7842: RPC: do not print minping time in getpeerinfo when no ping received yet 62a6486 RPC: do not print ping info in getpeerinfo when no ping received yet, fix help (Pavel Janík) * Merge bitcoin#8918: Qt: Add "Copy URI" to payment request context menu 21f5a63 Qt: Add "Copy URI" to payment request context menu (Luke Dashjr) * Merge bitcoin#8925: qt: Display minimum ping in debug window. 1724a40 Display minimum ping in debug window. (R E Broadley) * Merge bitcoin#8972: [Qt] make warnings label selectable (jonasschnelli) ef0c9ee [Qt] make warnings label selectable (Jonas Schnelli) * Make background of warning icon transparent in modaloverlay * Merge bitcoin#9088: Reduce ambiguity of warning message 77cbbd9 Make warning message about wallet balance possibly being incorrect less ambiguous. (R E Broadley) * Replace Bitcoin with Dash in modal overlay * Remove clicked signals from labelWalletStatus and labelTransactionsStatus As both are really just labels, clicking on those is not possible. This is different in Bitcoin, where these labels are actually buttons. * Pull out modaloverlay show/hide into it's own if/else block and switch to time based check Also don't use masternodeSync.IsBlockchainSynced() for now as it won't report the blockchain being synced before the first block (or other MN data?) arrives. This would otherwise give the impression that sync is being stuck.
1 task
furszy
added a commit
to PIVX-Project/PIVX
that referenced
this pull request
Jun 28, 2020
fc81158 [QA] Add test_change_position case to rpc_fundrawtransaction.py (random-zebra) dd35760 [QA] Add test_option_feerate to rpc_fundrawtransaction functional test (random-zebra) 5bca4f4 Add more clear interface for CoinControl.h regarding individual feerate (random-zebra) 169bc3b [RPC] add feerate option to fundrawtransaction (random-zebra) 87dbdf8 [QA] Test new options in rpc_fundrawtransaction functional test (random-zebra) bc9dc67 Add lockUnspents option to fundrawtransaction (random-zebra) a3ac191 Add change options to fundrawtransaction (random-zebra) 0c1f7ba Add strict flag to RPCTypeCheckObj (random-zebra) d655b42 Use CCoinControl selection in CWallet::FundTransaction (random-zebra) 76c8d54 [QA] Test watchonly addrs in fundrawtransaction tests (random-zebra) 134c5d2 Implement watchonly support in fundrawtransaction (random-zebra) 1b153e5 Update importaddress help to push its use to script-only (random-zebra) 7b4eb6d Add importpubkey method to import a watch-only pubkey (random-zebra) 816dabb Add p2sh option to importaddress to import redeemScripts (random-zebra) 60a20a4 Split up importaddress into helper functions (random-zebra) cbffa80 Add logic to track pubkeys as watch-only, not just scripts (random-zebra) 12b38b0 Add have-pubkey distinction to ISMINE flags (random-zebra) fab6556 Exempt unspendable transaction outputs from dust checks (random-zebra) ab407ff [Tests] Fix and enable fundrawtransaction functional tests (random-zebra) bc44ba0 [wallet] allow transaction without change if keypool is empty (random-zebra) a2f8071 [wallet] CreateTransaction: simplify change address check (random-zebra) 761e60e Add fundrawtransaction RPC method (random-zebra) ccb18dd Add FundTransaction method to wallet (random-zebra) 692b827 Add DummySignatureCreator which just creates zeroed sigs (random-zebra) Pull request description: based on top of - [x] #1662 This introduces a new wallet function, `CWallet::FundTransaction()` (and exposes it via RPC with `fundrawtransaction`), to fill a tx containing only vouts (or not enough vins to cover the vouts) with unspent coins from the wallet. `fundrawtransaction` will not modify existing inputs, and will add one change output (if needed) to the outputs. It will not sign the inputs (so can include also watch-only or multi-sig inputs, if enabled). backported from: - bitcoin#6088 - bitcoin#17219 [`*`] - bitcoin#6417 - bitcoin#6444 - bitcoin#6415 - bitcoin#6828 - bitcoin#7296 (only bebe58b) - bitcoin#7506 - bitcoin#7518 - bitcoin#7967 adapting the tests for the (more recent) framework. [`*`] Note: this has been included to be able to call `fundrawtransaction` without the need for an unencrypted wallet (for the change address key) ACKs for top commit: furszy: re ACK fc81158 . Fuzzbawls: ACK fc81158 Tree-SHA512: 10235ce6e672a1cfd4ae2cad9312864c82971f6a4aa1a8ed9489d85156f5c4126c293180a7f1b86b7c65d07caab484e9a6d7a87ebf032bee55adb98d3e08e7b9
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Trivial improvement.