dkg: generate deposit data #491

xenowits · 2022-05-05T19:39:08Z

Add types for DepositData and DepositMessage.
Add HashTreeRoot methods on above types.
Implement marshalling DepositData

category: feature
ticket: #481
feature_set: alpha

leolara · 2022-05-06T04:51:25Z

testutil/eth2/deposit_data.go

@@ -0,0 +1,79 @@
+package eth2


@xenowits why is this in testutil? isn't it used in the normal DKG process?

I asked him to put it here for now, next to keystore, we need to move them both and some other "eth2" related utils somewhere else but I'm not sure where. Waiting for a pattern to emerge.

@xenowits please rename package to deposits

@corverroos don't you think we should avoid the eth2 term as it has become unofficial? Perhaps instead of using eth2 we could use ECL?

as discussed, we are sticking with eth2 for now

corverroos

commented

corverroos · 2022-05-06T06:00:04Z

testutil/eth2/deposit_data.go

@@ -0,0 +1,79 @@
+package eth2


I asked him to put it here for now, next to keystore, we need to move them both and some other "eth2" related utils somewhere else but I'm not sure where. Waiting for a pattern to emerge.

@xenowits please rename package to deposits

corverroos · 2022-05-06T06:02:11Z

go.mod

@@ -22,6 +22,7 @@ require (
 	github.com/multiformats/go-multiaddr v0.5.0
 	github.com/prometheus/client_golang v1.12.1
 	github.com/prysmaticlabs/go-bitfield v0.0.0-20210809151128-385d8c5e3fb7
+	github.com/prysmaticlabs/prysm/v2 v2.0.1


I would avoid importing prysm, it is a HUGE dependency. And we do not need it actually.

corverroos · 2022-05-06T06:02:47Z

testutil/eth2/deposit_data.go

+	}
+
+	domainType := [4]byte{1, 2, 3, 4}
+	domain, err := signing.ComputeDomain(domainType, nil, nil)


we already have this logic, see validatorapi.GetDomain()

you might beed to dig into how eth2client does it and copy inner logic since genesis and forkversion is nil in this case.

Why is forkversion nil in this case?

Mainnet started with genesis_fork_version of 0x00000000, but I think some testnets started with a non-zero genesis fork version, if that's where the nil is coming from.

this is how prysm does it, probably in die spec somewhere. Will confirm

corverroos · 2022-05-06T06:03:24Z

testutil/eth2/deposit_data.go

+}
+
+// GenerateDepositData generates a deposit data object by populating the required fields.
+func GenerateDepositData(def cluster.Definition, pubkey string) (DepositData, error) {


pass in only what you need. You only use withdrawal address and forkversion from the definition, rather just pass them in directly.

corverroos · 2022-05-06T06:04:12Z

testutil/eth2/deposit_data.go

+	case "00000064":
+		return "gnosis"
+	default:
+		return "mainnet"


not sure defaulting to mainnet is correct. Rather default to "unknown"

i'm not sure either but sure will default to unknown

No no! Jim McDonald and others pushed to change defaults to mainnet instead of testnet, to reduce the chance of an invalid deposit on mainnet. An unknown/non-mainnet default could blow up lots of money. Sending a mainnet deposit data to testnet blows up monopoly money. (I once blew up 120 testnet ether when Jim made this change. Before then ethdo pulled fork_version from connected beacon node, now it defaults to mainnet and you have to explicitly tell it to do something else.

ok sure, mainnet default

corverroos · 2022-05-06T06:06:57Z

testutil/eth2/deposit_data.go

+
+	domainType := [4]byte{1, 2, 3, 4}
+	domain, err := signing.ComputeDomain(domainType, nil, nil)
+	depositMessageRoot, err := signing.ComputeSigningRoot(depositMessage, domain)


we also alreadyhave this, see validatorapi.prepSigningData. You can just copy it.

please, don't copy/paste code if it is possible

corverroos · 2022-05-06T06:09:10Z

testutil/eth2/deposit_data.go

+// GenerateDepositData generates a deposit data object by populating the required fields.
+func GenerateDepositData(def cluster.Definition, pubkey string) (DepositData, error) {
+	amount := uint64(32000000000) // 32 Ether in Gwei
+	depositMessage := DepositMessage{


suggest making DepositMessage unexported, and adding a HashTreeRoot() function to it. See prysm, cluster.Lock and cluster.Defintion how to implement HashTreeRoot

corverroos · 2022-05-06T06:18:38Z

testutil/eth2/deposit_data.go

+		return DepositData{}, errors.Wrap(err, "compute domain")
+	}
+
+	depositDataRoot := ""


same here depositDataRoot needs to be calcualted by a method you need to add to DepositData called HashTreeRoot, see prsym which fields and what order to hash them.

corverroos · 2022-05-06T06:26:02Z

testutil/eth2/deposit_data.go

+}
+
+// GenerateDepositData generates a deposit data object by populating the required fields.
+func GenerateDepositData(def cluster.Definition, pubkey string) (DepositData, error) {


always use proper types, use eth2p0.PublicKey not string

corverroos · 2022-05-06T06:29:23Z

testutil/eth2/deposit_data.go

+	deposit_data := DepositData{
+		PubKey:                pubkey,
+		Amount:                amount,
+		WithdrawalCredentials: def.WithdrawalAddress,


note that withdrawal credentials isn't an address, is looks different to me

Correct, they need to be slightly converted

See the spec here https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/validator.md#eth1_address_withdrawal_prefix

And an example here https://discord.com/channels/930779837218562078/932001790763864094/970934579563204688

We should only support type 0x01 for creation, because they are the only type that exits will be supported for (split-keys shouldn't care/know what the withdrawal creds are)

corverroos · 2022-05-06T06:35:08Z

testutil/eth2/deposit_data.go

+	}
+
+	depositDataRoot := ""
+	signature := "" // sign the depositMsgRoot with the bls key


signing in a distributed validator is not trivial, so we need to restructure this logic, you cannot sign "inside a function".

Suggest splitting into two functions:

deposits.GetMessageRoot(ctx, eth2Cl, pubkey, withdrawalPubKey) eth2p0.Root, error

We will then threshold sign this sig root, and aggregate it to get a the signature, and only then make deposit data.

deposits.GetDepositData(pubkey, withdrawalPubKey, forkversion, messageRoot, messageSig) DepositData, error

corverroos · 2022-05-06T06:54:26Z

testutil/eth2/deposit_message.go

+	idx := hh.Index()
+
+	// Field 0 'PubKey`
+	hh.PutBytes([]byte(d.PubKey))


this is going to fail, since the type of pubkey is a string, so it is really unclear what format should be. Rather make it []byte then we know it is a standard public key bytes format

corverroos · 2022-05-06T06:55:34Z

testutil/eth2/deposit_message.go

+	return b, nil
+}
+
+func (d DepositMessage) HashTreeRootWith(hh *ssz.Hasher) error {


please add tests for this, try to find reference data in prysm. Ie. given a message with values X,Y,Z, we know the hash should be something

corverroos · 2022-05-06T06:56:22Z

testutil/eth2/deposit_message.go

+	hh.PutUint64(d.Amount)
+
+	// Field 2 'WithdrawalCredentials'
+	hh.PutBytes([]byte(d.WithdrawalCredentials))


same here, this should be bytes, also please reference the format spec.

…owits/add-deposit-data

corverroos · 2022-05-08T11:18:54Z

eth2util/deposit/deposit_message.go

+	"github.com/obolnetwork/charon/app/z"
+)
+
+const ETH1_ADDRESS_WITHDRAWAL_PREFIX = "01"


do not export types/interfaces/variables/constants by default, always start unexported

corverroos · 2022-05-08T11:20:10Z

eth2util/deposit/deposit_message.go

+
+	withdrawalCreds := make([]byte, 0, 32)
+
+	prefix, err := hex.DecodeString(ETH1_ADDRESS_WITHDRAWAL_PREFIX)


rather make the constant a []byte, then no need to convert

ok. made it a single byte: byte(1)

corverroos · 2022-05-08T11:21:31Z

eth2util/deposit/deposit_message.go

+		return WithdrawalCredentials{}, errors.New("invalid withdrawal address", z.Str("address", addr))
+	}
+
+	withdrawalCreds := make([]byte, 0, 32)


introduces variables just before they are used

canonical slice instantiation: var creds []byte

avoid micro optimisations (adding length to slice). It draws attention ... to nothing in this case.

Only do micro optimisations if really required, like in some hot path.

corverroos · 2022-05-08T11:24:43Z

eth2util/deposit/deposit_message.go

+	// Append the single byte ETH1_ADDRESS_WITHDRAWAL_PREFIX as prefix.
+	withdrawalCreds = append(withdrawalCreds, prefix...)
+
+	zeroes, err := hex.DecodeString("0000000000000000000000")


same here, make this a global var elevenZeros = []byte{0x00,0x00, ...}

corverroos · 2022-05-08T11:26:00Z

eth2util/deposit/deposit_message.go

+	withdrawalCreds = append(withdrawalCreds, zeroes...)
+
+	// Finally, append 20 bytes of ethereum address.
+	withdrawalCreds = append(withdrawalCreds, addr...)


you are appending a hex string to a byte slice...

codecov · 2022-05-08T16:52:51Z

Codecov Report

Merging #491 (f74eedd) into main (419dbe5) will increase coverage by 0.57%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #491      +/-   ##
==========================================
+ Coverage   55.87%   56.44%   +0.57%     
==========================================
  Files          84       87       +3     
  Lines        7578     7979     +401     
==========================================
+ Hits         4234     4504     +270     
- Misses       2770     2874     +104     
- Partials      574      601      +27

Impacted Files	Coverage Δ
app/app.go	`63.31% <0.00%> (-2.08%)`	⬇️
core/qbft/uponrule_string.go	`40.00% <0.00%> (ø)`
core/qbft/qbft.go	`69.74% <0.00%> (ø)`
core/qbft/msgtype_string.go	`50.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 419dbe5...f74eedd. Read the comment docs.

corverroos · 2022-05-09T07:29:06Z

eth2util/deposit/deposit_data.go

+	var version eth2p0.Version
+	copy(version[:], forkVersion)
+
+	depositData := DepositData{


inline this: return DepositData{...}, nil

corverroos · 2022-05-09T07:30:13Z

eth2util/deposit/deposit_data.go

+}
+
+// GenerateDepositData generates a deposit data object by populating the required fields.
+func GenerateDepositData(addr string, forkVersion string, pubkey eth2p0.BLSPubKey) (DepositData, error) {


nit: NewDepositData

Also add signature as parameter

Another idea is to skip the DepositData struct and go stright to raw json bytes:

// MarshalDepositData returns the serialised json deposit data bytes to be written to disk. func MarshalDepositData(pubkey eth2p0.BLSPubKey, withdrawalAddr string, forkVersion string, signature []byte) []byte, error

the idea to skip the structs obviously seems very simple. But, i think that'd be oversimplifying it. We're not doing anything complex here so we can keep the types for the time being. (Pro tip: makes tree hashing easier)

we can get rid of the unmarshal functions though since we only need to marshal and save to disk

corverroos · 2022-05-09T07:32:08Z

eth2util/deposit/deposit_message.go

+
+// WithdrawalCredentials is the 0x01 withdrawal credentials. See spec:
+// https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/validator.md#withdrawal-credentials
+type WithdrawalCredentials [32]byte


type not required, can just use [32]byte directly

I wouldn't oppose a type, and maybe an assertion that the first byte is 01, along with maybe some 0x prepend handling.

Overkill though?

corverroos · 2022-05-09T07:36:06Z

eth2util/deposit/deposit_message.go

+	// Append 11 bytes of 0.
+	withdrawalCreds = append(withdrawalCreds, elevenZeroes...)
+
+	addrBytes, err := hex.DecodeString(addr)


might have "0x" prefix, trim it first: addr = strings.TrimPrefix(addr, "0x")

Is this appending or prepending the zeros? Because it should be at the start of the string.

E.g. 0x01 + 00000000000 + %s

corverroos · 2022-05-09T07:36:55Z

eth2util/deposit/deposit_message.go

+	return string(credentials[12:]), nil
+}
+
+func (d depositMessage) HashTreeRoot() ([32]byte, error) {


colocate type and its methods

corverroos · 2022-05-09T07:37:47Z

eth2util/deposit/deposit_data.go

+}
+
+// ddFmt is the json formatter for DepositData.
+type ddFmt struct {


nit: ddJSON

corverroos · 2022-05-09T07:39:55Z

eth2util/deposit/deposit_data.go

+
+// ddFmt is the json formatter for DepositData.
+type ddFmt struct {
+	PubKey                []byte `json:"pubkey"`


[]byte is encoded as base64, so this needs to be string

all hex fields should be string

Other option is to create your own wrapper type called type hex []byte

Then implement json.Marshaller and json.Unmarshaller to does hex conversion.

Then just wrap the byte:

json.Marshal(ddJson{ Pubkey: hex(pkBytes), Signature: hex(sig), })

corverroos · 2022-05-09T07:53:07Z

eth2util/deposit/deposit_data.go

+	if err != nil {
+		return DepositData{}, err
+	}
+


TODO(): verify the signature

verify sig using: pubkey == crypto.SigToPub(depositMessageRoot[:], sig)

corverroos · 2022-05-09T15:48:49Z

eth2util/deposit/deposit_data.go

+}
+
+// MarshalDepositData returns the json serialised deposit data bytes to be written to disk.
+func MarshalDepositData(pubkey eth2p0.BLSPubKey, msgRoot eth2p0.Root, sig eth2p0.BLSSignature, withdrawalAddr, forkVersion string) ([]byte, error) {


maybe rather remove msgRoot, since you can calculate it

i'm thinking along the lines of if we have a sig (that's calculated on msgRoot), we already have a msgRoot. So, we could have that as an input parameter.

corverroos · 2022-05-09T15:49:23Z

eth2util/deposit/deposit_data.go

+	hh.PutUint64(uint64(d.Amount))
+
+	// Field 2 'Eth1WithdrawalAddress'
+	hh.PutBytes([]byte(d.Eth1WithdrawalAddress))


this should probably be credentials, not address

correct, also changed the ordering of fields according to spec: https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#depositdata

corverroos · 2022-05-09T15:52:20Z

eth2util/deposit/deposit_data.go

+
+	// construct DepositData and then calculate the hash.
+	var version eth2p0.Version
+	copy(version[:], forkVersion)


forkVersion is hex string, you need to convert to bytes first

corverroos · 2022-05-09T15:53:29Z

eth2util/deposit/deposit_data.go

+const depositAmt = 32000000000
+
+// DepositData contains all the information required for activating validators on the Ethereum Network.
+type DepositData struct { //nolint:revive


unexport this

add basic structure for deposit data

1006053

xenowits marked this pull request as draft May 5, 2022 19:39

leolara reviewed May 6, 2022

View reviewed changes

corverroos reviewed May 6, 2022

View reviewed changes

corverroos added the do not merge Indicate to bulldozer bot that this PR should not be merged label May 6, 2022

corverroos reviewed May 6, 2022

View reviewed changes

add functions for hash tree root

ad505ee

corverroos reviewed May 6, 2022

View reviewed changes

xenowits added 4 commits May 6, 2022 17:42

Merge branch 'main' of https://github.com/ObolNetwork/charon into xen…

6bc4d12

…owits/add-deposit-data

remove prysm

cc95ae5

Merge branch 'main' of https://github.com/ObolNetwork/charon into xen…

94697fe

…owits/add-deposit-data

add tree hashing and json serialization

1ff4ba7

corverroos reviewed May 8, 2022

View reviewed changes

xenowits added 2 commits May 8, 2022 18:21

fix deposit message code

68edcad

add final types

4325cc8

add nolint revive to depositdata

ba1c580

xenowits marked this pull request as ready for review May 8, 2022 17:00

corverroos reviewed May 9, 2022

View reviewed changes

address review comments

368d967

xenowits mentioned this pull request May 9, 2022

deposit: save deposit data to disk #502

Closed

OisinKyne added this to the Devnet 1 milestone May 9, 2022

corverroos reviewed May 9, 2022

View reviewed changes

corverroos approved these changes May 9, 2022

View reviewed changes

xenowits added 2 commits May 9, 2022 21:54

reordered hash fields according to specs

2f030c5

fix golangci lint error

f74eedd

xenowits added merge when ready Indicates bulldozer bot may merge when all checks pass and removed do not merge Indicate to bulldozer bot that this PR should not be merged labels May 9, 2022

obol-bulldozer bot merged commit 660e8a2 into main May 9, 2022

obol-bulldozer bot deleted the xenowits/add-deposit-data branch May 9, 2022 16:30


		withdrawalCreds := make([]byte, 0, 32)

		prefix, err := hex.DecodeString(ETH1_ADDRESS_WITHDRAWAL_PREFIX)

dkg: generate deposit data #491

dkg: generate deposit data #491

Conversation

xenowits commented May 5, 2022 • edited Loading

Choose a reason for hiding this comment

corverroos May 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos left a comment

Choose a reason for hiding this comment

corverroos May 6, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corverroos May 6, 2022 • 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

corverroos May 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 8, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xenowits May 9, 2022 • 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

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

xenowits commented May 5, 2022 •

edited

Loading

corverroos May 6, 2022 •

edited

Loading

corverroos May 6, 2022 •

edited

Loading

corverroos May 6, 2022 •

edited

Loading

corverroos May 8, 2022 •

edited

Loading

codecov bot commented May 8, 2022 •

edited

Loading

xenowits May 9, 2022 •

edited

Loading