Deneb - web3signer #12767

james-prysm · 2023-08-21T20:24:24Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

implements web3signer requirements for deneb

add Web3signer support for Deneb blocks
add Web3signer support for signed blobs

Webhooks defined here ethereum/remote-signing-api#4

Which issues(s) does this PR fix?

part of #12248 and tracked in #9994

validator/keymanager/remote-web3signer/v1/web3signer_types.go

prestonvanloon · 2023-09-12T17:51:36Z

encoding/ssz/htrutils.go

-func transactionRoot(tx []byte) ([32]byte, error) {
-	chunkedRoots, err := PackByChunk([][]byte{tx})
+// ByteSliceRoot is a helper func to merkleize an arbitrary List[Byte, N]
+// this func runs Chunkify + MerkleizeVector
+func ByteSliceRoot(slice []byte) ([32]byte, error) {


Are there no tests for this method? Surprised you could rename it without breaking tests 😱

hmm your concern here is probably valid I'll double check with kasey and potuz 😅

prestonvanloon · 2023-09-12T17:56:00Z

validator/keymanager/remote-web3signer/keymanager.go

+	return json.Marshal(blobRequest)
+}
+
+func handleBlindedBlob(ctx context.Context, validator *validator.Validate, request *validatorpb.SignRequest, genesisValidatorsRoot []byte) ([]byte, error) {


genesisValidatorsRoot is not used, is this to conform to some interface? Several methods in this file that have unused params.

this is a good point I can probably remove, i had to make this function because of the linter cognit I can remove for many of these.

oof ok it actually needs the fork info so thanks for catching

terencechain · 2023-09-20T20:33:15Z

validator/keymanager/remote-web3signer/v1/web3signer_types.go

@@ -337,6 +345,18 @@ type ValidatorRegistration struct {
 	Pubkey       hexutil.Bytes `json:"pubkey"  validate:"required"`       /* bls hexadecimal string */
 }

+// BlobSidecar a sub property of BlobSidecarSignRequest
+type BlobSidecar struct {


Shouldn't this be blind blob sidecar?

The name is as the web3signer defines (not builder) ethereum/remote-signing-api#4

james-prysm · 2023-09-27T21:17:14Z

testing/endtoend/components/web3remotesigner.go

 	rawYaml = append(rawYaml, []byte(depContractStr)...)
+	rawYaml = append(rawYaml, params.NetworkConfigToYaml(params.BeaconNetworkConfig())...)
+
+	rawYaml = append(rawYaml, []byte("\nEPOCHS_PER_SUBNET_SUBSCRIPTION: 256\n")...)


what should I do with this? should i update config to yaml to include network properties? and should these be included to the network configs?
related to ethereum/consensus-specs#3375 and #12864

nisdas · 2023-09-29T08:56:12Z

encoding/ssz/htrutils.go

-
-	maxLength := (fieldparams.MaxBytesPerTxLength + 31) / 32
-	bytesRoot, err := BitwiseMerkleize(chunkedRoots, uint64(len(chunkedRoots)), uint64(maxLength))
+	maxRootLength := maxLength / fieldparams.RootLength


this looks odd, the reason we did

(value + 31)/32

is so that the ending chunk if it isnt fully occupied ( < 32 bytes) is still
correctly addressed as 1 full chunk.

let me revert this then, maybe there's a better way to do this

saolyn · 2023-10-13T11:31:15Z

validator/keymanager/remote-web3signer/keymanager.go

+	return json.Marshal(randaoRevealSignRequest)
+}
+
+func handleExit(ctx context.Context, validator *validator.Validate, request *validatorpb.SignRequest, genesisValidatorsRoot []byte) ([]byte, error) {


Same here, should this be more like handleVoluntaryExit

saolyn · 2023-10-13T11:33:26Z

validator/keymanager/remote-web3signer/keymanager.go

+	return json.Marshal(aggregateAndProofSignRequest)
+}
+
+func handleSlot(ctx context.Context, validator *validator.Validate, request *validatorpb.SignRequest, genesisValidatorsRoot []byte) ([]byte, error) {


A nit pick but this sounds less descriptive than the other handlers, should it be more like handleAggregationSlot

validator/keymanager/remote-web3signer/metrics.go

saolyn · 2023-10-13T11:38:41Z

encoding/ssz/htrutils.go

-
-	maxLength := (fieldparams.MaxBytesPerTxLength + 31) / 32
-	bytesRoot, err := BitwiseMerkleize(chunkedRoots, uint64(len(chunkedRoots)), uint64(maxLength))
+	maxRootLength := (maxLength + 31) / 32 // nearest number divisible by root length (32)


Can we add a comment as to why we are doing + 31 and since 32 is the root length would it be better to minimize the magic numbers and have a const for this?

(value + 31)/32
is so that the ending chunk if it isnt fully occupied ( < 32 bytes) is still
correctly addressed as 1 full chunk.

potuz and nishant gave me this info.
i added the comment to represent this, any other opinions on making this clearer?

validator/keymanager/remote-web3signer/v1/requests.go

Co-authored-by: Sammy Rosso <[email protected]>

nisdas · 2023-10-16T04:55:58Z

cmd/beacon-chain/tosaccepted

Remove this file

james-prysm requested a review from a team as a code owner August 21, 2023 20:24

james-prysm requested review from terencechain, rkapka and nisdas and removed request for a team August 21, 2023 20:24

james-prysm self-assigned this Aug 21, 2023

james-prysm added Deneb PRs or issues for the Deneb upgrade Web3Signer Web3Signer related tasks labels Aug 21, 2023

james-prysm changed the title ~~Deneb web3signer~~ Deneb - web3signer Aug 21, 2023

kasey force-pushed the deneb-integration branch from fc45d3b to 963fefc Compare August 23, 2023 17:01

kasey requested a review from prestonvanloon as a code owner August 23, 2023 17:01

kasey force-pushed the deneb-integration branch from 963fefc to ce697f3 Compare August 23, 2023 21:28

prestonvanloon force-pushed the deneb-integration branch from 7b5fdfa to e166e5d Compare August 25, 2023 01:41

james-prysm force-pushed the deneb-web3signer branch from 77aac60 to e0f7eb8 Compare August 25, 2023 16:16

prestonvanloon force-pushed the deneb-integration branch from f6476a9 to 70143ef Compare August 31, 2023 13:01

Base automatically changed from deneb-integration to develop August 31, 2023 13:42

james-prysm added 5 commits September 1, 2023 14:36

wip

ad860f9

adding deneb block

8ba9e78

adding in support for blobs and fixing unit tests for deneb

4224b51

fixing linting

4d788b6

gaz

f05526a

james-prysm force-pushed the deneb-web3signer branch 2 times, most recently from 039d792 to f05526a Compare September 1, 2023 20:18

james-prysm and others added 2 commits September 8, 2023 13:50

Merge branch 'develop' into deneb-web3signer

33e8f9b

adding support for new web3signer version

fb264b4

james-prysm added the Ready For Review A pull request ready for code review label Sep 8, 2023

james-prysm and others added 3 commits September 8, 2023 15:44

Merge branch 'develop' into deneb-web3signer

72ecb90

Merge branch 'develop' into deneb-web3signer

188f91a

fixing tag name

5f6148a

prestonvanloon reviewed Sep 12, 2023

View reviewed changes

terencechain reviewed Sep 20, 2023

View reviewed changes

james-prysm and others added 4 commits September 26, 2023 10:09

Merge branch 'develop' into deneb-web3signer

dd42b1e

Merge branch 'develop' into deneb-web3signer

f8b4b58

adding network configs

0a53b89

Merge branch 'develop' into deneb-web3signer

a62a4c6

james-prysm commented Sep 27, 2023

View reviewed changes

james-prysm and others added 4 commits September 27, 2023 20:38

removing duplicate

d9a28bd

Merge branch 'develop' into deneb-web3signer

eb40cda

Merge branch 'develop' into deneb-web3signer

7a1e48a

Merge branch 'develop' into deneb-web3signer

49f460e

nisdas reviewed Oct 11, 2023

View reviewed changes

james-prysm and others added 2 commits October 12, 2023 15:55

changing based on nishant's feedback

415da49

Merge branch 'develop' into deneb-web3signer

ad21475

saolyn reviewed Oct 13, 2023

View reviewed changes

validator/keymanager/remote-web3signer/v1/requests.go Outdated Show resolved Hide resolved

james-prysm and others added 4 commits October 13, 2023 10:40

Update validator/keymanager/remote-web3signer/v1/requests.go

05ff368

Co-authored-by: Sammy Rosso <[email protected]>

Update validator/keymanager/remote-web3signer/metrics.go

2e6f2cd

Co-authored-by: Sammy Rosso <[email protected]>

sammy's suggestions

fb01148

Merge branch 'develop' into deneb-web3signer

b32e2ac

nisdas reviewed Oct 16, 2023

View reviewed changes

cmd/beacon-chain/tosaccepted Outdated

Copy link

Member

nisdas Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this file

removing temp file

24b1cd7

nisdas approved these changes Oct 16, 2023

View reviewed changes

Merge branch 'develop' into deneb-web3signer

109d4cb

nisdas added the OK to merge label Oct 16, 2023

james-prysm removed the Ready For Review A pull request ready for code review label Oct 16, 2023

prylabs-bulldozer bot added 2 commits October 16, 2023 16:41

Merge refs/heads/develop into deneb-web3signer

69543ab

Merge refs/heads/develop into deneb-web3signer

00099a9

prylabs-bulldozer bot merged commit 493a717 into develop Oct 16, 2023
16 of 17 checks passed

prylabs-bulldozer bot deleted the deneb-web3signer branch October 16, 2023 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deneb - web3signer #12767

Deneb - web3signer #12767

james-prysm commented Aug 21, 2023 •

edited

Loading

prestonvanloon Sep 12, 2023

james-prysm Sep 12, 2023

prestonvanloon Sep 12, 2023

james-prysm Sep 12, 2023

james-prysm Sep 12, 2023

terencechain Sep 20, 2023

james-prysm Sep 26, 2023

james-prysm Sep 27, 2023

nisdas Sep 29, 2023

james-prysm Oct 11, 2023

saolyn Oct 13, 2023

saolyn Oct 13, 2023

saolyn Oct 13, 2023

james-prysm Oct 13, 2023

nisdas Oct 16, 2023

Deneb - web3signer #12767

Deneb - web3signer #12767

Conversation

james-prysm commented Aug 21, 2023 • 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

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

james-prysm commented Aug 21, 2023 •

edited

Loading