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

Don't allow Sprout-to-Sapling migration when syncing during IBD or after wake from sleep. #3995

Merged

Conversation

@bitcartel
Copy link
Contributor

commented May 8, 2019

Prevent migration transactions from being created in response to incoming blocks when a node launches and syncs (the initial block download phase) and when a node wakes from sleep/hibernation and starts syncing old blocks rapidly.

@bitcartel bitcartel added this to Needs Prioritization in Arborist Team via automation May 8, 2019

@bitcartel bitcartel requested review from Eirik0, daira and LarryRuane May 8, 2019

@bitcartel bitcartel added this to the v2.0.5 milestone May 9, 2019

@tromer

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

This doesn't address the same migration-during-syncup problem that would occur if a zcashd node is suspended and later resumed.

This may happen when the user suspends/hibernates a laptop running zcashd, and likewise with suspended virtual machines and maybe also when suspending zcashd via Ctrl-Z.

@tromer

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Ditto for prolonged loss of network connectivity, even is zcashd was still running.

@tromer

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

One solution approach is to extend the meaning of IsInitialBlockDownload() (or add a new flag) to also capture "I just woke up from a suspend" situations, as well as "I just got my network connectivity restored".
For example, make this flag be true also if:

  • No blocks were seen for the last 25 minutes (according to the node's local clock)
  • The last 10 blocks seen were received in less than 5 minutes (according to the node's local clock)

Calibrate these numbers so the probability of false-positives for a running-and-connected node is tiny, but reconnection/resume will be detected with certainty if it's long enough to be a concern for the anchor selection, tx expirty and migration heuristics.

While at it, it would be nice to get this status information on the metrics screen and via RPC (see #2206).

@daira

daira approved these changes May 9, 2019

Copy link
Contributor

left a comment

utACK. This has no tests; how feasible would it be to test it? I don't remember seeing other tests that distinguish initial block download.

@daira

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Remember that 500 blocks is ~20.83 hours. So we have plenty of margin to detect the problem, even in the presence of severe clock skew. If a block would otherwise trigger a migration batch but is more than, say, three hours old by the node's local clock, then it can't be the most recent trigger block, and should be ignored. I think that is the simplest rule that will solve the problem.

I chose three hours because it is difficult for an adversary to postdate block timestamps by more than two hours. From section 7.5 of the protocol spec:

In addition, a full validator MUST NOT accept blocks with nTime more than two hours in the future according to its clock. This is not strictly a consensus rule because it is nondeterministic, and clock time varies between nodes. Also note that a block that is rejected by this rule at a given point in time may later be accepted.

So, an adversary cannot (easily) force the "more recent than three hours old" check to pass by postdating block timestamps.

@tromer

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@daira's idea is simple and effective. Though not perfect (e.g., might be fooled by extreme timezone errors: consider a user that crosses the International Date Line and adjusts their laptop's clock instead of its timezone).

In any case, for a good measure let's combine the wallclock check with the IsInitialBlockDownload() check. (Or maybe just put the wallclock check inside IsInitialBlockDownload()? Need to see where else it's used...)

@bitcartel bitcartel force-pushed the bitcartel:no_migration_during_block_download_phase branch from 3ff191a to a5024e9 May 9, 2019

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Pushed an update with @daira's proposal.

@mms710 mms710 moved this from Needs Prioritization to Bugs/Security Issues/TechDebt Backlog in Arborist Team May 9, 2019

@mms710 mms710 moved this from Bugs/Security Issues/TechDebt Backlog to Current Sprint in Arborist Team May 9, 2019

@mms710 mms710 moved this from Current Sprint to In Review in Arborist Team May 9, 2019

@bitcartel bitcartel changed the title Do not allow Sprout-to-Sapling migration during initial block download. Don't allow Sprout-to-Sapling migration when syncing during IBD or after wake from sleep. May 9, 2019

@LarryRuane
Copy link
Contributor

left a comment

ACK, I tested manually by adding a debug print when we hit this condition, and ran zcashd --reindex and the debug print happens very quickly many times, for example:

2019-05-09 16:27:33 XXX migration IsInitialBlockDownload
2019-05-09 16:27:33 UpdateTip: new best=0002f58002c8f2338f614e4bd77965c0625f082760734ceab7e697e5d2dd7548  height=840  log2_work=22.743451  tx=841  date=2016-10-29 17:10:07 progress=0.000284  cache=0.2MiB(840tx)

@bitcartel bitcartel force-pushed the bitcartel:no_migration_during_block_download_phase branch from a5024e9 to 6921c81 May 9, 2019

@LarryRuane

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Retesting commit hash 6921c81, added debug print to show the values of the variables involved, all looks well at least during reindex (many occurrences of pairs of lines like this):

2019-05-09 16:39:40 UpdateTip: new best=000007af59a559c228f72ea76b0092fea99141823b0bb4d59546d83951aaee84  height=440  log2_work=20.592333  tx=441  date=2016-10-29 01:09:30 progress=0.000149  cache=0.1MiB(440tx)
2019-05-09 16:39:40 XXX IsInitialBlockDownload: 1 pblock->GetBlockTime(): 1477703370 GetAdjustedTime(): 1557419980 GetAdjustedTime() - 3 * 60 * 60: 1557409180

time 1477703370 is Fri Oct 28 19:09:30 2016 and time 1557419980 is Thu May 9 10:39:40 2019

Perhaps it would help if I show the debug print:

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index bde0eb9d1..414b4cde0 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -583,6 +583,7 @@ void CWallet::ChainTip(const CBlockIndex *pindex,
         ChainTipAdded(pindex, pblock, sproutTree, saplingTree);
         // Prevent migration transactions from being created when node is syncing after launch,
         // and also when node wakes up from suspension/hibernation and incoming blocks are old.
+        LogPrintf("XXX IsInitialBlockDownload: %d pblock->GetBlockTime(): %d GetAdjustedTime(): %d GetAdjustedTime() - 3 * 60 * 60: %d\n", IsInitialBlockDownload(), pblock->GetBlockTime(), GetAdjustedTime(), GetAdjustedTime() - 3 * 60 * 60);
         if (!IsInitialBlockDownload() &&
             pblock->GetBlockTime() > GetAdjustedTime() - 3 * 60 * 60)
         {
@Eirik0

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

ACK. I tested that this we not try to create migration transactions during the IBD, and that they are still being created after we are synced up. I did not test logic to do with the time adjustment, but that code looks good to me.

@Eirik0

Eirik0 approved these changes May 9, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

📌 Commit 6921c81 has been approved by Eirik0

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

⌛️ Testing commit 6921c81 with merge e97621a...

zkbot added a commit that referenced this pull request May 9, 2019

Auto merge of #3995 - bitcartel:no_migration_during_block_download_ph…
…ase, r=Eirik0

Don't allow Sprout-to-Sapling migration when syncing during IBD or after wake from sleep.

Prevent migration transactions from being created in response to incoming blocks when a node launches and syncs (the initial block download phase) and when a node wakes from sleep/hibernation and starts syncing old blocks rapidly.
@daira

daira approved these changes May 9, 2019

Copy link
Contributor

left a comment

utACK. We decided that manual testing was sufficient for the release, but please file a ticket to add an automated test.

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

☀️ Test successful - pr-merge
Approved by: Eirik0
Pushing e97621a to master...

@zkbot zkbot merged commit 6921c81 into zcash:master May 9, 2019

1 check passed

homu Test successful
Details

Arborist Team automation moved this from In Review to Released (Merged in Master) May 9, 2019

@Eirik0

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

In adding the 3 hour window this PR makes it possible to try to send migration transactions ~70 blocks after the 500th block. If a node is suspended after generating migration transactions, but before propagating them to the network, and then resumes in less than 3 hours from when the 500th block was mined on the main chain, this situation will occur. #4005 makes it no longer possible to send expired or soon-to-expire transactions within this window.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.