wallet: Remove CWalletTx::fTimeReceivedIsTxTime #32768

pull achow101 wants to merge 2 commits into bitcoin:master from achow101:rm-timereceivedistxtime changing 3 files +9 −36
  1. achow101 commented at 9:28 pm on June 17, 2025: member
    CWalletTx::fTimeReceivedIsTxTime is no longer used and can be removed. This additionally allows the removal of the upgraded_txs loop in LoadWallet.
  2. DrahtBot commented at 9:28 pm on June 17, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32768.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko, rkrux, Eunovo, davidgumberg, w0xlt, PeterWrighten

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32763 (wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members by achow101)
    • #32685 (wallet: Allow read-only database access for info and dump commands by PeterWrighten)
    • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
    • #27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)

    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.

  3. DrahtBot added the label Wallet on Jun 17, 2025
  4. DrahtBot added the label CI failed on Jun 17, 2025
  5. achow101 force-pushed on Jun 18, 2025
  6. wallet: Remove unused fTimeReceivedIsTxTime 
    fTimeReceivedIsTxTime has been unused unused since 0.3.24
    c668033709
  7. walletdb: Remove unused upgraded_txs 9eb2c82e7c
  8. achow101 force-pushed on Jun 18, 2025
  9. DrahtBot removed the label CI failed on Jun 18, 2025
  10. maflcko commented at 7:39 am on June 20, 2025: member
    lgtm ACK 9eb2c82e7c911a066781d67e6846cf6bbbaba6e9
  11. in src/wallet/wallet.cpp:2230 in c668033709 outdated 
    2225@@ -2227,7 +2226,6 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
2226         CHECK_NONFATAL(wtx.vOrderForm.empty());
2227         wtx.mapValue = std::move(mapValue);
2228         wtx.vOrderForm = std::move(orderForm);
2229-        wtx.fTimeReceivedIsTxTime = true;
    rkrux commented at 10:52 am on June 20, 2025:
    I find it slightly weird that boolean was used to set an unsigned int.
  12. in src/wallet/transaction.h:279 in c668033709 outdated 
    276+        bool dummy_bool = false; //!< Used to be fFromMe, and fSpent
277+        uint32_t dummy_int = 0; // Used to be fTimeReceivedIsTxTime
278         uint256 serializedHash = TxStateSerializedBlockHash(m_state);
279         int serializedIndex = TxStateSerializedIndex(m_state);
280-        s << TX_WITH_WITNESS(tx) << serializedHash << dummy_vector1 << serializedIndex << dummy_vector2 << mapValueCopy << vOrderForm << fTimeReceivedIsTxTime << nTimeReceived << dummy_bool << dummy_bool;
281+        s << TX_WITH_WITNESS(tx) << serializedHash << dummy_vector1 << serializedIndex << dummy_vector2 << mapValueCopy << vOrderForm << dummy_int << nTimeReceived << dummy_bool << dummy_bool;
    rkrux commented at 10:53 am on June 20, 2025:
    Is there a possibility that sometime in the future we stop serialising and unserialising dummy values?
    achow101 commented at 7:31 pm on June 20, 2025:
    Not with the way that stuff is serialized into the wallet.
    davidgumberg commented at 8:51 pm on June 20, 2025:
    I think if we ever started using the SQLite database as a relational database instead of as a key-value store as @achow101 has suggestd in the past, adding and removing fields will be neater.
  13. in src/wallet/walletdb.cpp:1033 in c668033709 outdated 
    1029@@ -1030,27 +1030,6 @@ static DBErrors LoadTxRecords(CWallet* pwallet, DatabaseBatch& batch, std::vecto
1030             if (wtx.GetHash() != hash)
1031                 return false;
1032 
1033-            // Undo serialize changes in 31600
    rkrux commented at 10:58 am on June 20, 2025:
    Interesting that v0.3.16 is not present in the tags, might have been buggy for which this undoing was needed.
  14. in src/wallet/walletdb.cpp:1194 in 9eb2c82e7c 
    1187@@ -1189,9 +1188,6 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
1188     if (result != DBErrors::LOAD_OK)
1189         return result;
1190 
1191-    for (const Txid& hash : upgraded_txs)
1192-        WriteTx(pwallet->mapWallet.at(hash));
1193-
    rkrux commented at 10:59 am on June 20, 2025:
    Looks fine to me to remove because fTimeReceivedIsTxTime is not really used in the wallet codebase at present, so I see no point in writing this upgraded transaction back.
  15. rkrux commented at 10:59 am on June 20, 2025: contributor
    ACK 9eb2c82e7c911a066781d67e6846cf6bbbaba6e9
  17. davidgumberg commented at 10:20 pm on June 20, 2025: contributor

    ACK https://github.com/bitcoin/bitcoin/commit/9eb2c82e7c911a066781d67e6846cf6bbbaba6e9

    The fTimeReceivedIsTxTime once indicated whether a transactions “time” was the time the wallet heard about the transaction, or the time that the transaction was included in a block.

    It was set to true for transactions created by the wallet, where presumably we care more about what time we made the transaction then what time we heard about it in a block, but for other transaction’s the time that we “receive” the transaction may be much later (e.g. IBD) than the time it was included in a block, so we don’t want the transaction’s “time” to be nTimeReceived .:

    https://github.com/bitcoin/bitcoin/blob/4405b78d6059e536c36974088a8ed4d9f0f29898/main.cpp#L263-L279 1

    This field was mistakenly an unsigned int rather than a char/bool:

    https://github.com/bitcoin/bitcoin/blob/4405b78d6059e536c36974088a8ed4d9f0f29898/main.h#L678

    A later commit (e4ff4e6898d378b1a3e83791034a7af455fde3ab), that AFAICT predates the practice of using dummy types for deprecated fields in serialization, attempted to “fix” this inefficiency of using 4 bytes to represent a bool. Since the commit was adding an nVersion field to the serialized transaction data anyways, and nVersion is 4 bytes, it reused the fTimeReceivedIsTxTime field as the nVersion field, and changed fTimeReceivedIsTxTime to a bool and moved it to the end of tx serialization:

     0diff --git a/main.h b/main.h
 1index 7d1145b5ff..dda5f730c7 100644
 2
 3-        READWRITE(fTimeReceivedIsTxTime);
 4+        READWRITE(nVersion);
 5+        if (fRead && nVersion < 100)
 6+            const_cast<CWalletTx*>(this)->fTimeReceivedIsTxTime = nVersion;
 7         READWRITE(nTimeReceived);
 8         READWRITE(fFromMe);
 9         READWRITE(fSpent);
10+        if (nVersion >= 31404)
11+        {
12+            READWRITE(fTimeReceivedIsTxTime);
13+            READWRITE(fUnused);
14+            READWRITE(strFromAccount);
15+        }
16     )

    This broke transaction times for wallets that were upgraded and then loaded into an older bitcoin client, since these wallets would have giant version numbers like 31404 in the fTimeReceivedIsTxTime for every transaction, and fTimeReceivedIsTxTime would always be true.

    A later commit (https://github.com/bitcoin/bitcoin/commit/865c3a23832e36d50cb873d38c976032b027b5d3) reverted the above change to fix this upgrade-then-downgrade bug, but it had to include a special case for fixing transactions that had very large fTimeReceivedIsTxTime that matched the version numbers that were affected by this bug

    https://github.com/bitcoin/bitcoin/blob/482d2553764ef1fa3f9c06389b2e13a735d86eda/src/wallet/walletdb.cpp#L1033-L1053

    And Bitcoin Core retains this special-case handling today, even though, for a long time, even before the introduction of smart time in #1393, GetTxTime() has not used fTimeReceivedIsTxTime (471426fb3b2c2fa37640c0381):

    https://github.com/bitcoin/bitcoin/blob/471426fb3b2c2fa37640c03819c4f7be69ba8301/src/wallet.cpp#L354-L358

    1. This snippet is from the initial git commit: 4405b78d6059e536c36 ↩︎

  21. glozow merged this on Jun 25, 2025
  22. glozow closed this on Jun 25, 2025


achow101 DrahtBot maflcko rkrux davidgumberg Eunovo w0xlt PeterWrighten

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-07-25 18:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me