sign type block_v2 to support Altair and future forks #422

usmansaleem · 2021-08-25T07:43:33Z

Block signing request for Altair fork uses different block than phase 0. Introduce new sign type BLOCK_V2 which will allow Altair and future forks block signing requests. Keep BLOCK for phase 0 backward compatibility.

Signed-off-by: Usman Saleem <[email protected]>

...tance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfigurationBuilder.java

Signed-off-by: Usman Saleem <[email protected]>

siladu · 2021-08-31T05:48:18Z

...rc/test/java/tech/pegasys/web3signer/tests/signing/Eth2AltairBlockSigningAcceptanceTest.java

@@ -77,7 +72,7 @@ void signAndVerifyBlockV2Signature(final SpecMilestone specMilestone) throws Exc
  @Test
  void signAndVerifyLegacyBlockSignature() throws Exception {
    final Eth2BlockSigningRequestUtil util = new Eth2BlockSigningRequestUtil(SpecMilestone.PHASE0);
-    setupSigner("eth2", null, "minimal", Optional.empty());
+    setupSigner("eth2", null, "minimal", null);


Might be overkill in this case, but could avoid passing the null into the Optional with another overloaded setupSigner method which doesn't call withAltairForkEpoch

you are correct, its technically only for legacy block signing that we need to start web3signer without altair fork. Rest of signing types works with altair fork.

Rather we didn't pass in null as well, even an optional altair block would be nicer. Might be nit picking though.

Signed-off-by: Usman Saleem <[email protected]>

jframe · 2021-09-01T00:51:37Z

...tance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfigurationBuilder.java

@@ -196,8 +197,13 @@ public SignerConfigurationBuilder withUseConfigFile(final boolean useConfigFile)
    return this;
  }

-  public SignerConfigurationBuilder withAltairForkEpoch(final long altairForkEpoch) {
-    this.altairForkEpoch = altairForkEpoch;
+  public SignerConfigurationBuilder withAltairForkEpoch(final Long altairForkEpoch) {


Be nicer to use the primitive long

jframe · 2021-09-01T00:52:42Z

...tance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfigurationBuilder.java

-  public SignerConfigurationBuilder withAltairForkEpoch(final long altairForkEpoch) {
-    this.altairForkEpoch = altairForkEpoch;
+  public SignerConfigurationBuilder withAltairForkEpoch(final Long altairForkEpoch) {
+    this.altairForkEpoch = Optional.ofNullable(altairForkEpoch);


Is this ofNullable because null is being passed into the builder somewhere? Rather didn't do that

jframe · 2021-09-01T00:59:02Z

...tance-tests/src/test/java/tech/pegasys/web3signer/dsl/utils/Eth2BlockSigningRequestUtil.java

+    signingRoot = signingRootUtil.signingRootForSignBlock(beaconBlock, tekuForkInfo);
+  }
+
+  public Eth2SigningRequestBody createRandomBlockV2Request() {


Is this actually random? Looks like dataStructureUtil creates deterministic data keyed off the slot?

it is deterministic ... I think createCanned.... naming makes more sense.

jframe · 2021-09-01T00:59:34Z

...tance-tests/src/test/java/tech/pegasys/web3signer/dsl/utils/Eth2BlockSigningRequestUtil.java

+        null);
+  }
+
+  public Eth2SigningRequestBody createRandomLegacyBlockRequest() {


Is there any benefit in having this method and the one in Eth2RequestUtils?

jframe · 2021-09-01T01:04:02Z

...ance-tests/src/test/java/tech/pegasys/web3signer/tests/signing/BlsSigningAcceptanceTest.java

@@ -59,7 +59,7 @@
  private static final BLSPublicKey publicKey = keyPair.getPublicKey();

  @ParameterizedTest
-  @EnumSource
+  @EnumSource(mode = EnumSource.Mode.EXCLUDE, names = "BLOCK_V2")


Why do we need to exclude BLOCK_V2? It would good to include the block v2 in these tests for completeness.

jframe · 2021-09-01T01:08:37Z

...rc/test/java/tech/pegasys/web3signer/tests/signing/Eth2AltairBlockSigningAcceptanceTest.java

@@ -77,7 +72,7 @@ void signAndVerifyBlockV2Signature(final SpecMilestone specMilestone) throws Exc
  @Test
  void signAndVerifyLegacyBlockSignature() throws Exception {
    final Eth2BlockSigningRequestUtil util = new Eth2BlockSigningRequestUtil(SpecMilestone.PHASE0);
-    setupSigner("eth2", null, "minimal", Optional.empty());
+    setupSigner("eth2", null, "minimal", null);


Rather we didn't pass in null as well, even an optional altair block would be nicer. Might be nit picking though.

jframe · 2021-09-01T01:13:45Z

.../tech/pegasys/web3signer/core/service/http/handlers/signing/eth2/Eth2SigningRequestBody.java

@@ -44,6 +45,7 @@ public Eth2SigningRequestBody(
      @JsonProperty("signingRoot") final Bytes signingRoot,
      @JsonProperty("fork_info") final ForkInfo fork_info,
      @JsonProperty("block") final BeaconBlock block,
+      @JsonProperty("beacon_block") final BlockRequest blockRequest,


It is a bit confusing having block and beacon_block in the request. Not sure there is much we can do about now that Teku has implemented this though

jframe · 2021-09-01T01:14:43Z

...egasys/web3signer/core/service/http/handlers/signing/eth2/json/BlockRequestDeserializer.java

+  }
+
+  @Override
+  public BlockRequest deserialize(final JsonParser p, final DeserializationContext ctxt)


Perhaps a test? We do have AT though, so maybe that is enough

jframe · 2021-09-01T01:18:53Z

core/src/main/java/tech/pegasys/web3signer/core/service/http/SigningJsonProvider.java

+import org.apache.tuweni.bytes.Bytes32;
+import org.apache.tuweni.units.bigints.UInt64;
+
+public class SigningJsonProvider {


The way we are using this doing new SigningJsonProvider().getObjectMapper() perhaps a factory would be would be better?

Signed-off-by: Usman Saleem <[email protected]>

jframe

LGTM

sign type block_v2 to support Altair and future forks

0ec2632

Signed-off-by: Usman Saleem <[email protected]>

ajsutton mentioned this pull request Aug 26, 2021

Support Altair #391

Closed

usmansaleem added 3 commits August 27, 2021 10:35

Update OpenAPI spec for Altair block signing

75288cb

Signed-off-by: Usman Saleem <[email protected]>

exclude BLOCK_V2 from current bls signing AT

899f60b

Signed-off-by: Usman Saleem <[email protected]>

Altair bloc signing AT

d31026f

Signed-off-by: Usman Saleem <[email protected]>

usmansaleem added the TeamCerberus Under active development by TeamCerberus @Consensys label Aug 27, 2021

usmansaleem self-assigned this Aug 27, 2021

usmansaleem added 4 commits August 27, 2021 18:37

Fix web3signer-eth2.yaml - WIP

ee3e038

Signed-off-by: Usman Saleem <[email protected]>

working openapi spec

b3ddd6e

Signed-off-by: Usman Saleem <[email protected]>

Add network to AT DSL

0be9a5c

Signed-off-by: Usman Saleem <[email protected]>

Update Teku libraries version to 21.8.2

168e9ea

Signed-off-by: Usman Saleem <[email protected]>

usmansaleem marked this pull request as ready for review August 30, 2021 00:00

usmansaleem added 7 commits August 30, 2021 15:34

update openapi schema

f2cbf69

Signed-off-by: Usman Saleem <[email protected]>

license information

c1787ab

Signed-off-by: Usman Saleem <[email protected]>

test case failing

46c00c9

Signed-off-by: Usman Saleem <[email protected]>

AT - fix altair fork for phase0 tests

15afc38

Signed-off-by: Usman Saleem <[email protected]>

Reversing temporary debug logging

18184c4

Signed-off-by: Usman Saleem <[email protected]>

minor refactoring

2b691cc

Signed-off-by: Usman Saleem <[email protected]>

removing debug

fbaffe4

Signed-off-by: Usman Saleem <[email protected]>

usmansaleem requested a review from jframe August 31, 2021 04:22

siladu reviewed Aug 31, 2021

View reviewed changes

...tance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfigurationBuilder.java Outdated Show resolved Hide resolved

Altair fork in DSL - switch back to non-optional arg

4794e18

Signed-off-by: Usman Saleem <[email protected]>

siladu reviewed Aug 31, 2021

View reviewed changes

Changelog

034a2d0

Signed-off-by: Usman Saleem <[email protected]>

jframe reviewed Sep 1, 2021

View reviewed changes

usmansaleem mentioned this pull request Sep 1, 2021

Add support for Prater Altair fork #423

Closed

usmansaleem added 3 commits September 1, 2021 16:23

cleanup SigningAcceptanceTestBase

b9f324e

Signed-off-by: Usman Saleem <[email protected]>

remove exclude BLOCK_V2 from existing AT

f7ee01c

Signed-off-by: Usman Saleem <[email protected]>

object mapper factory

b1b044f

Signed-off-by: Usman Saleem <[email protected]>

usmansaleem requested review from siladu and jframe September 1, 2021 07:23

siladu closed this Sep 1, 2021

siladu reopened this Sep 1, 2021

jframe approved these changes Sep 1, 2021

View reviewed changes

usmansaleem merged commit 15fc5c5 into Consensys:master Sep 2, 2021

usmansaleem deleted the altair_block_signing branch September 2, 2021 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sign type block_v2 to support Altair and future forks #422

sign type block_v2 to support Altair and future forks #422

usmansaleem commented Aug 25, 2021

siladu Aug 31, 2021

usmansaleem Aug 31, 2021

jframe Sep 1, 2021

jframe Sep 1, 2021

jframe Sep 1, 2021

jframe Sep 1, 2021

usmansaleem Sep 1, 2021

jframe Sep 1, 2021

jframe Sep 1, 2021

jframe Sep 1, 2021

jframe Sep 1, 2021

jframe Sep 1, 2021

jframe Sep 1, 2021

jframe left a comment

sign type block_v2 to support Altair and future forks #422

sign type block_v2 to support Altair and future forks #422

Conversation

usmansaleem commented Aug 25, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jframe left a comment

Choose a reason for hiding this comment