Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Daemon+WalletBackend timestamp adjustments #704

Closed
zpalmtree opened this issue Jan 26, 2019 · 12 comments
Closed

Daemon+WalletBackend timestamp adjustments #704

zpalmtree opened this issue Jan 26, 2019 · 12 comments

Comments

@zpalmtree
Copy link
Collaborator

@zpalmtree zpalmtree commented Jan 26, 2019

The current /getwalletsyncdata rounds a timestamp to midnight. Depending on what time of the day you start a fresh wallet, you may have no blocks to grab (we need to roll back a bit more than we currently do with the timestamp adjustment), or too many (since it's rounding to midnight which is quite far away).

We should instead just respect the timestamp given, and have a better adjust so we always sync about 100 or so blocks to be safe.

Might require a bit of work, since the timestamp fetching routines I believe hit the two DB caches, which aren't the most fun to work with.

@zpalmtree
Copy link
Collaborator Author

@zpalmtree zpalmtree commented Jun 22, 2019

Something seriously wrong with this method... converting a timestamp of 1512840544 to a height of 1602960...

@ngeojiajun
Copy link
Contributor

@ngeojiajun ngeojiajun commented Aug 30, 2019

is it ok to just remove the call to roundToMidnight(timestamp) at databaseblockchaincache.cpp#1710?

@zpalmtree
Copy link
Collaborator Author

@zpalmtree zpalmtree commented Aug 30, 2019

There is also the in memory cache that needs altering. That might fix it, but I'm not sure what other methods need the roundToMidnight - so may be a good idea to dupe the call and just update getWalletSyncData to use the new call instead to prevent breaking anything which relies on the roundToMidnight call

@ngeojiajun
Copy link
Contributor

@ngeojiajun ngeojiajun commented Aug 31, 2019

the method cannot be removed safely as it was referenced in some critical functions such as DatabaseBlockchainCache::pushBlock.

brandonlehmann added a commit to brandonlehmann/turtlecoin that referenced this issue Nov 15, 2019
brandonlehmann added a commit to brandonlehmann/turtlecoin that referenced this issue Nov 16, 2019
@brandonlehmann brandonlehmann self-assigned this Nov 16, 2019
zpalmtree added a commit to zpalmtree/turtlecoin that referenced this issue Nov 16, 2019
@brandonlehmann
Copy link
Collaborator

@brandonlehmann brandonlehmann commented Nov 28, 2019

Re-opening as #933 is being reverted.

@brandonlehmann
Copy link
Collaborator

@brandonlehmann brandonlehmann commented Nov 28, 2019

Copying from revert thread

The DatabaseBlockchainCache is created as a node syncs. As this was previously rounding this to midnight, the only indexes that exist are those that are rounded to midnight. #933 had stopped rounding to midnight and thus is passing the straight timestamp in for reading. Unfortunately, there is no index that matches this position in the past and thus we fail to actually get a response to this call and thus breaks a few of the different RPC calls that rely on getClosestTimestampBlockIndex(). While it may not be ideal that the rounding occurs to midnight forcing a resync to rebuild that state data isn't necessarily a good idea either.

@zpalmtree
Copy link
Collaborator Author

@zpalmtree zpalmtree commented Nov 28, 2019

So.. do we instead use a different method in /getwalletsyncdata and /getrawblocks instead of touching this one? Not sure if that's possible on the codepath we take.

@brandonlehmann
Copy link
Collaborator

@brandonlehmann brandonlehmann commented Nov 28, 2019

All paths other than rounding to midnight are going to require a full resync because the indexes won't exist in previous sync data. Keeping that in mind, we could proceed however we want if we are okay forcing a resync. Otherwise, we're kind of stuck.

@zpalmtree
Copy link
Collaborator Author

@zpalmtree zpalmtree commented Nov 28, 2019

I'm not sure I follow, because the patch as it stands seems to work fine for /getwalletsyncdata, it's just /getliteblocks which breaks, no?

@brandonlehmann
Copy link
Collaborator

@brandonlehmann brandonlehmann commented Nov 28, 2019

When i tested before reverting, getwalletsyncdata was also causing it to trip -- with a deep timestamp.

@zpalmtree
Copy link
Collaborator Author

@zpalmtree zpalmtree commented Nov 28, 2019

OK

@brandonlehmann
Copy link
Collaborator

@brandonlehmann brandonlehmann commented Aug 2, 2020

Im going to close this issue as (a) its staleness; and, (b) the fact it’s been open for 18 months; and, (c) it’s going to require a full resync to implement; and, (d) it’s nice to speed up wallet sync a wee bit but the return for doing all this is negligible in my opinion.

If anyone objects, feel free to comment or re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants