Use block time for wallet transactions found in rescan #1159

pull sipa wants to merge 1 commits into bitcoin:master from sipa:rescantime changing 2 files +8 −5

sipa commented at 4:26 PM on April 28, 2012: member

Instead of rescan time.
Use block time for wallet transactions found in rescan 30e2bfdbe9
luke-jr commented at 5:49 PM on April 28, 2012: member

During normal 24/7 operation, the first-seen time is still used, right?
sipa commented at 8:59 PM on April 28, 2012: member

the behaviour with this patch would be: transactions first seen in a "tx" message or self-generated get clock time, transactions first seen in a block or found by rescanning: block time.
luke-jr commented at 9:22 PM on April 28, 2012: member

It's problematic if transactions can be "timed" older than the most recent wallet transaction, or in the future. Doing so would make "listtransactions" out of order (or worse, reordered).
gmaxwell commented at 9:58 PM on April 28, 2012: contributor

In general I prefer this behavior to the current one... though txn appearing 'out of order' in the transaction history is unfortunate. ... but traditional banking sites do that all the time and people seem to survive.
luke-jr commented at 10:55 PM on April 28, 2012: member

I'd mind less if they actually appeared out of order, but I'm pretty sure they're sorted by time, so they'll reorder. :/

I care less, if listtransactions shows all 3 times (seen, block, and "best guess") and sorts by seen time...
laanwj commented at 9:18 AM on April 29, 2012: member

I think it's fine for the JSON call to show all the different times, as people may need them for different purposes.

On the other hand, @Sipa's change is great for the GUI.

ghost commented at 5:26 AM on May 9, 2012: none

Why not do it this way, without changing function args list:

diff --git a/src/wallet.cpp b/src/wallet.cpp
index 9989098..defab00 100644
--- a/src/wallet.cpp
+++ b/src/wallet.cpp
@@ -322,7 +322,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn)
         CWalletTx& wtx = (*ret.first).second;
         wtx.BindWallet(this);
         bool fInsertedNew = ret.second;
-        if (fInsertedNew)
+        if (fInsertedNew && wtx.nTimeReceived == 0)
             wtx.nTimeReceived = GetAdjustedTime();

         bool fUpdated = false;
@@ -397,6 +397,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pbl
         if (fExisted || IsMine(tx) || IsFromMe(tx))
         {
             CWalletTx wtx(this,tx);
+            wtx.nTimeReceived = pblock->GetBlockTime();
             // Get merkle branch if transaction was found in a block
             if (pblock)
                 wtx.SetMerkleBranch(pblock);

gmaxwell commented at 2:43 AM on May 18, 2012: contributor

Can we get some more discussion here? Agreement on this would be nice.
luke-jr commented at 2:50 AM on May 18, 2012: member
We discussed this a bit back in December. I think having all 3 times (received, block, and "smart") in JSON-RPC and just the smart times in Bitcoin-Qt is the best solution.
```
"time": smart timestamp,
"receivetime": timestamp,
"blocktime": timestamp,
```
gmaxwell commented at 4:29 AM on May 18, 2012: contributor

Thats sounds good to me— perhaps a later commit can make a tooltip show the three times in the gui? I assume that recievetime would be a sent time for txn you sent (being that you received them the same instant)?
luke-jr commented at 5:15 AM on May 18, 2012: member

Yes, the smart time would also logically be the sent time as well.
laanwj commented at 6:30 AM on May 18, 2012: member

Right, we can put all the times in the "transaction details" window.
jgarzik commented at 3:41 PM on August 1, 2012: contributor

I'm officially -ENOCARE. Code appears correct to my minimal scan. @gavinandresen ? @gmaxwell based on your comments, it sounds like you ACK this code?
gavinandresen commented at 3:53 PM on August 1, 2012: contributor

ACK. Looks obviously better than the current behavior.
luke-jr commented at 3:55 PM on August 1, 2012: member

This is NACK since it breaks listtransactions. #1393
sipa commented at 3:57 PM on August 1, 2012: member

I'm very unsure myself whether I want this merged. Luke's version is more complex but probably behaves as expected in more use cases.

I'm not sure either should be merged without some testing...
gmaxwell closed this on Aug 23, 2012
gmaxwell commented at 8:14 PM on August 23, 2012: contributor

(Obsoleted by Luke's version.)
lateminer referenced this in commit fcdf663b23 on Jan 22, 2019
lateminer referenced this in commit 1ae53d0a2a on Dec 25, 2019
dexX7 referenced this in commit f473a31a7d on Aug 20, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Milestone
0.7.0

Linked (view graph)

#1393 Refactor transaction/accounting time #16904 Bus error (core dumped) after "Using obfuscation key for" log line