[Merged by Bors] - Add graffiti cli flag to the validator client. #1425

realbigsean · 2020-07-31T00:31:36Z

Issue Addressed

Proposed Changes

Creates a --graffiti cli flag in the validator client. If the flag is set, it overrides graffiti in the beacon node.

Additional Info

AgeManning · 2020-07-31T01:15:07Z

Curious about your thoughts on this.

As you know the VC asks for a beacon block from the BN. The BN can set the graffiti and that will be the default for block when the VC asks to produce a block.

I was thinking potentially a simpler approach would be like this.
VC -> Give me block -> BN (asks for block from BN)
VC <- Block with default graffiti <- BN (BN sends block with default grafiti - No code modifications on the BN side).
VC Modifies graffiti field of block and signs (we just change a single field on the block before signing)
VC -> Signed Block -> BN (Sends the signed block with the VC graffiti to the BN to propagate).

I think there would be very minimal code changes and hence complexity in this approach.

What do you think?

realbigsean · 2020-07-31T01:48:15Z

Yea that would definitely cut down the code changes, it's a little janky the way I have it. I was trying to avoid modifying the block after producing it though. Was thinking we'd want to keep the relationship more strictly BN --> produce block. VC --> sign block. But if it's not worth the complexity I can update it, just let me know.

AgeManning · 2020-07-31T01:59:53Z

Yeah fair point.

@michaelsproul - What logic do you think we should go with here?

michaelsproul · 2020-07-31T02:53:33Z

Hmm, I can see benefits to both approaches but I'm in favour of keeping it simple and letting the validator client modify the graffiti. It makes sense that the VC has "control" over what it signs, including mutating the block if it wants to.

Thanks @realbigsean for fixing this pain point ☺️

This reverts commit d4309b4.

… beacon node.

realbigsean · 2020-07-31T04:14:36Z

Ok, I updated this, but am getting some errors. I'm seeing this log in the beacon node:

Jul 31 03:50:25.411 WARN Invalid block from local validator outcome: StateRootMismatch { block: 0xa0a253636feb79e41b5e218d70adb52df04e597f42682409e94632e72fd2c75b, local: 0xd6858efd8c8fc021127024f570e707b858339af9dd4d2e25f09fcf05696c1908 }, service: http

and this log in the validator client:

Jul 31 03:50:25.412 CRIT Published block was invalid slot: 12, message: The SignedBeaconBlock could not be processed and has not been published: StateRootMismatch { block: 0xa0a253636feb79e41b5e218d70adb52df04e597f42682409e94632e72fd2c75b, local: 0xd6858efd8c8fc021127024f570e707b858339af9dd4d2e25f09fcf05696c1908 }, service: block

I'm not too familiar with how the state root's calculated. But if it incorporates the graffiti, it seems like the beacon node isn't able to validate the signed block correctly because it has a different idea of what the state is.

michaelsproul · 2020-07-31T04:17:38Z

Ah, you're totally right. Let's stick with your original approach, recalculating the state root is expensive. Sorry I didn't think of this before

This reverts commit d4309b4.

This reverts commit 49dc9da.

realbigsean · 2020-07-31T04:33:38Z

ok cool, updated it back 👍

michaelsproul · 2020-07-31T05:42:26Z

validator_client/src/block_service.rs

+        // If graffiti has been set in the validator client, overwrite any graffiti sent from the beacon node.
+        if let Some(graffiti) = self.graffiti {
+            block.body.graffiti = graffiti;
+        }
+


This still seems to be doing things the mutating way, something might have gone awry during the reverting process?

Yep, messed something up trying to revert. Removed this, thanks.

AgeManning · 2020-07-31T07:00:44Z

Another thing we need to concern ourselves is following the eth2 API spec. The VC <-> BN API is supposed to be standardised so we can't ad-hoc modify that API.

As it turns out, the new spec requires the graffiti field. However it's not an Option, but rather a String.

It might be best to conform to the API's here to save us the hassle in the future. See here:
https://ethereum.github.io/eth2.0-APIs/#/Validator/produceBlock

realbigsean · 2020-07-31T14:44:50Z

As it turns out, the new spec requires the graffiti field. However it's not an Option, but rather a String.

Yea so the graffiti is an Option as it's being passed into the produce_block method here: https://github.com/realbigsean/lighthouse/blob/graffiti-cli-option-in-vc/common/remote_beacon_node/src/lib.rs#L316

but in the actual GET request, it's a String query param that may or may not be included:

https://github.com/realbigsean/lighthouse/blob/graffiti-cli-option-in-vc/common/remote_beacon_node/src/lib.rs#L327

Which is in line with the API (the graffiti param is the only of the three not required). There are a couple of other things about this endpoint that are not in line with the API though, like the name is "block" rather than "blocks" and "slot" is a query param rather than a path param, but I'm guessing those would be out of scope for this PR?

AgeManning · 2020-08-01T05:49:16Z

Yep perfect!

Yeah we need to do one big PR to bring us in line with the API specs.

This looks good to me. I'll await @michaelsproul final approval before merging. Thanks!

michaelsproul

LGTM

Are we merging post-Medalla launch?

AgeManning · 2020-08-06T03:55:09Z

Would like to run this on the testnet first

AgeManning · 2020-08-11T01:59:21Z

This looks fine to me. Lets get it in to prevent PR upkeep.

bors r+

## Issue Addressed #1419 ## Proposed Changes Creates a `--graffiti` cli flag in the validator client. If the flag is set, it overrides graffiti in the beacon node. ## Additional Info

bors · 2020-08-11T03:06:00Z

Pull request successfully merged into master.

Build succeeded:

## Issue Addressed #1419 ## Proposed Changes Creates a `--graffiti` cli flag in the validator client. If the flag is set, it overrides graffiti in the beacon node. ## Additional Info

Add graffiti cli flag to the validator client.

d4309b4

realbigsean added 2 commits July 30, 2020 23:15

Revert "Add graffiti cli flag to the validator client."

49dc9da

This reverts commit d4309b4.

Add graffiti cli flag to the validator client, without changes to the…

0f4014c

… beacon node.

realbigsean marked this pull request as draft July 31, 2020 03:52

realbigsean added 2 commits July 31, 2020 00:22

Revert "Add graffiti cli flag to the validator client."

c426a77

This reverts commit d4309b4.

Revert "Revert "Add graffiti cli flag to the validator client.""

41c56a2

This reverts commit 49dc9da.

realbigsean marked this pull request as ready for review July 31, 2020 04:34

michaelsproul requested changes Jul 31, 2020

View reviewed changes

Remove code that should have been reverted previously

16fc749

michaelsproul approved these changes Aug 4, 2020

View reviewed changes

merge master

190fc01

bors bot changed the title ~~Add graffiti cli flag to the validator client.~~ [Merged by Bors] - Add graffiti cli flag to the validator client. Aug 11, 2020

bors bot closed this Aug 11, 2020

torfbolt mentioned this pull request Aug 11, 2020

No graffiti flag for validator client to receive POAP badge for the Medalla testnet #1410

Closed

realbigsean deleted the graffiti-cli-option-in-vc branch August 25, 2020 21:25

[Merged by Bors] - Add graffiti cli flag to the validator client. #1425

[Merged by Bors] - Add graffiti cli flag to the validator client. #1425

Uh oh!

Conversation

realbigsean commented Jul 31, 2020

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

AgeManning commented Jul 31, 2020

Uh oh!

realbigsean commented Jul 31, 2020

Uh oh!

AgeManning commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelsproul commented Jul 31, 2020

Uh oh!

realbigsean commented Jul 31, 2020

Uh oh!

michaelsproul commented Jul 31, 2020

Uh oh!

realbigsean commented Jul 31, 2020

Uh oh!

michaelsproul Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

realbigsean Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

AgeManning commented Jul 31, 2020

Uh oh!

realbigsean commented Jul 31, 2020

Uh oh!

AgeManning commented Aug 1, 2020

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

AgeManning commented Aug 6, 2020

Uh oh!

AgeManning commented Aug 11, 2020

Uh oh!

bors bot commented Aug 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AgeManning commented Jul 31, 2020 •

edited

Loading