Skip to content
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

qt: fix confirmed transaction labeled "open" (#13299) #14556

Merged
merged 1 commit into from Jan 15, 2019

Conversation

Projects
None yet
8 participants
@hebasto
Copy link
Member

commented Oct 23, 2018

Fix #13299.

Since the default nSequence is 0xFFFFFFFE and locktime is enabled, the checking wtx.is_final is meaningless until the syncing has completed (ref: #1026).

This PR makes the wallet mark a transaction "Unconfirmed" instead of misleading "Open for NNN more blocks" when syncing after a period of being offline.

Before this PR (with the issue):
screenshot from 2018-12-12 15-56-23

With this PR (the issue has been resolved):
screenshot from 2018-12-12 15-54-41

@MarcoFalke MarcoFalke added the GUI label Oct 23, 2018

src/qt/transactionrecord.cpp Outdated
QDateTime currentDate = QDateTime::currentDateTime();
qint64 secs = blockDate.secsTo(currentDate);

if ((secs < 90 * 60) && !wtx.is_final) {

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 23, 2018

Member

90 * 60 here is meant to match the if(secs < 90*60) in src/qt/bitcoingui.cpp, right? If so, I suggest adding a common constant to make sure these values stay in sync over time :-)

This comment has been minimized.

Copy link
@hebasto

hebasto Oct 23, 2018

Author Member

Nice. Which file could be a proper place for this common constant?

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 26, 2018

Member

I don't know the QT code well enough to give a good recommendation here. I'll let other developers fill in :-)

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Oct 28, 2018

Member

What is 90*60? How do you get this number?

This comment has been minimized.

Copy link
@hebasto

hebasto Oct 28, 2018

Author Member

As @practicalswift mentioned above:

// Set icon state: spinning if catching up, tick otherwise
if(secs < 90*60)
{
tooltip = tr("Up to date") + QString(".<br>") + tooltip;
labelBlocksIcon->setPixmap(platformStyle->SingleColorIcon(":/icons/synced").pixmap(STATUSBAR_ICONSIZE, STATUSBAR_ICONSIZE));

This comment has been minimized.

Copy link
@hebasto

hebasto Dec 16, 2018

Author Member

Ref #1026

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14711 (Remove uses of chainActive and mapBlockIndex in wallet code by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@hebasto hebasto force-pushed the hebasto:20181023-fix13299 branch Nov 5, 2018

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Nov 5, 2018

Rebased.

@hebasto hebasto force-pushed the hebasto:20181023-fix13299 branch 2 times, most recently Nov 5, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 5, 2018

@hebasto hebasto force-pushed the hebasto:20181023-fix13299 branch 2 times, most recently Nov 5, 2018

@jonasschnelli jonasschnelli changed the title qt: Fix bug #13299 qt fix: confirmed transaction labeled "open" (#13299) Nov 13, 2018

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2018

Screenshots added.

@hebasto hebasto force-pushed the hebasto:20181023-fix13299 branch Dec 16, 2018

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Dec 16, 2018

@practicalswift @ken2812221
Your comments has been addressed.

@hebasto hebasto changed the title qt fix: confirmed transaction labeled "open" (#13299) [WIP] qt: fix confirmed transaction labeled "open" (#13299) Dec 16, 2018

@hebasto hebasto force-pushed the hebasto:20181023-fix13299 branch Dec 16, 2018

@hebasto hebasto changed the title [WIP] qt: fix confirmed transaction labeled "open" (#13299) qt: fix confirmed transaction labeled "open" (#13299) Dec 16, 2018

@hebasto hebasto force-pushed the hebasto:20181023-fix13299 branch Dec 17, 2018

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

Rebased on top of 3e21b69.

@promag
Copy link
Member

left a comment

Concept ACK.

src/qt/transactiontablemodel.cpp Outdated
if (wallet.tryGetTxStatus(rec->hash, wtx, numBlocks) && rec->statusUpdateNeeded(numBlocks)) {
rec->updateStatus(wtx, numBlocks);
if (wallet_model->wallet().tryGetTxStatus(rec->hash, wtx, numBlocks) && rec->statusUpdateNeeded(numBlocks)) {
rec->updateStatus(wtx, numBlocks, QDateTime::fromTime_t(wallet_model->node().getLastBlockTime()));

This comment has been minimized.

Copy link
@promag

promag Dec 31, 2018

Member

Should avoid evaluating QDateTime::fromTime_t(wallet_model->node().getLastBlockTime()) for each record mostly because of excessive cs_main locks.

src/qt/transactionrecord.h Outdated
@@ -8,6 +8,7 @@
#include <amount.h>
#include <uint256.h>

#include <QDateTime>

This comment has been minimized.

Copy link
@promag

promag Dec 31, 2018

Member

nit, here a forward declaration would do.

src/qt/transactionrecord.h Outdated
@@ -23,6 +24,8 @@ struct WalletTxStatus;
class TransactionStatus
{
public:
static constexpr int TIME_AGO_OF_LAST_BLOCK_IN_SECONDS = 90 * 60;

This comment has been minimized.

Copy link
@promag

promag Dec 31, 2018

Member

nit, this name is kind of weird to me.

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 2, 2019

Author Member

@promag
Good naming is hard :)
Would you mind proposing a better name?

src/qt/bitcoingui.cpp Outdated
@@ -16,6 +16,7 @@
#include <qt/optionsmodel.h>
#include <qt/platformstyle.h>
#include <qt/rpcconsole.h>
#include <qt/transactionrecord.h>

This comment has been minimized.

Copy link
@promag

promag Dec 31, 2018

Member

Maybe the constant is in the wrong place?

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 2, 2019

Author Member

Yes, this place seems inappropriate. The reason was to avoid circular dependencies.
@promag Could you point out a more appropriate place?

@hebasto hebasto force-pushed the hebasto:20181023-fix13299 branch Jan 2, 2019

Don't label transactions "Open" while catching up
Since the default `nSequence` is `0xFFFFFFFE` and locktime is enabled,
the checking `wtx.is_final` is meaningless until the syncing has
completed.

@hebasto hebasto force-pushed the hebasto:20181023-fix13299 branch to fb3ce75 Jan 2, 2019

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

@promag Thank you for your review.
Your comments have been addressed. Would you mind re-reviewing?

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

utACK fb3ce75

1 similar comment
@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

utACK fb3ce75

@laanwj laanwj merged commit fb3ce75 into bitcoin:master Jan 15, 2019

2 checks passed

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

laanwj added a commit that referenced this pull request Jan 15, 2019

Merge #14556: qt: fix confirmed transaction labeled "open" (#13299)
fb3ce75 Don't label transactions "Open" while catching up (Hennadii Stepanov)

Pull request description:

  Fix #13299.

  Since the default `nSequence` is `0xFFFFFFFE` and locktime is enabled, the checking `wtx.is_final` is meaningless until the syncing has completed (ref: #1026).

  This PR makes the wallet mark a transaction "Unconfirmed" instead of misleading "Open for NNN more blocks" when syncing after a period of being offline.

  Before this PR (with the issue):
  ![screenshot from 2018-12-12 15-56-23](https://user-images.githubusercontent.com/32963518/49874288-cdd06880-fe26-11e8-8441-f3ceb479611b.png)

  With this PR (the issue has been resolved):
  ![screenshot from 2018-12-12 15-54-41](https://user-images.githubusercontent.com/32963518/49874336-e9d40a00-fe26-11e8-8c05-9aeee2eb1bba.png)

Tree-SHA512: 358ec83b43c266a4d32a37a79dda80e80d40a2b77ad38261c84a095e613399f674aa7184805b3f6310e51ddb83ae2636b8849fcc7c4333e1b3ecbb0f70ad86d3
* Maximum gap between node time and block time used
* for the "Catching up..." mode in GUI.
*
* Ref: https://github.com/bitcoin/bitcoin/pull/1026

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jan 15, 2019

Member

nit: instead of an external link you could inline the information (I guess the fp rate?)

@hebasto hebasto deleted the hebasto:20181023-fix13299 branch Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.