wallet: Fix for confirmed column in csv export for payment to self transactions #14988

pull benthecarman wants to merge 1 commits into bitcoin:master from benthecarman:confirmations_in_csv_export changing 1 files +1 −1

benthecarman commented at 11:37 PM on December 17, 2018: contributor

Closes #3455
fanquake added the label GUI on Dec 17, 2018
benthecarman renamed this:
~~Number of confirmations added to transaction csv export~~
wallet: Number of confirmations added to transaction csv export
on Dec 18, 2018
laanwj commented at 11:00 AM on December 18, 2018: member

Concept ACK; # confirmations is more useful information than confirmed or not.
promag commented at 2:56 PM on December 18, 2018: member
I'm leaning towards NAK:
- does this fixes something? what is the point of, for instance, reading 1000 confirmations?
- column "Type" already helps identify trusted transactions
- this changes column order and while columns are "named" this can cause some failures if CSV is processed elsewhere
Maybe more useful is to append the block height, if any?
jonasschnelli commented at 7:03 AM on March 15, 2019: contributor

Tend to agree with @promag. Confirmation count is also probably invalid (old) the moment you export...

[...] which can lead to transactions sent to yourself with 0 confirmations to be labeled as confirmed

Why not fix that instead?
benthecarman force-pushed on Mar 20, 2019
benthecarman commented at 4:18 PM on March 20, 2019: contributor

Changed to just fix the confirmed column
benthecarman renamed this:
~~wallet: Number of confirmations added to transaction csv export~~
wallet: Fix for confirmed column in csv export for payment to self transactions
on Mar 20, 2019

in src/qt/transactiontablemodel.cpp:621 in c03e2c0bc6 outdated

 617 | @@ -618,7 +618,7 @@ QVariant TransactionTableModel::data(const QModelIndex &index, int role) const
 618 |              return details;
 619 |          }
 620 |      case ConfirmedRole:
 621 | -        return rec->status.countsForBalance;
 622 | +        return (rec->status.depth > 0);

promag commented at 10:57 AM on March 24, 2019:

Why ( )?

benthecarman force-pushed on Mar 24, 2019
jonasschnelli commented at 9:23 AM on October 9, 2019: contributor

Tested a bit. This fixes the unconfirmed send-to-self transactions but it will list immature mined funds as confirmed. Maybe make use of status.status instead of accessing the depth?
wallet: Fix for exported confirmation field in payment to self transactions 9760293ce6
benthecarman force-pushed on Oct 13, 2019
MarkLTZ referenced this in commit 5963a5835f on Nov 18, 2019
jonasschnelli commented at 7:17 AM on May 29, 2020: contributor

Closing due to inactivity / missing interest. Feel free to reopen.
jonasschnelli closed this on May 29, 2020
MarcoFalke commented at 12:32 PM on May 29, 2020: member

@jonasschnelli This fixes a "bug". So either all (the bug and the pull) should be closed or both should be kept open (or marked for "up for grabs").
jonasschnelli commented at 12:42 PM on May 29, 2020: contributor

Oh. I missed the point that my last comments where silently addressed.

Tested ACK 9760293ce632e09f0175368ebf0c8502ac9b10d4
jonasschnelli reopened this on May 29, 2020
jonasschnelli merged this on May 29, 2020
jonasschnelli closed this on May 29, 2020
MarcoFalke commented at 12:54 PM on May 29, 2020: member

What are the steps to test for non-fluent-gui people? Does this need release notes?
benthecarman deleted the branch on May 29, 2020
benthecarman commented at 12:58 PM on May 29, 2020: contributor

Oh. I missed the point that my last comments where silently addressed.

Tested ACK 9760293

Sorry, should have made a comment.
jonasschnelli commented at 1:00 PM on May 29, 2020: contributor

What are the steps to test for non-fluent-gui people?

Export a CSV?

Does this need release notes?

I don't think it needs an extra note (next to the PR title that usually will be listed under GUI). It's not a notable change/fix.
MarcoFalke commented at 1:23 PM on May 29, 2020: member

Thanks. Tested that this indeed seems to fix a bug.
sidhujag referenced this in commit 0abf9392f9 on May 31, 2020
PastaPastaPasta referenced this in commit 66ae78e44a on Jun 27, 2021
PastaPastaPasta referenced this in commit 82359dba95 on Jun 28, 2021
PastaPastaPasta referenced this in commit 433a5694c1 on Jun 29, 2021
PastaPastaPasta referenced this in commit 5cac6df22a on Jul 1, 2021
PastaPastaPasta referenced this in commit 8bf74c60e9 on Jul 1, 2021
PastaPastaPasta referenced this in commit 53c29d35dd on Jul 14, 2021
PastaPastaPasta referenced this in commit f8d785665e on Jul 15, 2021
Fabcien referenced this in commit 075adcca13 on Aug 25, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#3455 Confirmed is always true in export file