Increase max attestation inclusion slot (p2p changes) #7305

StefanBratanov · 2023-06-26T10:21:35Z

PR Description

Created AttestationUtilDeneb to deal with slot inclusion gossip validation changes for attestations in Deneb. Moved the previous slot inclusion validations in AttestationUtilPhase0
Split AttestationValidatorTest into Deneb and Phase0 one
Other small nits/refactors

p2p-interface.md changes

Fixed Issue(s)

fixes #7299

Documentation

I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

I thought about adding a changelog entry, and added one if I deemed necessary.

rolfyone · 2023-07-03T22:17:47Z

...nsition/src/main/java/tech/pegasys/teku/statetransition/validation/AttestationValidator.java

+                  slotInclusionGossipValidationWithStateResult =
+                      attestationUtil.performSlotInclusionGossipValidation(attestation, state);


If we were going to do this, we'd be needing to pass the head state, not just any state chosen for gossip topic inclusion. Head state is also sketchy (eg. future slot errors where we haven't completed epoch transitions).

Because it's using the state to compute the current epoch and previous epoch, if we end up using something like a fork state from previous epoch as the best state, then we'd actually be using current-1, current-2, and that'd be bad...
My suggestion if they're only using state to check against epoch is we compute the epoch against wall time inside that function, and then even if the state machine is behind we're accepting gossip correctly...

Then performSlotInclusionGossipValidation would only take the attestation... unless we need a state because of test cases or something...

If we did we could probably just have a wrapper interface that takes attestation and state and computes the epoch of the state then calls a private function that takes attestation, headEpoch, and the default public interface could derive headEpoch from wallTime...

Anyway, that's a couple of ideas. happy to talk further.

Did change implementation to use the wall time instead.

...pec/src/main/java/tech/pegasys/teku/spec/logic/versions/deneb/util/AttestationUtilDeneb.java

tbenr

we discussed a couple of things in a call. This is one of them

tbenr · 2023-07-07T09:41:39Z

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/helpers/MiscHelpers.java

+    return computeStartTimeAtSlot(genesisTime, epochStartSlot);
+  }
+
+  public UInt64 computeStartTimeAtSlot(final UInt64 genesisTime, final UInt64 slot) {


Shouldn't be just another version of computeTimeAtSlot, otherwise it should be computeSlotStartTime (same for epoch). According to my hears.

rolfyone · 2023-07-10T10:08:00Z

...c/src/main/java/tech/pegasys/teku/spec/logic/versions/phase0/util/AttestationUtilPhase0.java

+    return attestationForkChoiceEligibleTimeMillis.isGreaterThan(discardAttestationsAfterMillis);
+  }
+
+  private boolean isCurrentTimeBeforeMinimumAttestationBroadcastTime(
+      final UInt64 attestationSlot, final UInt64 genesisTime, final UInt64 currentTimeMillis) {
+    final UInt64 minimumBroadcastTimeMillis =
+        minimumBroadcastTimeMillis(attestationSlot, genesisTime);
+    return currentTimeMillis.isLessThan(minimumBroadcastTimeMillis);
+  }
+
+  private boolean isCurrentTimeAfterAttestationPropagationSlotRange(
+      final UInt64 attestationSlot, final UInt64 genesisTime, final UInt64 currentTimeMillis) {
+    return maximumBroadcastTimeMillis(attestationSlot, genesisTime).isLessThan(currentTimeMillis);


can we check code coverage, and if its not immaculate, then we should consider making tests for these - they could easily be package private static, and we could feed in values to validate with great coverage..

Sure, I will extend current tests for Phase0. I didn't touch them but good idea to see we are covering all methods.

rolfyone · 2023-07-10T10:09:46Z

...pec/src/main/java/tech/pegasys/teku/spec/logic/versions/deneb/util/AttestationUtilDeneb.java

+  private boolean isAttestationSlotAfterCurrentTime(
+      final UInt64 attestationSlotTimeMillis, final UInt64 currentTimeMillis) {
+    return attestationSlotTimeMillis.isGreaterThan(
+        currentTimeMillis.plus(specConfig.getMaximumGossipClockDisparity()));
+  }
+
+  private boolean isAttestationSlotInCurrentOrPreviousEpoch(
+      final UInt64 attestationSlot, final UInt64 genesisTime, final UInt64 currentTimeMillis) {
+    final UInt64 currentSlot =
+        miscHelpers.computeSlotAtTime(genesisTime, millisToSeconds(currentTimeMillis));
+    final UInt64 currentEpoch = miscHelpers.computeEpochAtSlot(currentSlot);
+    final UInt64 previousEpoch = currentEpoch.minusMinZero(1);
+    final int slotDisparity = calculateMaximumGossipClockDisparityInSlots();
+    // min and max slot for the given attestation slot based on previous and current epoch with
+    // MAXIMUM_GOSSIP_CLOCK_DISPARITY
+    final UInt64 minSlot =
+        miscHelpers.computeStartSlotAtEpoch(previousEpoch).minusMinZero(slotDisparity);
+    final UInt64 maxSlot = miscHelpers.computeEndSlotAtEpoch(currentEpoch).plus(slotDisparity);
+
+    return attestationSlot.isGreaterThanOrEqualTo(minSlot)
+        && attestationSlot.isLessThanOrEqualTo(maxSlot);
+  }
+
+  private int calculateMaximumGossipClockDisparityInSlots() {
+    return (specConfig.getMaximumGossipClockDisparity() / specConfig.getMillisPerSlot()) + 1;
+  }


likewise here, could definitely do some unit testing if we need to check any of this... it would show us our business logic is correct for isAttestationSlotInCurrentOrPreviousEpoch for example, and we could easily boundary test etc...

rolfyone · 2023-07-10T10:10:40Z

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/helpers/MiscHelpers.java

+  public UInt64 computeEndSlotAtEpoch(final UInt64 epoch) {
+    return computeStartSlotAtEpoch(epoch).plus(specConfig.getSlotsPerEpoch() - 1);
+  }
+
+  public UInt64 computeSlotAtTime(final UInt64 genesisTime, final UInt64 currentTime) {
+    return currentTime.minusMinZero(genesisTime).dividedBy(specConfig.getSecondsPerSlot());
+  }
+
+  public UInt64 computeTimeAtSlot(final UInt64 genesisTime, final UInt64 slot) {
+    return genesisTime.plus(slot.times(specConfig.getSecondsPerSlot()));
+  }
+
+  public UInt64 computeTimeAtSlot(final BeaconState state, final UInt64 slot) {


test... they're simple in this case but we really do need basic tests so that when we refactor we can find issues.

...c/test/java/tech/pegasys/teku/spec/logic/versions/phase0/util/AttestationUtilPhase0Test.java

+import tech.pegasys.teku.spec.datastructures.operations.Attestation;
+import tech.pegasys.teku.spec.util.DataStructureUtil;
+
+class AttestationUtilPhase0Test {


...eum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/operations/AttestationData.java

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/helpers/MiscHelpers.java

StefanBratanov force-pushed the increase_max_attestation_inclusion_slot branch 3 times, most recently from baeadd1 to abd3bbd Compare June 27, 2023 12:22

StefanBratanov mentioned this pull request Jun 27, 2023

Move slot inclusion gossip validation to AttestationUtil #7307

Merged

2 tasks

StefanBratanov force-pushed the increase_max_attestation_inclusion_slot branch 2 times, most recently from bc36366 to e6e8917 Compare June 28, 2023 11:37

StefanBratanov marked this pull request as ready for review June 28, 2023 11:38

StefanBratanov force-pushed the increase_max_attestation_inclusion_slot branch 2 times, most recently from f7dafef to 23f2ffd Compare June 28, 2023 16:11

StefanBratanov changed the title ~~Increase max attestation inclusion slot~~ Increase max attestation inclusion slot (p2p changes) Jun 29, 2023

StefanBratanov marked this pull request as draft June 29, 2023 11:12

StefanBratanov force-pushed the increase_max_attestation_inclusion_slot branch from ec62ffa to 9c87726 Compare July 3, 2023 08:31

StefanBratanov marked this pull request as ready for review July 3, 2023 09:01

StefanBratanov force-pushed the increase_max_attestation_inclusion_slot branch from 4972cb3 to dceb04a Compare July 3, 2023 10:13

rolfyone reviewed Jul 3, 2023

View reviewed changes

StefanBratanov force-pushed the increase_max_attestation_inclusion_slot branch 3 times, most recently from 92f4980 to d420eb7 Compare July 5, 2023 14:56

rolfyone reviewed Jul 5, 2023

View reviewed changes

...pec/src/main/java/tech/pegasys/teku/spec/logic/versions/deneb/util/AttestationUtilDeneb.java Show resolved Hide resolved

StefanBratanov force-pushed the increase_max_attestation_inclusion_slot branch from 0a7c832 to 3cf4f85 Compare July 6, 2023 13:40

tbenr reviewed Jul 7, 2023

View reviewed changes

StefanBratanov force-pushed the increase_max_attestation_inclusion_slot branch 2 times, most recently from f8ef5b9 to 79d2ba7 Compare July 10, 2023 08:12

rolfyone reviewed Jul 10, 2023

View reviewed changes

github-advanced-security bot found potential problems Jul 10, 2023

View reviewed changes

StefanBratanov force-pushed the increase_max_attestation_inclusion_slot branch from 6f4f6bd to b669fde Compare July 10, 2023 19:33

rolfyone reviewed Jul 10, 2023

View reviewed changes

...eum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/operations/AttestationData.java Outdated Show resolved Hide resolved

rolfyone reviewed Jul 10, 2023

View reviewed changes

ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/helpers/MiscHelpers.java Outdated Show resolved Hide resolved

StefanBratanov added 26 commits July 12, 2023 10:28

improve logic

99e9d76

more improvements

77096b3

tiny refactor

0224b1f

fix

29dffd1

add unit tests

390c2a6

small nits

6877ce2

better calculation

0e70ce2

revert back GossipTopics changes

32ffa62

implement second gossip check from state

a7c8583

changes as per Paul's feedback

613d1a9

comment nit

219eee8

test improvements

c209fab

changes as per feedback

a6ea719

more simpler calculation

47b709a

some changes

54f0dfb

simplify calculation

1ef25d5

add test coverage

e81fdd1

add more tests

288aa60

remove duplicate method

a511ccd

suggestion

805ef17

fix build

9cbb1fb

fix build

d9099a5

improve tests

30e5246

provide useful comments

21303a6

adding more tests

62bb92b

dummy commit

d629e9c

StefanBratanov force-pushed the increase_max_attestation_inclusion_slot branch from 9d5c4a9 to d629e9c Compare July 12, 2023 09:28

StefanBratanov enabled auto-merge (squash) July 12, 2023 09:28

StefanBratanov merged commit 12a6c96 into Consensys:master Jul 12, 2023

tbenr mentioned this pull request Mar 19, 2024

Update seenTTL to cover 2 full epochs #8104

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase max attestation inclusion slot (p2p changes) #7305

Increase max attestation inclusion slot (p2p changes) #7305

StefanBratanov commented Jun 26, 2023 •

edited

Loading

rolfyone Jul 3, 2023

StefanBratanov Jul 5, 2023

tbenr left a comment

tbenr Jul 7, 2023

rolfyone Jul 10, 2023

StefanBratanov Jul 10, 2023

rolfyone Jul 10, 2023

rolfyone Jul 10, 2023

		slotInclusionGossipValidationWithStateResult =
		attestationUtil.performSlotInclusionGossipValidation(attestation, state);

Increase max attestation inclusion slot (p2p changes) #7305

Increase max attestation inclusion slot (p2p changes) #7305

Conversation

StefanBratanov commented Jun 26, 2023 • edited Loading

PR Description

Fixed Issue(s)

Documentation

Changelog

rolfyone Jul 3, 2023

Choose a reason for hiding this comment

StefanBratanov Jul 5, 2023

Choose a reason for hiding this comment

tbenr left a comment

Choose a reason for hiding this comment

tbenr Jul 7, 2023

Choose a reason for hiding this comment

rolfyone Jul 10, 2023

Choose a reason for hiding this comment

StefanBratanov Jul 10, 2023

Choose a reason for hiding this comment

rolfyone Jul 10, 2023

Choose a reason for hiding this comment

rolfyone Jul 10, 2023

Choose a reason for hiding this comment

StefanBratanov commented Jun 26, 2023 •

edited

Loading