Merge branch 'master' into enable-large-cache-per-cf

CONTRIBUTING.md

@@ -33,7 +33,19 @@ Start by looking through the 'good first issue' and 'help wanted' issues:
 * [Good First Issue][search-label-good-first-issue] - issues which should only require a few lines of code, and a test or two.
 * [Help wanted issues][search-label-help-wanted] - issues that are a bit more involved than `good first issue` issues.
 
-Please keep in mind that we do not accept non-code contributions like fixing comments, typos or some other trivial fixes. Although we appreciate the extra help, managing lots of these small contributions is unfeasible, and puts extra pressure in our continuous delivery systems (running all tests, etc). Feel free to open an issue pointing any of those errors and we will batch them into a single change.
+Please reach out in discord if you're looking to help out, and we can assist you in finding a good candidate ticket to work on, or discuss the idea you have. 
+
+We have a [Teku](https://discord.com/channels/697535391594446898/697539289042649190) channel, and also a [Teku Contributors](https://discord.com/channels/697535391594446898/1050616638497640548) channel.
+
+Due to the prevalence of 'airdrop farming' type practices, this unfortunately puts heightened scrutiny on first time contributors, but if you're genuinely looking to help out, we'd really love to assist you in any way we can.
+This does mean however that we will generally reject 'random' fixes such as 'TODO' fixes, typos, and generally things that add no value that we haven't identified as something we need. These are likely to be rejected with 'due to contribution guidelines' type responses.
+This includes but is not limited to
+* code replacement of TODO's that are not well tested or justified by performance and regression tests to prove their worth.
+* typos, even if valid, will be worked into other PRs or just ignored completely if they're from first time contributors with no substantative value.
+* things like replacing RuntimeException with a new exception type that's not well tested and adding value.
+* rewording of comments
+
+Minimal discussion will be given in PR's due to the volume we're needing to deal with currently of this type of PR, which is taking away from actual development time, so please don't be offended if you're genuinely trying to help out; and we say 'see contribution guidelines'.
 
 ### Local Development
 The codebase is maintained using the "*contributor workflow*" where everyone without exception contributes patch proposals using "*pull-requests*". This facilitates social contribution, easy testing and peer review.

ethereum/executionclient/src/integration-test/java/tech/pegasys/teku/ethereum/executionclient/rest/RestBuilderClientTest.java

@@ -673,9 +673,12 @@ private void runGetPayloadSuccessAsJson() {
               verifyBuilderPayloadResponse(builderPayload);
             });
     final Consumer<RecordedRequest> containsConsensusVersionHeader =
-        req ->
-            assertThat(req.getHeader("Eth-Consensus-Version"))
-                .isEqualTo(milestone.name().toLowerCase(Locale.ROOT));
+        req -> {
+          assertThat(req.getHeader("Eth-Consensus-Version"))
+              .isEqualTo(milestone.name().toLowerCase(Locale.ROOT));
+          assertThat(req.getHeader("Accept"))
+              .isEqualTo("application/octet-stream;q=1.0,application/json;q=0.9");
+        };
     verifyRequest(
         "POST",
         "/eth/v1/builder/blinded_blocks",

ethereum/executionclient/src/main/java/tech/pegasys/teku/ethereum/executionclient/rest/ResponseHandler.java

@@ -85,7 +85,8 @@ void handleResponse(final Request request, final okhttp3.Response response) {
         return;
       }
 
-      if (!responseMediaType.is(MediaType.JSON_UTF_8)) {
+      if (!responseMediaType.type().equals(MediaType.JSON_UTF_8.type())
+          || !responseMediaType.subtype().equals(MediaType.JSON_UTF_8.subtype())) {
         LOG.warn(
             "Response contains an incorrect Content-Type header: {}, attempting to parse as {} [{}]",
             responseMediaType,

ethereum/executionclient/src/main/java/tech/pegasys/teku/ethereum/executionclient/rest/RestBuilderClient.java

@@ -212,7 +212,9 @@ public SafeFuture<Response<BuilderPayload>> getPayload(
     return restClient
         .postAsync(
             BuilderApiMethod.GET_PAYLOAD.getPath(),
-            Map.of(HEADER_CONSENSUS_VERSION, milestone.name().toLowerCase(Locale.ROOT)),
+            Map.ofEntries(
+                Map.entry(HEADER_CONSENSUS_VERSION, milestone.name().toLowerCase(Locale.ROOT)),
+                ACCEPT_HEADER),
             signedBlindedBeaconBlock,
             LAST_RECEIVED_HEADER_WAS_IN_SSZ.get(),
             responseTypeDefinition)

ethereum/executionlayer/src/main/java/tech/pegasys/teku/ethereum/executionlayer/BuilderBidValidatorImpl.java

@@ -123,7 +123,7 @@ public void validateBuilderBid(
     final UInt64 proposedGasLimit = executionPayloadHeader.getGasLimit();
 
     final UInt64 expectedGasLimit = expectedGasLimit(parentGasLimit, preferredGasLimit);
-    if (!expectedGasLimit.equals(preferredGasLimit)) {
+    if (!expectedGasLimit.equals(proposedGasLimit)) {
       eventLogger.builderBidNotHonouringGasLimit(
           parentGasLimit, proposedGasLimit, expectedGasLimit, preferredGasLimit);
     }

ethereum/executionlayer/src/test/java/tech/pegasys/teku/ethereum/executionlayer/BuilderBidValidatorTest.java

@@ -167,7 +167,7 @@ void expectedGasLimitTestCases(
   @Test
   void shouldNotLogEventIfGasLimitDecreases() throws BuilderBidValidationException {
     // 1023001 is as high as it can move in 1 shot
-    prepareGasLimit(UInt64.valueOf(1024_000), UInt64.valueOf(1022_000), UInt64.valueOf(1023_001));
+    prepareGasLimit(UInt64.valueOf(1024_000), UInt64.valueOf(1023_001), UInt64.valueOf(1022_000));
 
     builderBidValidatorWithMockSpec.validateBuilderBid(
         signedBuilderBid, validatorRegistration, state, Optional.empty());
@@ -178,7 +178,7 @@ void shouldNotLogEventIfGasLimitDecreases() throws BuilderBidValidationException
   @Test
   void shouldNotLogEventIfGasLimitIncreases() throws BuilderBidValidationException {
     // 1024999 is as high as it can move in 1 shot
-    prepareGasLimit(UInt64.valueOf(1024_000), UInt64.valueOf(1025_000), UInt64.valueOf(1024_999));
+    prepareGasLimit(UInt64.valueOf(1024_000), UInt64.valueOf(1024_999), UInt64.valueOf(1025_000));
 
     builderBidValidatorWithMockSpec.validateBuilderBid(
         signedBuilderBid, validatorRegistration, state, Optional.empty());

ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/attestation/AggregatingAttestationPool.java

@@ -15,6 +15,7 @@
 
 import it.unimi.dsi.fastutil.ints.Int2IntMap;
 import java.util.Collection;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -26,6 +27,7 @@
 import java.util.TreeMap;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Predicate;
+import java.util.stream.Stream;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.tuweni.bytes.Bytes;
@@ -57,6 +59,11 @@ public class AggregatingAttestationPool implements SlotEventsChannel {
   /** The valid attestation retention period is 64 slots in deneb */
   static final long ATTESTATION_RETENTION_SLOTS = 64;
 
+  static final Comparator<Attestation> ATTESTATION_INCLUSION_COMPARATOR =
+      Comparator.<Attestation>comparingInt(
+              attestation -> attestation.getAggregationBits().getBitCount())
+          .reversed();
+
   /**
    * Default maximum number of attestations to store in the pool.
    *
@@ -238,22 +245,20 @@ public synchronized SszList<Attestation> getAttestationsForBlock(
         schemaDefinitions.getAttestationSchema().requiresCommitteeBits();
 
     final AtomicInteger prevEpochCount = new AtomicInteger(0);
+
     return dataHashBySlot
         // We can immediately skip any attestations from the block slot or later
         .headMap(stateAtBlockSlot.getSlot(), false)
         .descendingMap()
         .values()
         .stream()
-        .flatMap(Collection::stream)
-        .map(attestationGroupByDataHash::get)
-        .filter(Objects::nonNull)
-        .filter(group -> isValid(stateAtBlockSlot, group.getAttestationData()))
-        .filter(forkChecker::areAttestationsFromCorrectFork)
-        .flatMap(MatchingDataAttestationGroup::stream)
-        .map(ValidatableAttestation::getAttestation)
-        .filter(
-            attestation ->
-                attestation.requiresCommitteeBits() == blockRequiresAttestationsWithCommitteeBits)
+        .flatMap(
+            dataHashSetForSlot ->
+                streamAggregatesForDataHashesBySlot(
+                    dataHashSetForSlot,
+                    stateAtBlockSlot,
+                    forkChecker,
+                    blockRequiresAttestationsWithCommitteeBits))
         .limit(attestationsSchema.getMaxLength())
         .filter(
             attestation -> {
@@ -267,6 +272,25 @@ public synchronized SszList<Attestation> getAttestationsForBlock(
         .collect(attestationsSchema.collector());
   }
 
+  private Stream<Attestation> streamAggregatesForDataHashesBySlot(
+      final Set<Bytes> dataHashSetForSlot,
+      final BeaconState stateAtBlockSlot,
+      final AttestationForkChecker forkChecker,
+      final boolean blockRequiresAttestationsWithCommitteeBits) {
+
+    return dataHashSetForSlot.stream()
+        .map(attestationGroupByDataHash::get)
+        .filter(Objects::nonNull)
+        .filter(group -> isValid(stateAtBlockSlot, group.getAttestationData()))
+        .filter(forkChecker::areAttestationsFromCorrectFork)
+        .flatMap(MatchingDataAttestationGroup::stream)
+        .map(ValidatableAttestation::getAttestation)
+        .filter(
+            attestation ->
+                attestation.requiresCommitteeBits() == blockRequiresAttestationsWithCommitteeBits)
+        .sorted(ATTESTATION_INCLUSION_COMPARATOR);
+  }
+
   public synchronized List<Attestation> getAttestations(
       final Optional<UInt64> maybeSlot, final Optional<UInt64> maybeCommitteeIndex) {
 

ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/attestation/AggregatingAttestationPoolTest.java

@@ -63,7 +63,7 @@
 class AggregatingAttestationPoolTest {
 
   public static final UInt64 SLOT = UInt64.valueOf(1234);
-  private static final int COMMITTEE_SIZE = 20;
+  private static final int COMMITTEE_SIZE = 130;
 
   private Spec spec;
   private SpecMilestone specMilestone;
@@ -260,15 +260,61 @@ void getAttestationsForBlock_shouldIncludeMoreRecentAttestationsFirst() {
 
   @TestTemplate
   public void getAttestationsForBlock_shouldNotAddMoreAttestationsThanAllowedInBlock() {
-    final BeaconState state = dataStructureUtil.randomBeaconState(ONE);
+    final int allowed =
+        Math.toIntExact(
+            spec.atSlot(ONE)
+                .getSchemaDefinitions()
+                .getBeaconBlockBodySchema()
+                .getAttestationsSchema()
+                .getMaxLength());
+
+    final int validatorCount = allowed + 1;
+    final BeaconState state = dataStructureUtil.randomBeaconState(validatorCount, 100, ONE);
     final AttestationData attestationData = dataStructureUtil.randomAttestationData(ZERO);
-    final Attestation attestation1 = addAttestationFromValidators(attestationData, 1, 2, 3, 4);
-    final Attestation attestation2 = addAttestationFromValidators(attestationData, 2, 5);
-    // Won't be included because of the 2 attestation limit.
-    addAttestationFromValidators(attestationData, 2);
 
-    assertThat(aggregatingPool.getAttestationsForBlock(state, forkChecker))
-        .containsExactly(attestation1, attestation2);
+    final int lastValidatorIndex = validatorCount - 1;
+
+    // add non aggregatable attestations, more than allowed in block
+    for (int i = 0; i < validatorCount; i++) {
+      addAttestationFromValidators(attestationData, i, lastValidatorIndex);
+    }
+
+    assertThat(aggregatingPool.getAttestationsForBlock(state, forkChecker)).hasSize(allowed);
+  }
+
+  @TestTemplate
+  public void getAttestationsForBlock_shouldGivePriorityToBestAggregationForEachSlot() {
+    // let's test this on electra only, which has only 8 attestations for block
+    assumeThat(specMilestone).isGreaterThanOrEqualTo(ELECTRA);
+    assertThat(
+            spec.atSlot(ONE)
+                .getSchemaDefinitions()
+                .getBeaconBlockBodySchema()
+                .getAttestationsSchema()
+                .getMaxLength())
+        .isEqualTo(8);
+
+    final BeaconState state = dataStructureUtil.randomBeaconState(ONE);
+
+    // let's prepare 2 different attestationData for the same slot
+    final AttestationData attestationData0 = dataStructureUtil.randomAttestationData(ZERO);
+    final AttestationData attestationData1 = dataStructureUtil.randomAttestationData(ZERO);
+
+    // let's fill up the pool with non-aggregatable attestationsData0
+    addAttestationFromValidators(attestationData0, 1, 2);
+    addAttestationFromValidators(attestationData0, 1, 3);
+    addAttestationFromValidators(attestationData0, 1, 4);
+    addAttestationFromValidators(attestationData0, 1, 5);
+    addAttestationFromValidators(attestationData0, 1, 6);
+    addAttestationFromValidators(attestationData0, 1, 7);
+    addAttestationFromValidators(attestationData0, 1, 8);
+    addAttestationFromValidators(attestationData0, 1, 9);
+
+    // let's add a better aggregation for attestationData1
+    final Attestation bestAttestation = addAttestationFromValidators(attestationData1, 11, 14, 15);
+
+    assertThat(aggregatingPool.getAttestationsForBlock(state, forkChecker).get(0))
+        .isEqualTo(bestAttestation);
   }
 
   @TestTemplate

infrastructure/logging/src/main/java/tech/pegasys/teku/infrastructure/logging/EventLogger.java

@@ -336,12 +336,12 @@ public void depositContractLogsSyncingDisabled() {
   public void builderBidNotHonouringGasLimit(
       final UInt64 parentGasLimit,
       final UInt64 proposedGasLimit,
-      final UInt64 targetGasLimit,
+      final UInt64 expectedGasLimit,
       final UInt64 preferredGasLimit) {
     String reorgEventLog =
         String.format(
             "Builder proposed a bid not honouring the validator gas limit preference. Parent: %s - Proposed: %s - Expected %s - Target: %s",
-            parentGasLimit, proposedGasLimit, targetGasLimit, preferredGasLimit);
+            parentGasLimit, proposedGasLimit, expectedGasLimit, preferredGasLimit);
     warn(reorgEventLog, Color.YELLOW);
   }