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.

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

Use CCoinControl selection in CWallet::FundTransaction #7506

Merged
merged 1 commit into from Mar 24, 2016

Conversation

Projects
None yet
7 participants
@promag
Member

promag commented Feb 11, 2016

Trivial improvement.

@laanwj laanwj added the Wallet label Feb 11, 2016

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Feb 11, 2016

Member

@laanwj is this acceptable for the 0.12 release cycle?

Member

promag commented Feb 11, 2016

@laanwj is this acceptable for the 0.12 release cycle?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 11, 2016

Member

Unless it fixes a critical bug, no. This will be reviewed and merged for 0.13.

Member

laanwj commented Feb 11, 2016

Unless it fixes a critical bug, no. This will be reviewed and merged for 0.13.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Feb 11, 2016

Contributor

utACK 8310baf

Contributor

dcousens commented Feb 11, 2016

utACK 8310baf

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Feb 11, 2016

Contributor

Matt was here

Contributor

TheBlueMatt commented Feb 11, 2016

Matt was here

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Feb 13, 2016

Member

utACK 8310baf

Member

MarcoFalke commented Feb 13, 2016

utACK 8310baf

}
}
if (!found)
if (!coinControl.IsSelected(txin.prevout))

This comment has been minimized.

@laanwj

laanwj Feb 17, 2016

Member

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?

@laanwj

laanwj Feb 17, 2016

Member

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?

This comment has been minimized.

@promag

promag Feb 17, 2016

Member

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.

@promag

promag Feb 17, 2016

Member

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.

This comment has been minimized.

@laanwj

laanwj Feb 17, 2016

Member

Thanks for the explanation, makes sense to me. I missed the part where setSelected was prepopulated.

@laanwj

laanwj Feb 17, 2016

Member

Thanks for the explanation, makes sense to me. I missed the part where setSelected was prepopulated.

@jonasschnelli jonasschnelli added the GUI label Feb 19, 2016

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Feb 19, 2016

Member

Do I understand this correctly: this only make sense if you use the GUI (CoinControl enabled) and fundrawtransaction (over RPC with the -server option or over the GUI console)?

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?

Member

jonasschnelli commented Feb 19, 2016

Do I understand this correctly: this only make sense if you use the GUI (CoinControl enabled) and fundrawtransaction (over RPC with the -server option or over the GUI console)?

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?

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Feb 19, 2016

Member

@jonasschnelli this is only an enhancement of the CWallet::FundTransaction implementation.

Member

promag commented Feb 19, 2016

@jonasschnelli this is only an enhancement of the CWallet::FundTransaction implementation.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Feb 20, 2016

Member

@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)?
What usecases would be possible with this change?

Member

jonasschnelli commented Feb 20, 2016

@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)?
What usecases would be possible with this change?

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Feb 20, 2016

Member

fundrawtransactionwas already using CCoinControl.

Member

promag commented Feb 20, 2016

fundrawtransactionwas already using CCoinControl.

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Feb 25, 2016

Member

@jonasschnelli maybe CCoinControl should be considered core/wallet and not gui?

Member

promag commented Feb 25, 2016

@jonasschnelli maybe CCoinControl should be considered core/wallet and not gui?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Feb 25, 2016

Member

@promag: but there is no functionality/possibility to use CoinControl in core/wallet (non gui)?

Member

jonasschnelli commented Feb 25, 2016

@promag: but there is no functionality/possibility to use CoinControl in core/wallet (non gui)?

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Feb 25, 2016

Member

Maybe I'm not understanding what you're saying. I'm just saying fundrawtransaction is a wallet RPC command and it uses CCoinControl.

Member

promag commented Feb 25, 2016

Maybe I'm not understanding what you're saying. I'm just saying fundrawtransaction is a wallet RPC command and it uses CCoinControl.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli
Member

jonasschnelli commented Feb 25, 2016

utACK

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 8, 2016

Member

@promag: needs a (force)push for a travis re-trigger.

Member

jonasschnelli commented Mar 8, 2016

@promag: needs a (force)push for a travis re-trigger.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Mar 16, 2016

Member

utACK

Member

jtimon commented Mar 16, 2016

utACK

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Mar 24, 2016

Member

@laanwj bump.

Member

promag commented Mar 24, 2016

@laanwj bump.

@laanwj laanwj merged commit d6cc6a1 into bitcoin:master Mar 24, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 24, 2016

Merge #7506: Use CCoinControl selection in CWallet::FundTransaction
d6cc6a1 Use CCoinControl selection in CWallet::FundTransaction (João Barbosa)

codablock added a commit to codablock/dash that referenced this pull request Sep 7, 2017

Merge #7506: Use CCoinControl selection in CWallet::FundTransaction
d6cc6a1 Use CCoinControl selection in CWallet::FundTransaction (João Barbosa)

UdjinM6 added a commit to dashpay/dash that referenced this pull request Sep 9, 2017

Backport Bitcoin Qt/Gui changes up to 0.14.x part 2 (#1615)
* Merge #7506: Use CCoinControl selection in CWallet::FundTransaction

d6cc6a1 Use CCoinControl selection in CWallet::FundTransaction (João Barbosa)

* Merge #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 #7707: [RPC][QT] UI support for abandoned transactions

8efed3b [Qt] Support for abandoned/abandoning transactions (Jonas Schnelli)

* Merge #7688: List solvability in listunspent output and improve help

c3932b3 List solvability in listunspent output and improve help (Pieter Wuille)

* Merge #8006: Qt: Add option to disable the system tray icon

8b0e497 Qt: Add option to hide the system tray icon (Tyler Hardin)

* Merge #8073: qt: askpassphrasedialog: Clear pass fields on accept

02ce2a3 qt: askpassphrasedialog: Clear pass fields on accept (Pavel Vasin)

* Merge #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 #8257: Do not ask a UI question from bitcoind

1acf1db Do not ask a UI question from bitcoind (Pieter Wuille)

* Merge #8463: [qt] Remove Priority from coincontrol dialog

fa8dd78 [qt] Remove Priority from coincontrol dialog (MarcoFalke)

* Merge #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 #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 #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 #8805: Trivial: Grammar and capitalization

c9ce17b Trivial: Grammar and capitalization (Derek Miller)

* Merge #8885: gui: fix ban from qt console

cb78c60 gui: fix ban from qt console (Cory Fields)

* Merge #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 #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 #8918: Qt: Add "Copy URI" to payment request context menu

21f5a63 Qt: Add "Copy URI" to payment request context menu (Luke Dashjr)

* Merge #8925: qt: Display minimum ping in debug window.

1724a40 Display minimum ping in debug window. (R E Broadley)

* Merge #8972: [Qt] make warnings label selectable (jonasschnelli)

ef0c9ee [Qt] make warnings label selectable (Jonas Schnelli)

* Make background of warning icon transparent in modaloverlay

* Merge #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.

@dagurval dagurval referenced this pull request Dec 27, 2017

Merged

Add fundrawtransaction #288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment