[gui] Defer coin control instancing #12327

pull promag wants to merge 1 commits into bitcoin:master from promag:2018-02-fix-12312 changing 3 files +25 −20

promag commented at 5:00 PM on February 1, 2018: member

Defer the GUI coin control instancing so that argument processing is taken into account for the default coin control values.

Fixes #12312
promag commented at 5:02 PM on February 1, 2018: member

Note to reviewers, I would like to refactor the code, specially to remove these (or some) static values, but I though maybe we should fix the problem first.
instagibbs commented at 5:21 PM on February 1, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12327/commits/b3e74d626e6c2109492f490a4c8271b4b0bd52c2

This should fix both g_change_type; and signalRbf being set at instance time to variables not yet set globally. Other settings are set per-usage of CCoinControl or are simply not used when not explicitly set, such as m_feerate.
laanwj added the label GUI on Feb 1, 2018
laanwj added this to the milestone 0.16.0 on Feb 1, 2018
laanwj commented at 5:26 PM on February 1, 2018: member

Note to reviewers, I would like to refactor the code, specially to remove there (or some) static values,

Yes, we should get rid of the global coincontrol object and just pass it around and store it where necessary. But indeed, not a priority for fixing this.
in src/qt/coincontroldialog.cpp:605 in b3e74d626e outdated
```
 598 | @@ -600,6 +599,13 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog)
 599 |          label->setVisible(nChange < 0);
 600 |  }
 601 |  
 602 | +CCoinControl* CoinControlDialog::coinControl()
 603 | +{
 604 | +    static CCoinControl* coin_control = nullptr;
 605 | +    if (!coin_control) coin_control = new CCoinControl();
```
laanwj commented at 5:28 PM on February 1, 2018:

I think we leak this pointer?

If you'd make this a smart pointer, it'd get freed at shutdown at least.

promag commented at 5:30 PM on February 1, 2018:

It was leaking already.

laanwj commented at 5:37 PM on February 1, 2018:

But wouldn't that be a one-line change, to use static unique_ptr<CCoinControl> coin_control;? Or am I missing something?

unknown commented at 5:46 PM on February 1, 2018:

Do a Meyer's singleton

CCoinControl& CoinControlDialog::coinControl() { static CCoinControl coin_control; return coin_control; }

No pointers, no leaks.

promag commented at 5:52 PM on February 1, 2018:

In that case when is coin_control instanced? I think the problem remains, but please correct me if I'm wrong.

promag commented at 5:52 PM on February 1, 2018:

For now, pushed with std::unique_ptr as suggested by @laanwj.

laanwj commented at 6:10 PM on February 1, 2018:

Thanks!

I think @kekimusmaximus's solution also works, static within a function is initialized on first use.

promag commented at 6:14 PM on February 1, 2018:

That's new to me, I'll change to that instead.

in src/qt/coincontroldialog.cpp:381 in b3e74d626e outdated

 377 | @@ -379,11 +378,11 @@ void CoinControlDialog::viewItemChanged(QTreeWidgetItem* item, int column)
 378 |          COutPoint outpt(uint256S(item->text(COLUMN_TXHASH).toStdString()), item->text(COLUMN_VOUT_INDEX).toUInt());
 379 |  
 380 |          if (item->checkState(COLUMN_CHECKBOX) == Qt::Unchecked)
 381 | -            coinControl->UnSelect(outpt);
 382 | +            CoinControlDialog::coinControl()->UnSelect(outpt);

laanwj commented at 5:29 PM on February 1, 2018:

No need to prefix with CoinControlDialog:: here, this is still in the class.

promag commented at 5:30 PM on February 1, 2018:

I know, but this way it's clear it's a static member.

laanwj commented at 5:31 PM on February 1, 2018:

Let's not do that, we intend to make it non-static field later anyway, so this seems moving the wrong direction.

promag commented at 5:37 PM on February 1, 2018: member

Pushed a commit with @laanwj suggestion if others want to suggest other way.
laanwj commented at 5:38 PM on February 1, 2018: member

I think ideally we'd instantiate the CCoinControl at a defined time during the GUI initialization process (when we're sure the options have been parsed), instead at first use. But this works I guess...
laanwj added the label Needs backport on Feb 1, 2018
meshcollider commented at 7:05 PM on February 1, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/12327/commits/29e7f9f7a2afa6836fbbe7371000805dfb8ec638 after squash
jonasschnelli commented at 7:12 PM on February 1, 2018: contributor

utACK 29e7f9f7a2afa6836fbbe7371000805dfb8ec638 , please squash.

[gui] Defer coin control instancing

Defer the GUI coin control instancing so that argument processing
is taken into account for the default coin control values.

6558f8acc3

promag force-pushed on Feb 1, 2018
promag commented at 7:47 PM on February 1, 2018: member

Done @jonasschnelli @MeshCollider.
Sjors commented at 7:50 PM on February 1, 2018: member

Tested 6558f8a. Enabling coin control now honors -changetype for me, and also uses a native segwit change address when sending to a bech32 address.
jonasschnelli merged this on Feb 1, 2018
jonasschnelli closed this on Feb 1, 2018
jonasschnelli referenced this in commit 41363fe11d on Feb 1, 2018
laanwj referenced this in commit 0dc6a30ef3 on Feb 2, 2018
promag deleted the branch on Feb 4, 2018
laanwj removed the label Needs backport on Feb 8, 2018
HashUnlimited referenced this in commit dcfdedda54 on Mar 16, 2018
ccebrecos referenced this in commit 1050278fe5 on Sep 14, 2018
PastaPastaPasta referenced this in commit b2af19b664 on Apr 4, 2020
PastaPastaPasta referenced this in commit 625142bfa0 on Apr 5, 2020
ckti referenced this in commit 0f5f6537b8 on Mar 28, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.16.0

Linked (view graph)

#12312 QT ignores -changetype=bech32 when coin control features are enabled