During normal 24/7 operation, the first-seen time is still used, right?

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.

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).

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.

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...

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.

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);

Can we get some more discussion here? Agreement on this would be nice.

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,

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)?

Yes, the smart time would also logically be the sent time as well.

Right, we can put all the times in the "transaction details" window.

I'm officially -ENOCARE. Code appears correct to my minimal scan. @gavinandresen ?

@gmaxwell based on your comments, it sounds like you ACK this code?

ACK. Looks obviously better than the current behavior.

This is NACK since it breaks listtransactions. #1393

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...

(Obsoleted by Luke's version.)