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

Relay blocks when pruning #2815

Merged
merged 2 commits into from Nov 15, 2018

Conversation

@str4d
Copy link
Contributor

str4d commented Dec 19, 2017

Cherry-picked from bitcoin/bitcoin#6148

Part of #2074.

sdaftuar added some commits May 13, 2015

Do not inv old or missing blocks when pruning
When responding to a getblocks message, only return inv's as
long as we HAVE_DATA for blocks in the chain, and only for blocks
that we aren't likely to delete in the near future.
@tedy27

This comment has been minimized.

Copy link

tedy27 commented Jan 12, 2018

Relay blocks when pruning #2815
str4d:2074-relay-pruning

@str4d str4d added this to the v1.1.1 milestone Apr 5, 2018

@str4d str4d added this to 1.1.1: Upstream Improvements in Release planning Apr 5, 2018

@mdr0id mdr0id added this to Blocked in Zcashd Team Jun 7, 2018

@str4d str4d removed this from the v1.1.1 milestone Jun 19, 2018

@str4d str4d moved this from Blocked to Review Backlog in Zcashd Team Jun 22, 2018

@Eirik0

Eirik0 approved these changes Jul 31, 2018

Copy link
Contributor

Eirik0 left a comment

utACK. Confirmed that this change set is the same as in bitcoin/bitcoin#6148.

@daira

daira approved these changes Nov 5, 2018

Copy link
Contributor

daira left a comment

utACK. Has no tests, but the upstream PR didn't either, and this has presumably been working for a long time in Bitcoin Core. It doesn't seem likely that we have any differences from Bitcoin that would block merging this.

@Eirik0 Eirik0 moved this from Review Backlog to Merge Queue in Zcashd Team Nov 7, 2018

@bitcartel bitcartel moved this from Merge Queue to In Review in Zcashd Team Nov 8, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 8, 2018

I'm moving this back to the "In Review" column.

Before we merge, It would be prudent to make sure the extended qa test pruning.py passes, both with and without this PR.

@arielgabizon

This comment has been minimized.

Copy link
Contributor

arielgabizon commented Nov 8, 2018

Hahh.. I got some error running prunning.py on this PR:
image

@arielgabizon

This comment has been minimized.

Copy link
Contributor

arielgabizon commented Nov 8, 2018

I get exactly same error on master though
Seems this has to do with the following TODO in wallet.cpp

image

i.e. the pruning test does a 288 block reorg whereas our code seems to handle only reorgs of length 100

@arielgabizon

This comment has been minimized.

Copy link
Contributor

arielgabizon commented Nov 11, 2018

At @bitcartel's suggestion. I ran the test both on current master and after merging the PR, with the constant MAX_REORG_LENGTH altered from 100 to 2500. The test then passes in both cases.

@Eirik0
Copy link
Contributor

Eirik0 left a comment

Are changes required for this? Do we need adjust MIN_BLOCKS_TO_KEEP so that it is in alignment with MAX_REORG_LENGTH?

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 12, 2018

As discussed with @arielgabizon the pruning test is not run on CI but if it were it would fail unless we add a runtime option to set the MAX_REORG_LENGTH (fairly easy) or update the test so that the reorg of 288 blocks is <100 blocks (complicated).

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 14, 2018

@Eirik0 We're not making any changes to MAX_REORG_LENGTH; it was changed locally by @arielgabizon for testing the qa test. So no changes to MIN_BLOCKS_TO_KEEP.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 14, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 14, 2018

📌 Commit ba3cdf5 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 14, 2018

⌛️ Testing commit ba3cdf5 with merge 245f5dd...

zkbot added a commit that referenced this pull request Nov 14, 2018

Auto merge of #2815 - str4d:2074-relay-pruning, r=bitcartel
Relay blocks when pruning

Cherry-picked from bitcoin/bitcoin#6148

Part of #2074.

@bitcartel bitcartel added this to the v2.0.2 milestone Nov 14, 2018

@bitcartel bitcartel moved this from In Review to Merge Queue in Zcashd Team Nov 14, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 14, 2018

💥 Test timed out

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 14, 2018

Infrastructure issue with mac builder. @zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 15, 2018

⌛️ Testing commit ba3cdf5 with merge 3b6487d...

zkbot added a commit that referenced this pull request Nov 15, 2018

Auto merge of #2815 - str4d:2074-relay-pruning, r=bitcartel
Relay blocks when pruning

Cherry-picked from bitcoin/bitcoin#6148

Part of #2074.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 15, 2018

💔 Test failed - pr-merge

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 15, 2018

Looks like a transient failure of some sort on one of the builders. Or the test is bad? (cc @str4d @Eirik0)

[ RUN      ] WalletTests.ClearNoteWitnessCache
wallet/gtest/test_wallet.cpp:1536: Failure
Value of: (bool) saplingWitnesses[1]
  Actual: true
Expected: false
wallet/gtest/test_wallet.cpp:1547: Failure
Value of: (bool) saplingWitnesses[1]
  Actual: true
Expected: false
[  FAILED  ] WalletTests.ClearNoteWitnessCache (4 ms)

The main debian 8 builder passed, so will try again.
@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 15, 2018

⌛️ Testing commit ba3cdf5 with merge 570e09e...

zkbot added a commit that referenced this pull request Nov 15, 2018

Auto merge of #2815 - str4d:2074-relay-pruning, r=bitcartel
Relay blocks when pruning

Cherry-picked from bitcoin/bitcoin#6148

Part of #2074.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 15, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 570e09e to master...

@zkbot zkbot merged commit ba3cdf5 into zcash:master Nov 15, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from Merge Queue to Released (Merged in Master) Nov 15, 2018

@str4d str4d referenced this pull request Nov 16, 2018

Open

Bitcoin Core 0.12.0 #2074

195 of 452 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment