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

Support testnet rollback. #3443

Merged
merged 2 commits into from Aug 8, 2018

Conversation

@daira
Copy link
Contributor

daira commented Aug 2, 2018

Part of #1302. Closes #2905.

Support testnet rollback.
Signed-off-by: Daira Hopwood <daira@jacaranda.org>

@daira daira requested review from bitcartel and str4d Aug 2, 2018

@str4d str4d added this to the v2.0.0 milestone Aug 2, 2018

@str4d str4d added the chain sync label Aug 2, 2018

@daira daira added the Sapling label Aug 2, 2018

@str4d str4d added this to In Review in Zcashd Team Aug 2, 2018

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Aug 3, 2018

Does this actually close #1302 (in the general case), given #1302 (comment) ?

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 3, 2018

@daira Hmm, good point. Reducing to "part of".

uiInterface.ThreadSafeMessageBox(msg, "", CClientUIInterface::MSG_ERROR);
StartShutdown();
return false;
LogPrintf("*** First insufficiently validated block at height %d, rewind length %d\n", nHeight, rewindLength);

This comment has been minimized.

@str4d

str4d Aug 4, 2018

Contributor

If we want to print this, we should only print this if rewindLength > 0, otherwise it will just lead to startup log noise and likely user confusion (as the stars call it out in the logs). Also, it should use the same translation mechanism as the later message.

LogPrintf("*** First insufficiently validated block at height %d, rewind length %d\n", nHeight, rewindLength);
clearWitnessCaches = false;

if (rewindLength > 0) {

This comment has been minimized.

@Eirik0

Eirik0 Aug 6, 2018

Contributor

I like changing if (rewindLength > 0) to if (rewindLength > MAX_REORG_LENGTH). This branch only has side effects if rewindLength > MAX_REORG_LENGTH so we might as well check here and then remove those checks below.

This comment has been minimized.

@str4d

str4d Aug 7, 2018

Contributor

As @daira re-discovered, this check is necessary, because MAX_REORG_LENGTH is unsigned, but heights are represented as signed integers, so C++ silently coerces rewindLength = -1 to rewindLength = UINT_MAX.

This comment has been minimized.

@Eirik0

Eirik0 Aug 7, 2018

Contributor

Subtle... In that case, I think I prefer it as if (rewindLength > 0 && rewindLength > MAX_REORG_LENGTH), along with eliminating the subsequent checks of rewindLength > MAX_REORG_LENGTH.

uiInterface.ThreadSafeMessageBox(msg, "", CClientUIInterface::MSG_ERROR);
StartShutdown();
return false;
}

This comment has been minimized.

@Eirik0

Eirik0 Aug 6, 2018

Contributor

Feel free to ignore, but I think this would be a bit more readable if we wrote it as:

if (rewindLength > MAX_REORG_LENGTH) {
    // snip
    // This is true when we intend to do a long rewind.
    if (networkID == "test" && /* snip */) {
        clearWitnessCache = true;
        // Log msg
    } else {
    	// snip
    	return false;
    }
}
@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Aug 6, 2018

I synced testnet and reindexed a couple of times and did not notice anything out of the ordinary.

Regarding @str4d's comment on the startup log noise. I was not seeing this in the logs - certainly not nearly as much as I was seeing logs from UpdateTip, e.g.,

2018-08-06 21:17:37 UpdateTip: new best=00e141e8da20035b03a406ad93802cdcac010cba87341796af51029094383d45  height=3  log2_work=7  tx=4  date=2016-10-28 17:51:48 progress=0.000002  cache=0.0MiB(3tx)
2018-08-06 21:17:37 UpdateTip: new best=036b74ae369047798aa3772be049a9c06a8900b8af39f02362fb6e16ee21ef75  height=4  log2_work=7.3219281  tx=5  date=2016-10-28 17:53:04 progress=0.000003  cache=0.0MiB(4tx)

which is logged incessantly.

Also, this may be a silly question, or overly complicated to do, but would it make sense to try to update rewind_index.py to test these changes?

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 7, 2018

Regarding @str4d's comment on the startup log noise. I was not seeing this in the logs...

I was referring to when no reindex is required on startup. If a reindex is required, then yes there will be noise from UpdateTip, as expected.

Also, this may be a silly question, or overly complicated to do, but would it make sense to try to update rewind_index.py to test these changes?

It's impossible, because the RPC tests use regtest mode, while the changes solely affect testnet. Even if we worked around that, we can't recreate the exact desired hash in regtest anyway as we can't (yet) break SHA256d.

@str4d

str4d approved these changes Aug 8, 2018

Copy link
Contributor

str4d left a comment

utACK. Any cleanups can happen later.

@daira
Copy link
Contributor

daira left a comment

ACK 1375189

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 8, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 8, 2018

📌 Commit 1375189 has been approved by str4d

zkbot added a commit that referenced this pull request Aug 8, 2018

Auto merge of #3443 - daira:testnet-rollback, r=str4d
Support testnet rollback.

Part of #1302. Closes #2905.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 8, 2018

⌛️ Testing commit 1375189 with merge 40f320e...

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 8, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 40f320e to master...

@zkbot zkbot merged commit 1375189 into zcash:master Aug 8, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Review to Released (Merged in Master) Aug 8, 2018

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Aug 12, 2018

@str4d wrote:

@Eirik0 wrote:

Also, this may be a silly question, or overly complicated to do, but would it make sense to try to update rewind_index.py to test these changes?

It's impossible, because the RPC tests use regtest mode, while the changes solely affect testnet. Even if we worked around that, we can't recreate the exact desired hash in regtest anyway as we can't (yet) break SHA256d.

We could test it by adding a general mechanism to set long-rollback exceptions from tests, in the same way that we can set network upgrade activations from tests. I'm not sure it's worth the complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment