Factor out CWallet::nTimeSmart computation into a method. #9369

pull ryanofsky wants to merge 4 commits into bitcoin:master from ryanofsky:pr/atw-timesmart changing 3 files +129 −45
  1. ryanofsky commented at 3:10 pm on December 16, 2016: member

    No change in behavior, this change just pulls some code out of CWallet::AddToWallet that was making it very long into a separate method.

    A followup commit also adds documentation describing CWallet::GetTimeSmart and CWalletTx::nTimeSmart

  2. jonasschnelli approved
  3. jonasschnelli commented at 3:42 pm on December 16, 2016: contributor
    Makes sense. utACK 406e9759478e86d37cd4fca255841fd3291417c3
  4. in src/wallet/wallet.cpp: in 406e975947 outdated 
    3289+            }
3290+
3291+            int64_t blocktime = mapBlockIndex[wtx.hashBlock]->GetBlockTime();
3292+            nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
3293+        } else
3294+            LogPrintf("GetTimeSmart(): found %s in block %s not in index\n",
    paveljanik commented at 7:20 pm on December 16, 2016:
    You can use __func__ here.
    ryanofsky commented at 7:50 pm on December 16, 2016:
    Thanks, done.
  5. ryanofsky force-pushed on Dec 16, 2016
  6. sipa commented at 8:34 pm on December 16, 2016: member
    utACK 022267ac634b13690414252c4deba7f499f98c63
  7. in src/wallet/wallet.cpp: in 022267ac63 outdated 
    843@@ -844,51 +844,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
844         wtx.nTimeReceived = GetAdjustedTime();
845         wtx.nOrderPos = IncOrderPosNext(&walletdb);
846         wtxOrdered.insert(make_pair(wtx.nOrderPos, TxPair(&wtx, (CAccountingEntry*)0)));
847-
848-        wtx.nTimeSmart = wtx.nTimeReceived;
849-        if (!wtxIn.hashUnset())
    paveljanik commented at 8:25 am on December 17, 2016:
    The old code is testing hashUnset() of wtxIn. But the new code is checking hashUnset() of wtx. Is it the same?
    ryanofsky commented at 12:34 pm on December 17, 2016:
    Yes, the change replaces this and other uses of wtxIn with wtx. Because this code is running in the case where fInsertedNew is true, wtx at this point is just a copy of wtxIn with two extra fields set (nTimeReceived and nOrderPos)
  8. fanquake added the label Wallet on Dec 17, 2016
  10. fanquake commented at 1:42 am on December 19, 2016: member
    utACK 406e975
  11. in src/wallet/wallet.cpp: in 022267ac63 outdated 
    3255@@ -3300,6 +3256,46 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t> &mapKeyBirth) const {
3256         mapKeyBirth[it->first] = it->second->GetBlockTime() - 7200; // block times can be 2h off
3257 }
3258 
3259+unsigned int CWallet::GetTimeSmart(const CWalletTx& wtx) const
    laanwj commented at 11:33 am on December 19, 2016:
    Would it be an idea to put this on CWalletTx instead? A quick scan of the code doesn’t reveal any accesses to CWallet fields.
    ryanofsky commented at 11:46 am on December 19, 2016:
    I think this makes most sense here next to CWallet::GetKeyBirthTimes. It accesses CWallet::mapBlockIndex and scans the CWallet::wtxOrdered field.
    ryanofsky commented at 11:50 am on December 19, 2016:
    (Never mind part of comment above about mapBlockIndex, which is a global.)
  12. ryanofsky force-pushed on Jan 4, 2017
  13. JeremyRubin commented at 7:42 pm on January 13, 2017: contributor

    I think code style wise this would be improved by naming GetTimeSmart to RecomputeTimeSmart or ComputeTimeSmart because it’s not a simple accessor. If someone were to decide to call GetTimeSmart in place of accessing the cached value, the name wouldn’t imply the overhead otherwise.

    Otherwise, makes sense to me, utACK as is for 212a686.

  14. ryanofsky referenced this in commit 1c474c2531 on Jan 19, 2017
  15. ryanofsky force-pushed on Jan 19, 2017
  16. ryanofsky commented at 3:38 pm on January 19, 2017: member
    Renamed method to ComputeTimeSmart in 1c474c253170ffa7ebc50abca93bf673c0ae0f4f and squashed 1c474c253170ffa7ebc50abca93bf673c0ae0f4f -> b75a9b95a43bf88e9e977fb9ad5bcb5e3266bbd9.
  17. in src/wallet/wallet.cpp: in b75a9b95a4 outdated 
    3279@@ -3324,6 +3280,67 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t> &mapKeyBirth) const {
3280         mapKeyBirth[it->first] = it->second->GetBlockTime() - 7200; // block times can be 2h off
3281 }
3282 
3283+/**
3284+ * Compute smart timestamp for a transaction being added to the wallet.
    kallewoof commented at 1:55 am on February 10, 2017:
    I think an additional comment below this describing what a smart timestamp is would be helpful, even if the reader may deduce this from the logic section below. (It’s not super clear to me, but seems to be a way to adapt the time stamp depending on various hints deduced from the state of the transaction.)
    ryanofsky commented at 4:44 pm on February 10, 2017:
    I don’t think I understand how this should change. The comment on nTimeSmart describes what the smart timestamp is and the comment on ComputeTimeSmart describes how a smart timestamp is computed, and both comments reference each other.
    kallewoof commented at 12:59 pm on February 12, 2017:
    It just felt like I didn’t have a good grasp what it was by reading the code. You’re probably right that it’s sufficient if it’s described in the variable comment, though.
  18. in src/wallet/wallet.cpp: in b75a9b95a4 outdated 
    3333+                }
3334+            }
3335+
3336+            int64_t blocktime = mapBlockIndex[wtx.hashBlock]->GetBlockTime();
3337+            nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
3338+        } else
    kallewoof commented at 1:59 am on February 10, 2017:
    Nit / coding standard: E.g. Google coding style guide advises against if (..) { .. } else .., recommending instead if (..) { .. } else { .. }, i.e. use braces in else if braces were used in if.
    sipa commented at 2:38 am on February 10, 2017:
    Our own coding style document says the same.
    ryanofsky commented at 8:01 pm on February 10, 2017:
    Added braces in cb48cd7d720d7af466614652c1e2a56dcce068bc
  19. kallewoof approved
  20. kallewoof commented at 2:12 am on February 10, 2017: member
    Slight ACK b75a9b9 – would be useful with some test(s) to check this still works as intended, but not sure if possible.
  21. ryanofsky commented at 8:10 pm on February 10, 2017: member

    would be useful with some test(s) to check this still works as intended, but not sure if possible.

    Added tests in ed8a586d0e97c5432c6d2b46c8cf4e3d4b203a5a

  22. ryanofsky force-pushed on Feb 10, 2017
  23. ryanofsky commented at 8:22 pm on February 10, 2017: member
    Rearranged commits to put tests before refactoring cb48cd7d720d7af466614652c1e2a56dcce068bc -> 6587aa3b293b89c81dd7a4a8cc142d6e0a5670f8 (atw-timesmart.6 -> atw-timesmart.7)
  24. ryanofsky force-pushed on Mar 1, 2017
  25. ryanofsky commented at 10:44 am on March 1, 2017: member
    Rebased 6587aa3b293b89c81dd7a4a8cc142d6e0a5670f8 -> 2ffffb36e138a5e060b6b3eaada3080ee3c4de75 (atw-timesmart.7 -> atw-timesmart.8) because of conflict in wallet.h with #9108.
  26. Add tests for CWalletTx::nTimeSmart c6b82d1db5
  27. Factor out CWallet::nTimeSmart computation into a method. 
    No change in behavior, this change just pulls some code out of
CWallet::AddToWallet that was making it very long into a separate method.
    1f98abe47b
  28. Add documentation describing CWallet::nTimeSmart. 
    Most of the text comes from the 2012 Luke Dashjr <luke-jr+git@utopios.org>
c3f95ef commit message.
    6c996c2df7
  29. Clean up braces in CWallet::ComputeTimeSmart 630fc549e2
  30. ryanofsky force-pushed on Mar 3, 2017
  31. ryanofsky commented at 7:44 pm on March 3, 2017: member

    Fixed mocktime test bug in 819696357bb407196e6f01175f2d4459848c7d9b and rebased 819696357bb407196e6f01175f2d4459848c7d9b -> 79b25edd74053d7a655882d4c16bb5dba43c90a2 (pr/atw-timesmart.9 -> pr/atw-timesmart.10) because of conflicts with pwalletMain renames in #9775.

    Reduced test boilerplate in 5975523d1f647fe7de96dd16d89e363052dc38cb and squashed 5975523d1f647fe7de96dd16d89e363052dc38cb -> 630fc549e28cb33eb11df1b4f951339bf8152e4f (pr/atw-timesmart.11 -> pr/atw-timesmart.12)

  32. ryanofsky force-pushed on Mar 3, 2017
  33. laanwj merged this on Mar 7, 2017
  34. laanwj closed this on Mar 7, 2017
  35. laanwj referenced this in commit 3178b2c740 on Mar 7, 2017
  36. in src/wallet/wallet.cpp: in 630fc549e2 
    3511+            }
3512+
3513+            int64_t blocktime = mapBlockIndex[wtx.hashBlock]->GetBlockTime();
3514+            nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
3515+        } else {
3516+            LogPrintf("%s: found %s in block %s not in index\n", __func__, wtx.GetHash().ToString(), wtx.hashBlock.ToString());
    jnewbery commented at 6:09 pm on March 7, 2017:
    I mildly dislike that this log (which is about adding a transaction to the wallet and was originally in AddToWallet()) is now in ComputeTimeSmart() and will print with the name of the function, since it’s not really anything to do with computing nTimeSmart. Does it make sense to leave this log in the AddToWallet() function somehow?
  37. in src/wallet/wallet.cpp: in 630fc549e2 
    3510+                }
3511+            }
3512+
3513+            int64_t blocktime = mapBlockIndex[wtx.hashBlock]->GetBlockTime();
3514+            nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
3515+        } else {
    jnewbery commented at 6:11 pm on March 7, 2017:
    Style preference: consider not initializing nTimeSmart to wtx.nTimeReceived and only setting it here (to emphasise that nTimeSmart is only set to nTimeReceived when the transaction is received outside a block.
  38. jnewbery commented at 6:12 pm on March 7, 2017: member

    A couple of minor nits.

    utACK 630fc549e28cb33eb11df1b4f951339bf8152e4f

  39. PastaPastaPasta referenced this in commit d927895387 on Dec 30, 2018
  40. PastaPastaPasta referenced this in commit d319189479 on Dec 31, 2018
  41. PastaPastaPasta referenced this in commit 5ee059d40a on Dec 31, 2018
  42. PastaPastaPasta referenced this in commit 727b5f83f6 on Dec 31, 2018
  43. PastaPastaPasta referenced this in commit 197a272a99 on Jan 2, 2019
  44. PastaPastaPasta referenced this in commit 4d4050b731 on Jan 2, 2019
  45. PastaPastaPasta referenced this in commit 7accb8d9a3 on Jan 3, 2019
  46. PastaPastaPasta referenced this in commit 7eef8320da on Jan 21, 2019
  47. PastaPastaPasta referenced this in commit 49af2c0020 on Jan 27, 2019
  48. PastaPastaPasta referenced this in commit 649ca860c9 on Jan 29, 2019
  49. PastaPastaPasta referenced this in commit 1daa963e47 on Feb 5, 2019
  50. PastaPastaPasta referenced this in commit a5af6cc88f on Feb 5, 2019
  51. PastaPastaPasta referenced this in commit 1c08f9a5f5 on Feb 5, 2019
  52. MarcoFalke locked this on Sep 8, 2021


ryanofsky jonasschnelli paveljanik sipa fanquake laanwj JeremyRubin kallewoof jnewbery

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: 2024-12-18 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me