During fast/light sync, check seal on a small subset of headers #1010

gsalgado · 2018-07-12T10:39:10Z

For every header batch, pick a random subset and check the seal only on those.

Before this change: Imported 192 headers in 1.33 seconds, new head: #3583663

After this change: Imported 192 headers in 0.53 seconds, new head: #3585558

Closes: #932

gsalgado · 2018-07-12T13:50:27Z

setup.py

@@ -46,7 +46,7 @@
    ],
    'test': [
        "hypothesis==3.44.26",
-        "pytest~=3.2",
+        "pytest~=3.3",


This is needed because we now have some tests that use the caplog fixture, which is not available in 3.2

pipermerriam · 2018-07-12T18:07:03Z

evm/chains/base.py

+        if seal_check_frequency == 1:
+            headers_to_check_seal = all_indices
+        else:
+            headers_to_check_seal = random.sample(


I went as far as writing up a big comment with a secure version of random.sample implemented using secrets.choice. But then I deleted it because it felt a bit overly paranoid/tinfoil-hat. But then I still wanted to raise the issue.

I respect that there is very little likelihood that we're exposing attack surface by using random.sample here, but given the potential severity of a security exploit in this type of software, how would you feel about replacing this with such a small utility that uses secure sources of randomness. My basic implementation is below:

def samp(pop: Set, size: int) -> Set: # could probably use a generic for the items in the set `Set[G]` if size >= len(pop): return pop rejects = set() for _ in range(len(pop) - size): # tuple cast is needed to use `secrets.choice` rejects.add(secrets.choice(tuple(pop.difference(rejects)))) return pop.difference(rejects)

I initially wrote this using secrets.choice(), but changed it to random because we need to be python3.5 compatible as this is in the evm package, no?

You are correct it seems. 👍

pipermerriam · 2018-07-12T18:08:36Z

evm/chains/base.py

            if header.parent_hash != parent.hash:
                raise ValidationError(
                    "Invalid header chain; {} has parent {}, but expected {}".format(
                        header, header.parent_hash, parent.hash))
            vm_class = self.get_vm_class_for_block_number(header.block_number)
-            vm_class.validate_header(header, parent)
+            if i in headers_to_check_seal:


semantic nitpick

When I see inclusion checks I default to wanting the population object to be a set type. Thoughts on casting headers_to_check_seal to a set?

Makes sense

pipermerriam · 2018-07-12T18:09:30Z

evm/chains/base.py

+            if i in headers_to_check_seal:
+                check_seal = True
+            else:
+                check_seal = False


Maybe just embed the call to vm_class.validate_header(header, parent, check_seal=True) here and get rid of the check_seal variable?

Well, validate_header() does more than checking the seal, and we still want to perform those for every header -- it's only the seal check that we want to skip sometimes

Oh, duh, now I understand what you mean. Will do that

carver · 2018-07-12T18:01:46Z

p2p/constants.py

+# (https://github.com/ethereum/go-ethereum/pull/1889#issue-47241762), but in order to err on the
+# side of caution, we use 192/4 to ensure we check 4 headers out of every batch (of length
+# MAX_HEADERS_FETCH).
+FAST_SYNC_SEAL_CHECK_FREQUENCY = 192 // 4


Maybe it's clearer to specify the period here, like:

# check one in 4 headers FAST_SYNC_SEAL_CHECK_PERIOD = 4

Then, we aren't assuming this value is used with 192 headers, either.

AIUI, geth checks 1 in 100, and as I mentioned below we're doing 1 in 48. Why would we do 1 in 4 instead?

Sorry, I see now that the 'frequency` name was throwing me off. So I think my only issue with this is that it should be renamed to period. Frequency would be 1/period, ~0.02.

Edit: frequency weakly implies that the checks are cyclical instead of random, and period implies that even more strongly. Maybe the best solution is to just invert the number and keep it as frequency. Or come up with an alternative name for frequency like chance or probability or likelihood

What about seal_check_random_sample_rate?

sample_size = len(all_indices) // seal_check_random_sample_rate headers_to_check_seal = set(random.sample(all_indices, sample_size))

carver · 2018-07-12T18:04:29Z

evm/chains/base.py

+            headers_to_check_seal = all_indices
+        else:
+            headers_to_check_seal = random.sample(
+                all_indices, len(all_indices) // seal_check_frequency)


This looks like it's written as if the seal_check constant is a period. Otherwise, 192 // (192/4) would only check 4 headers total in a chain of 192.

Yes, that's what I intended. In geth they check every 100-th header, and we're doing every 48th

carver · 2018-07-12T18:09:56Z

evm/vm/base.py

@@ -712,7 +713,8 @@ def validate_block(self, block):
            )

    @classmethod
-    def validate_header(cls, header: BlockHeader, parent_header: BlockHeader) -> None:
+    def validate_header(
+            cls, header: BlockHeader, parent_header: BlockHeader, check_seal: bool) -> None:


I think it's okay to default check_seal to True, which should remove some of the boilerplate changes.

carver

~~I read the code as choosing too few headers.~~

I misread the comment about geth. 👍 to the logic.

carver

👍 with a rename from frequency.

Other options would be:

FAST_SYNC_SEAL_CHECK_PERIOD
FAST_SYNC_SEAL_CHECK_INVERSE_FREQUENCY
FAST_SYNC_SEAL_CHECK_ONE_IN_N
FAST_SYNC_SEAL_CHECK_EVERY
FAST_SYNC_SEAL_CHECK_SKIP (although this is semantically off-by-one)

OR to invert the value to be a frequency (plus the related logic of course), which is easier to name.

Edit: frequency weakly implies that the checks are cyclical instead of random, and period implies that even more strongly. Maybe the best solution is to just invert the number and keep it as frequency. We can drop the cyclical implication entirely with an alternative name like chance or probability or likelihood.

For every header batch, pick a random subset and check the seal only on those.

gsalgado mentioned this pull request Jul 12, 2018

Profile and optimize new VM.validate_header() #910

Closed

gsalgado force-pushed the issue-932 branch 3 times, most recently from 93abba7 to c97ea99 Compare July 12, 2018 11:02

gsalgado commented Jul 12, 2018

View reviewed changes

pipermerriam approved these changes Jul 12, 2018

View reviewed changes

carver reviewed Jul 12, 2018

View reviewed changes

carver requested changes Jul 12, 2018

View reviewed changes

carver approved these changes Jul 12, 2018

View reviewed changes

gsalgado force-pushed the issue-932 branch from c97ea99 to 425b338 Compare July 13, 2018 06:38

gsalgado added 2 commits July 13, 2018 07:50

During fast/light sync, check seal on a small subset of headers

8750d17

For every header batch, pick a random subset and check the seal only on those.

PR feedback

94038f4

gsalgado force-pushed the issue-932 branch from 425b338 to 94038f4 Compare July 13, 2018 07:43

pipermerriam approved these changes Jul 13, 2018

View reviewed changes

gsalgado merged commit 54f6ad6 into ethereum:master Jul 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

During fast/light sync, check seal on a small subset of headers #1010

During fast/light sync, check seal on a small subset of headers #1010

gsalgado commented Jul 12, 2018 •

edited

Loading

gsalgado Jul 12, 2018

pipermerriam Jul 12, 2018

gsalgado Jul 12, 2018

pipermerriam Jul 12, 2018

pipermerriam Jul 12, 2018

gsalgado Jul 13, 2018

pipermerriam Jul 12, 2018

gsalgado Jul 13, 2018

gsalgado Jul 13, 2018

carver Jul 12, 2018

gsalgado Jul 12, 2018

carver Jul 12, 2018 •

edited

Loading

gsalgado Jul 13, 2018

carver Jul 12, 2018

gsalgado Jul 12, 2018

carver Jul 12, 2018

carver left a comment •

edited

Loading

carver left a comment •

edited

Loading

During fast/light sync, check seal on a small subset of headers #1010

During fast/light sync, check seal on a small subset of headers #1010

Conversation

gsalgado commented Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver left a comment • edited Loading

Choose a reason for hiding this comment

carver left a comment • edited Loading

Choose a reason for hiding this comment

gsalgado commented Jul 12, 2018 •

edited

Loading

carver Jul 12, 2018 •

edited

Loading

carver left a comment •

edited

Loading

carver left a comment •

edited

Loading