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

ryanofsky · 2016-12-16T15:10:50Z

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

jonasschnelli

Makes sense.
utACK 406e975

paveljanik · 2016-12-16T19:20:59Z

src/wallet/wallet.cpp

+            int64_t blocktime = mapBlockIndex[wtx.hashBlock]->GetBlockTime();
+            nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
+        } else
+            LogPrintf("GetTimeSmart(): found %s in block %s not in index\n",


You can use __func__ here.

Thanks, done.

sipa · 2016-12-16T20:34:06Z

utACK 022267a

paveljanik · 2016-12-17T08:25:14Z

src/wallet/wallet.cpp

        wtxOrdered.insert(make_pair(wtx.nOrderPos, TxPair(&wtx, (CAccountingEntry*)0)));
-
-        wtx.nTimeSmart = wtx.nTimeReceived;
-        if (!wtxIn.hashUnset())


The old code is testing hashUnset() of wtxIn. But the new code is checking hashUnset() of wtx. Is it the same?

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)

paveljanik · 2016-12-17T16:43:28Z

ACK 022267a

fanquake · 2016-12-19T01:42:35Z

utACK 406e975

laanwj · 2016-12-19T11:33:22Z

src/wallet/wallet.cpp

        mapKeyBirth[it->first] = it->second->GetBlockTime() - 7200; // block times can be 2h off
 }

+unsigned int CWallet::GetTimeSmart(const CWalletTx& wtx) const


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.

I think this makes most sense here next to CWallet::GetKeyBirthTimes. It ~~accesses CWallet::mapBlockIndex and~~ scans the CWallet::wtxOrdered field.

(Never mind part of comment above about mapBlockIndex, which is a global.)

JeremyRubin · 2017-01-13T19:42:11Z

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.

@JeremyRubin

Rename GetTimeSmart method to ComputeTimeSmart as suggested by @JeremyRubin in bitcoin#9369 (comment)

ryanofsky · 2017-01-19T15:38:07Z

Renamed method to ComputeTimeSmart in 1c474c2 and squashed 1c474c2 -> b75a9b9.

kallewoof

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

kallewoof · 2017-02-10T01:55:06Z

src/wallet/wallet.cpp

 }

+/**
+ * Compute smart timestamp for a transaction being added to the wallet.


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

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.

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.

kallewoof · 2017-02-10T01:59:27Z

src/wallet/wallet.cpp

+
+            int64_t blocktime = mapBlockIndex[wtx.hashBlock]->GetBlockTime();
+            nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
+        } else


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.

Our own coding style document says the same.

Added braces in cb48cd7

ryanofsky

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

Added tests in ed8a586

ryanofsky · 2017-02-10T16:44:22Z

src/wallet/wallet.cpp

 }

+/**
+ * Compute smart timestamp for a transaction being added to the wallet.


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.

ryanofsky · 2017-02-10T20:01:56Z

src/wallet/wallet.cpp

+
+            int64_t blocktime = mapBlockIndex[wtx.hashBlock]->GetBlockTime();
+            nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
+        } else


Added braces in cb48cd7

ryanofsky · 2017-02-10T20:22:26Z

Rearranged commits to put tests before refactoring cb48cd7 -> 6587aa3 (atw-timesmart.6 -> atw-timesmart.7)

ryanofsky · 2017-03-01T10:44:08Z

Rebased 6587aa3 -> 2ffffb3 (atw-timesmart.7 -> atw-timesmart.8) because of conflict in wallet.h with #9108.

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

Most of the text comes from the 2012 Luke Dashjr <[email protected]> c3f95ef commit message.

ryanofsky · 2017-03-03T19:44:01Z

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

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

630fc54 Clean up braces in CWallet::ComputeTimeSmart (Russell Yanofsky) 6c996c2 Add documentation describing CWallet::nTimeSmart. (Russell Yanofsky) 1f98abe Factor out CWallet::nTimeSmart computation into a method. (Russell Yanofsky) c6b82d1 Add tests for CWalletTx::nTimeSmart (Russell Yanofsky) Tree-SHA512: 457a30251e572cf20dac0198af1a94128d269b1e0ce6605a213d56fc14d85c84a0a494e3dcbb18c201c4f39e6f7b000bd9cb6f283930d8452e4bb93ba406f8d4

jnewbery

A couple of minor nits.

utACK 630fc54

jnewbery · 2017-03-07T18:09:46Z

src/wallet/wallet.cpp

+            int64_t blocktime = mapBlockIndex[wtx.hashBlock]->GetBlockTime();
+            nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
+        } else {
+            LogPrintf("%s: found %s in block %s not in index\n", __func__, wtx.GetHash().ToString(), wtx.hashBlock.ToString());


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?

jnewbery · 2017-03-07T18:11:24Z

src/wallet/wallet.cpp

+
+            int64_t blocktime = mapBlockIndex[wtx.hashBlock]->GetBlockTime();
+            nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
+        } else {


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.

… method. 630fc54 Clean up braces in CWallet::ComputeTimeSmart (Russell Yanofsky) 6c996c2 Add documentation describing CWallet::nTimeSmart. (Russell Yanofsky) 1f98abe Factor out CWallet::nTimeSmart computation into a method. (Russell Yanofsky) c6b82d1 Add tests for CWalletTx::nTimeSmart (Russell Yanofsky) Tree-SHA512: 457a30251e572cf20dac0198af1a94128d269b1e0ce6605a213d56fc14d85c84a0a494e3dcbb18c201c4f39e6f7b000bd9cb6f283930d8452e4bb93ba406f8d4

jonasschnelli approved these changes Dec 16, 2016

View reviewed changes

paveljanik reviewed Dec 16, 2016

View reviewed changes

ryanofsky force-pushed the pr/atw-timesmart branch from 406e975 to 022267a Compare December 16, 2016 19:50

paveljanik reviewed Dec 17, 2016

View reviewed changes

fanquake added the Wallet label Dec 17, 2016

laanwj reviewed Dec 19, 2016

View reviewed changes

ryanofsky force-pushed the pr/atw-timesmart branch from a283b62 to 212a686 Compare January 4, 2017 21:18

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jan 19, 2017

fixup! Factor out CWallet::nTimeSmart computation into a method.

1c474c2

Rename GetTimeSmart method to ComputeTimeSmart as suggested by @JeremyRubin in bitcoin#9369 (comment)

ryanofsky force-pushed the pr/atw-timesmart branch from 1c474c2 to b75a9b9 Compare January 19, 2017 15:36

ryanofsky mentioned this pull request Feb 3, 2017

Remove CWalletTx merging logic from AddToWallet #9381

Merged

kallewoof approved these changes Feb 10, 2017

View reviewed changes

ryanofsky commented Feb 10, 2017

View reviewed changes

ryanofsky force-pushed the pr/atw-timesmart branch from cb48cd7 to 6587aa3 Compare February 10, 2017 20:21

ryanofsky force-pushed the pr/atw-timesmart branch from 6587aa3 to 2ffffb3 Compare March 1, 2017 10:42

ryanofsky added 4 commits March 3, 2017 11:22

Add tests for CWalletTx::nTimeSmart

c6b82d1

Factor out CWallet::nTimeSmart computation into a method.

1f98abe

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

Add documentation describing CWallet::nTimeSmart.

6c996c2

Most of the text comes from the 2012 Luke Dashjr <[email protected]> c3f95ef commit message.

Clean up braces in CWallet::ComputeTimeSmart

630fc54

ryanofsky force-pushed the pr/atw-timesmart branch from 8196963 to 79b25ed Compare March 3, 2017 19:42

ryanofsky force-pushed the pr/atw-timesmart branch from 5975523 to 630fc54 Compare March 3, 2017 22:04

laanwj merged commit 630fc54 into bitcoin:master Mar 7, 2017

jnewbery reviewed Mar 7, 2017

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

ryanofsky commented Dec 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Dec 16, 2016

Uh oh!

paveljanik Dec 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Dec 17, 2016

Uh oh!

fanquake commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyRubin commented Jan 13, 2017

Uh oh!

ryanofsky commented Jan 19, 2017

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Feb 10, 2017

Uh oh!

ryanofsky commented Mar 1, 2017

Uh oh!

ryanofsky commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

ryanofsky commented Dec 16, 2016 •

edited

Loading

paveljanik Dec 17, 2016 •

edited

Loading

ryanofsky Dec 19, 2016 •

edited

Loading

ryanofsky Feb 10, 2017 •

edited

Loading

ryanofsky Feb 10, 2017 •

edited

Loading

ryanofsky commented Mar 3, 2017 •

edited

Loading