Signing Blinded/BlobSidecar requests #887

usmansaleem · 2023-08-23T14:08:21Z

PR Description

Implement signing of BlindedBlobSidecar and BlobSidecar requests.

Fixed Issue(s)

Fixes #883

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.

Testing

I thought about testing these changes in a realistic/non-local environment.

jframe · 2023-08-24T06:31:46Z

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

-    return validatorRegistration;
-  }
-}
+public record Eth2SigningRequestBody(


That's much nicer to read

.../src/main/java/tech/pegasys/web3signer/core/service/http/handlers/signing/eth2/KZGProof.java

jframe · 2023-08-24T07:03:13Z

acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/utils/Eth2RequestUtils.java

+      final BlobSidecar tekuBlobSidecar = new DataStructureUtil(spec).randomBlobSidecar();
+      signingRoot =
+          new SigningRootUtil(spec).signingRootForBlobSidecar(tekuBlobSidecar, tekuForkInfo);
+      blindedBlobSidecar = fromInternalBlobSidecar(tekuBlobSidecar);


This looks like it building a blinded blob side car in both cases. Is there a difference between the two?

gfukushima

lgtm, few spdx to fix only

gfukushima · 2023-08-24T13:49:59Z

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

Nice clean up by using record

.../src/main/java/tech/pegasys/web3signer/core/service/http/handlers/signing/eth2/KZGProof.java

...ch/pegasys/web3signer/core/service/http/handlers/signing/eth2/json/KZGProofDeserializer.java

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

.../test/java/tech/pegasys/web3signer/tests/signing/BlsBlindedBlobcarSigningAcceptanceTest.java

...nce-tests/src/test/java/tech/pegasys/web3signer/dsl/utils/Eth2SigningRequestBodyBuilder.java

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

...c/main/java/tech/pegasys/web3signer/core/service/http/handlers/signing/eth2/BlobSidecar.java

siladu

Looks pretty straight forward, mostly just questions/renames.

It's a shame w3s needs to know about blinded blocks...is that just because of the Teku class hierarchy dependency?

CHANGELOG.md

...nce-tests/src/test/java/tech/pegasys/web3signer/dsl/utils/Eth2SigningRequestBodyBuilder.java

.../test/java/tech/pegasys/web3signer/tests/signing/BlsBlindedBlobcarSigningAcceptanceTest.java

...c/main/java/tech/pegasys/web3signer/core/service/http/handlers/signing/eth2/BlobSidecar.java

siladu · 2023-08-28T03:22:46Z

...c/main/java/tech/pegasys/web3signer/core/service/http/handlers/signing/eth2/BlobSidecar.java

+  public tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlindedBlobSidecar
+      asInternalBlindedBlobSidecar(final SpecVersion spec) {
+    final BlindedBlobSidecarSchema blindedBlobSidecarSchema =


Should this be BlobSidecar type instead now?

this is internal (Teku) class.

usmansaleem · 2023-08-28T04:51:14Z

Looks pretty straight forward, mostly just questions/renames.

It's a shame w3s needs to know about blinded blocks...is that just because of the Teku class hierarchy dependency?

Correct, Teku's classes are used to calculate signing root, hence we need them.

jframe · 2023-08-30T04:35:10Z

acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/utils/Eth2RequestUtils.java

+    if (isBlinded) {
+      // generate random blinded blobsidecar
+      final BlindedBlobSidecar tekuBlindedBlobSidecar =
+          new DataStructureUtil(spec).randomBlindedBlobSidecar();
+      signingRoot =
+          new SigningRootUtil(spec)
+              .signingRootForBlindedBlobSidecar(tekuBlindedBlobSidecar, tekuForkInfo);
+      blobSidecar = fromInternalBlindedBlobSidecar(tekuBlindedBlobSidecar);
+    } else {
+      // generate random blobsidecar
+      final BlobSidecar tekuBlobSidecar = new DataStructureUtil(spec).randomBlobSidecar();
+      signingRoot =
+          new SigningRootUtil(spec).signingRootForBlobSidecar(tekuBlobSidecar, tekuForkInfo);
+      blobSidecar = fromInternalBlobSidecar(tekuBlobSidecar);
+    }


Still don't understand why we are testing two cases. They are the same as far as Web3Signer is concerned. Are we gaining anything but having the other test case?

You are right, I was still thinking with regards to Teku side of things ... I'll fix the test case to test only "BlobSidecar"

jframe · 2023-08-30T04:39:42Z

gradle/versions.gradle

@@ -134,7 +134,7 @@ dependencyManagement {
    dependency 'com.azure:azure-core-http-netty:1.13.5'

    dependency 'com.zaxxer:HikariCP:5.0.1'
-    dependency 'org.postgresql:postgresql:42.5.3'
+    dependency 'org.postgresql:postgresql:42.6.0'


Was this fix an issue? Probably worthy of a changelog entry if that's the case

This was originally to fix an issue reported by owasp dependency-check plugin, however, it was later turned out to be false positive i.e. PostgreSQL 12 is affected, not the JDBC driver. I am going to revert this back to 42.5.3. The JDBC driver should be update from a separate PR.

usmansaleem added 7 commits August 23, 2023 22:25

Convert Eth2SigningRequestBody to record. Add blob_sidecar property

f2716bc

Update SigningObjectMapperFactory with KZGCommitment and KZGProof

3a26342

Builder for Eth2SigningRequest in AT

1d3bdb0

setup minimal web3signer for blob sidecar

f90a6e1

AT for Blobsidecar and BlindedBlobSidecar

b012e49

changelog

2ddc2bd

remove unnecessary comment

6356aec

usmansaleem requested review from gfukushima and jframe August 24, 2023 01:19

jframe reviewed Aug 24, 2023

View reviewed changes

gfukushima approved these changes Aug 24, 2023

View reviewed changes

jframe reviewed Aug 25, 2023

View reviewed changes

...nce-tests/src/test/java/tech/pegasys/web3signer/dsl/utils/Eth2SigningRequestBodyBuilder.java Outdated Show resolved Hide resolved

gfukushima reviewed Aug 25, 2023

View reviewed changes

.../tech/pegasys/web3signer/core/service/http/handlers/signing/eth2/Eth2SigningRequestBody.java Outdated Show resolved Hide resolved

usmansaleem added 2 commits August 25, 2023 15:38

Convert W3S BlindedBlobSidecar to BlobSidecar

44506fe

fixing year in license header

a7b2492

github-advanced-security bot found potential problems Aug 25, 2023

View reviewed changes

usmansaleem added 2 commits August 25, 2023 15:47

Updating PG JDBC driver due to CVE

8a9e980

Trigger Build

3b55dc7

siladu reviewed Aug 28, 2023

View reviewed changes

usmansaleem added 3 commits August 28, 2023 14:51

Refactoring after review

ca40080

remove @param from javadoc

91fa00a

Merge remote-tracking branch 'upstream/master' into blobsidecar_sign

fe2c707

usmansaleem requested review from siladu and jframe August 29, 2023 08:42

siladu approved these changes Aug 30, 2023

View reviewed changes

jframe reviewed Aug 30, 2023

View reviewed changes

usmansaleem added 2 commits August 30, 2023 20:39

Review suggestion - remove blinded blob sidecar acceptance test

c8f8398

Reverting PG JDBC driver version

b5aff5a

usmansaleem requested a review from jframe August 30, 2023 10:43

jframe approved these changes Aug 31, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/master' into blobsidecar_sign

b66ce9b

usmansaleem enabled auto-merge (squash) August 31, 2023 07:38

usmansaleem merged commit 0bb49c6 into Consensys:master Aug 31, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signing Blinded/BlobSidecar requests #887

Signing Blinded/BlobSidecar requests #887

usmansaleem commented Aug 23, 2023

jframe Aug 24, 2023

jframe Aug 24, 2023

gfukushima left a comment

gfukushima Aug 24, 2023

siladu left a comment

siladu Aug 28, 2023

usmansaleem Aug 28, 2023

usmansaleem commented Aug 28, 2023

jframe Aug 30, 2023

usmansaleem Aug 30, 2023

jframe Aug 30, 2023

usmansaleem Aug 30, 2023

Signing Blinded/BlobSidecar requests #887

Signing Blinded/BlobSidecar requests #887

Conversation

usmansaleem commented Aug 23, 2023

PR Description

Fixed Issue(s)

Documentation

Changelog

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfukushima left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siladu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usmansaleem commented Aug 28, 2023

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