Hex encode graffiti in transit when calling produce block APIs - #5109
Conversation
b186523 to
4c67741
Compare
According to beacon node API specification the graffiti should be a hex encoded string and have a length of 66 characters. https://ethereum.github.io/beacon-APIs/#/ValidatorRequiredApi/produceBlockV2 https://github.com/ethereum/beacon-APIs/blob/181575da112fe6556e48a5b7e0ad4a2154e95da0/types/primitive.yaml#L74
4c67741 to
2623842
Compare
| const ZERO_HASH_HEX = "0x" + ZERO_HASH.toString("hex"); | ||
| const randaoReveal = Buffer.alloc(96, 1); | ||
| const graffiti = "0xcf8e0d4e9587369b2301d0790347320302cc0943d5a1884560367e8208d920f2"; | ||
| const graffiti = "a".repeat(32); |
There was a problem hiding this comment.
0xcf8e0d4e9587369b2301d0790347320302cc0943d5a1884560367e8208d920f2 this value did not make too much sense in the first place, it is from the produceBlockV2 which makes sense there becasue it is is the value which should be passed to the beacon API but in our case it should be the actual validator graffiti value which is a UFT-8 string and not hex encoded.
The runGenericServerTest function currently uses the args as input to the api client and it uses that same value to check if it was received by the mock api. This means at the moment we generally assume we sent some value do some kind of encoding in writeReq and then decode in parseReq without modyfing the value.
The problem is that toGraffitiHex will trim or pad the value depending on its length, meaning the graffiti needs to be exactly 32 bytes for the test to pass.
philknows
added
the
meta-breaking-change
Providing an invalid graffiti hex string to the beacon node API would throw an error. To prevent errors when proposing a block we are replacing the malformed graffiti with an empty one.
55e6799 to
2243338
Compare
wemeetagain
left a comment
wemeetagain
left a comment
There was a problem hiding this comment.
LGTM, will wait for another ack for this one before merging
g11tech
left a comment
g11tech
left a comment
There was a problem hiding this comment.
can we add unit tests for to/from graffitiHex for normal and special character strings?
|
also check if we require similar change in keymanager api / external signer api |
added some unit test, those should cover all edge cases but let me know if I missed something |
did not find any references to the graffiti in the keymanager api spec or any other place in the code where we should apply graffiti encoding/decoding |
|
馃帀 This PR is included in v1.6.0 馃帀 |
4 participants
Motivation
According to beacon node API specification the graffiti should be a hex encoded string, see produceBlockV2 and have a length of 66 characters.
Issue highlighted by @dB2510 in discord.
Description
Hex encodes graffiti when writing
produceBlockrequest and decodes it when parsing it on the beacon node. The functions for converting a uft8 string to hex and vice-versa are browser compatible.If the graffiti is too long, it removes characters from the end and if it is too short, the hex value will be right-padded with zeros.
Could consider to explicitly fail validator on start if configured graffiti is too long instead of implicitly trimming it but should be discussed first if we want this behavior.