Attestation API updates for Electra #6557

pedromiguelmiranda · 2024-09-17T17:03:35Z

New GET and POST v2 versions for block and pool attestations according to spec ethereum/beacon-APIs/pull/448 .

~~New Attester slashing endpoints still need to be implemented, given that they still lack support on code base.~~ pool attester slashings api changes #6585

github-actions · 2024-09-17T18:34:06Z

Unit Test Results

        6 files -       3     900 suites - 449 22m 54s ⏱️ - 30m 20s
  5 149 tests +      5   4 801 ✔️ +      5 348 💤 ±  0 0 ❌ ±0
14 331 runs - 6 873 13 975 ✔️ - 6 825 356 💤 - 48 0 ❌ ±0

Results for commit e298672. ± Comparison against base commit 1feeff4.

♻️ This comment has been updated with latest results.

tersec · 2024-09-18T03:41:56Z

Could use updates to https://github.com/status-im/nimbus-eth2/blob/unstable/ncli/resttest-rules.json

Not sure why 1115e4d was premature?

beacon_chain/rpc/rest_beacon_api.nim

pedromiguelmiranda · 2024-09-18T12:10:29Z

Could use updates to https://github.com/status-im/nimbus-eth2/blob/unstable/ncli/resttest-rules.json

Not sure why 1115e4d was premature?

The reason why it is premature is because these new endpoint versions are not yet implemented in ncli and without those missing developments, the tests will fail if we add the resttest-rules.json.

beacon_chain/rpc/rest_beacon_api.nim

tersec · 2024-09-19T05:42:05Z

Could use updates to https://github.com/status-im/nimbus-eth2/blob/unstable/ncli/resttest-rules.json
Not sure why 1115e4d was premature?

The reason why it is premature is because these new endpoint versions are not yet implemented in ncli and without those missing developments, the tests will fail if we add the resttest-rules.json.

I'm not sure I understand, probably I'm missing something. https://github.com/status-im/nimbus-eth2/blob/unstable/ncli/resttest.nim doesn't really implement any endpoint versions to begin with. It just uses the input from resttest-rules.json to prepare basically arbitrary REST requests:

nimbus-eth2/ncli/resttest.nim

Lines 395 to 486 in e3fcd8b

    
           proc prepareRequest(uri: Uri, 
        
                               rule: JsonNode): Result[tuple[url: string, request: string], 
        
                                                       cstring] = 
        
             let request = rule.getOrDefault("request") 
        
             if isNil(request): 
        
               return err("Missing `request` field") 
        
             let meth = 
        
               block: 
        
                 let jmethod = request.getOrDefault("method") 
        
                 if isNil(jmethod): 
        
                   "GET" 
        
                 else: 
        
                   if jmethod.kind != JString: 
        
                     return err("Field `method` should be string") 
        
                   jmethod.str 
        
             let requestUri = 
        
               block: 
        
                 let juri = request.getOrDefault("url") 
        
                 if isNil(juri): 
        
                   return err("Missing requests' `url`") 
        
                 else: 
        
                   if juri.kind != JString: 
        
                     return err("Field `url` should be string") 
        
                   juri.str 
        
             let requestHeaders = 
        
               block: 
        
                 var default: seq[tuple[key: string, value: string]] 
        
                 let jheaders = request.getOrDefault("headers") 
        
                 if isNil(jheaders): 
        
                   default 
        
                 else: 
        
                   var res: seq[tuple[key: string, value: string]] 
        
                   if jheaders.kind != JObject: 
        
                     return err("Field `headers` should be an object") 
        
                   for key, value in jheaders.fields: 
        
                     if value.kind != JString: 
        
                       return err("Field `headers` element should be only strings") 
        
                     res.add((key, value.str)) 
        
                   res 
        
             let (requestBodyType, requestBodyData) = 
        
               block: 
        
                 let jbody = request.getOrDefault("body") 
        
                 if isNil(jbody): 
        
                   ("", "") 
        
                 else: 
        
                   if jbody.kind != JObject: 
        
                     return err("Field `body` should be object") 
        
                   let btype = jbody.getOrDefault("content-type") 
        
                   if isNil(btype): 
        
                     return err("Field `body.content-type` must be present") 
        
                   if btype.kind != JString: 
        
                     return err("Field `body.content-type` should be string") 
        
                   let bdata = jbody.getOrDefault("data") 
        
                   if isNil(bdata): 
        
                     return err("Field `body.data` must be present") 
        
                   if bdata.kind != JString: 
        
                     return err("Field `body.data` should be string") 
        
                   (btype.str, bdata.str) 
        
             var res = meth & " " & uri.path & requestUri & " HTTP/1.1\r\n" 
        
             res.add("Content-Length: " & Base10.toString(uint64(len(requestBodyData))) & 
        
                     "\r\n") 
        
             if len(requestBodyType) > 0: 
        
               res.add("Content-Type: " & requestBodyType & "\r\n") 
        
             for item in requestHeaders: 
        
               res.add(item.key & ": " & item.value & "\r\n") 
        
             let (hostPresent, datePresent) = 
        
               block: 
        
                 var flag1 = false 
        
                 var flag2 = false 
        
                 for item in requestHeaders: 
        
                   if cmpIgnoreCase(item.key, "host") == 0: 
        
                     flag1 = true 
        
                   elif cmpIgnoreCase(item.key, "date") == 0: 
        
                     flag2 = true 
        
                 (flag1, flag2) 
        
             if not(hostPresent): 
        
               res.add("Host: " & $uri.hostname & "\r\n") 
        
             if not(datePresent): 
        
               res.add("Date: " & httpDate() & "\r\n") 
        
             res.add("\r\n") 
        
             if len(requestBodyData) > 0: 
        
               res.add(requestBodyData) 
        
             ok((uri.path & requestUri, res))

and then compares the results, just using the provided string patterns.

It does not use, for example, beacon_chain/spec/eth2_apis/rest_beacon_calls.nim or any similar API definitions

pedromiguelmiranda · 2024-09-20T10:13:15Z

Could use updates to https://github.com/status-im/nimbus-eth2/blob/unstable/ncli/resttest-rules.json
Not sure why 1115e4d was premature?

The reason why it is premature is because these new endpoint versions are not yet implemented in ncli and without those missing developments, the tests will fail if we add the resttest-rules.json.

I'm not sure I understand, probably I'm missing something. https://github.com/status-im/nimbus-eth2/blob/unstable/ncli/resttest.nim doesn't really implement any endpoint versions to begin with. It just uses the input from resttest-rules.json to prepare basically arbitrary REST requests:

nimbus-eth2/ncli/resttest.nim

Lines 395 to 486 in e3fcd8b

proc prepareRequest(uri: Uri,

rule: JsonNode): Result[tuple[url: string, request: string],

cstring] =

let request = rule.getOrDefault("request")

if isNil(request):

return err("Missing `request` field")

let meth =

block:

let jmethod = request.getOrDefault("method")

if isNil(jmethod):

"GET"

else:

if jmethod.kind != JString:

return err("Field `method` should be string")

jmethod.str

let requestUri =

block:

let juri = request.getOrDefault("url")

if isNil(juri):

return err("Missing requests' `url`")

else:

if juri.kind != JString:

return err("Field `url` should be string")

juri.str

let requestHeaders =

block:

var default: seq[tuple[key: string, value: string]]

let jheaders = request.getOrDefault("headers")

if isNil(jheaders):

default

else:

var res: seq[tuple[key: string, value: string]]

if jheaders.kind != JObject:

return err("Field `headers` should be an object")

for key, value in jheaders.fields:

if value.kind != JString:

return err("Field `headers` element should be only strings")

res.add((key, value.str))

res

let (requestBodyType, requestBodyData) =

block:

let jbody = request.getOrDefault("body")

if isNil(jbody):

("", "")

else:

if jbody.kind != JObject:

return err("Field `body` should be object")

let btype = jbody.getOrDefault("content-type")

if isNil(btype):

return err("Field `body.content-type` must be present")

if btype.kind != JString:

return err("Field `body.content-type` should be string")

let bdata = jbody.getOrDefault("data")

if isNil(bdata):

return err("Field `body.data` must be present")

if bdata.kind != JString:

return err("Field `body.data` should be string")

(btype.str, bdata.str)

var res = meth & " " & uri.path & requestUri & " HTTP/1.1\r\n"

res.add("Content-Length: " & Base10.toString(uint64(len(requestBodyData))) &

"\r\n")

if len(requestBodyType) > 0:

res.add("Content-Type: " & requestBodyType & "\r\n")

for item in requestHeaders:

res.add(item.key & ": " & item.value & "\r\n")

let (hostPresent, datePresent) =

block:

var flag1 = false

var flag2 = false

for item in requestHeaders:

if cmpIgnoreCase(item.key, "host") == 0:

flag1 = true

elif cmpIgnoreCase(item.key, "date") == 0:

flag2 = true

(flag1, flag2)

if not(hostPresent):

res.add("Host: " & $uri.hostname & "\r\n")

if not(datePresent):

res.add("Date: " & httpDate() & "\r\n")

res.add("\r\n")

if len(requestBodyData) > 0:

res.add(requestBodyData)

ok((uri.path & requestUri, res))

and then compares the results, just using the provided string patterns.

It does not use, for example, beacon_chain/spec/eth2_apis/rest_beacon_calls.nim or any similar API definitions

Apologies, confused with different calls on validator interfaces.
Which leads to the source of the error, the tests added in 1115e4 were working and show that there is either a use case missing in beacon api spec or that I'm missing some default interpretation of the spec:

both input parameters (slot and committee_index) are not required. Without a slot, how can we get the correct consensus fork?
Or, should we just assume the default value of uint64 ( 0 ) and use slot 0 ?

tersec · 2024-09-23T00:19:52Z

The existing getPoolAttestations semantics, and the apparently intended semantics of getPoolAttestationsV2, treat each parameter as an optional filter:

if only slot is specified, filter on slot;
if only committee index is specified, filter on committee index;
if both slot and committee index are specified, filter on both; and
if neither is specified, don't filter, and enumerate the entire pool.

The existing getPoolAttestations implementation does this for the non-V2 call already:

nimbus-eth2/beacon_chain/rpc/rest_beacon_api.nim

Lines 1277 to 1303 in bf4abf8

    
           # https://ethereum.github.io/beacon-APIs/#/Beacon/getPoolAttestations 
        
           router.api2(MethodGet, "/eth/v1/beacon/pool/attestations") do ( 
        
             slot: Option[Slot], 
        
             committee_index: Option[CommitteeIndex]) -> RestApiResponse: 
        
             let vindex = 
        
               if committee_index.isSome(): 
        
                 let rindex = committee_index.get() 
        
                 if rindex.isErr(): 
        
                   return RestApiResponse.jsonError(Http400, 
        
                                                    InvalidCommitteeIndexValueError, 
        
                                                    $rindex.error) 
        
                 Opt.some(rindex.get()) 
        
               else: 
        
                 Opt.none(CommitteeIndex) 
        
             let vslot = 
        
               if slot.isSome(): 
        
                 let rslot = slot.get() 
        
                 if rslot.isErr(): 
        
                   return RestApiResponse.jsonError(Http400, InvalidSlotValueError, 
        
                                                    $rslot.error) 
        
                 Opt.some(rslot.get()) 
        
               else: 
        
                 Opt.none(Slot) 
        
             var res: seq[phase0.Attestation] 
        
             for item in node.attestationPool[].attestations(vslot, vindex): 
        
               res.add(item) 
        
             RestApiResponse.jsonResponse(res)

However, this does not address

both input parameters (slot and committee_index) are not required. Without a slot, how can we get the correct consensus fork?

which, indeed. The documentation is clear that it's supposed to be exactly one consensus fork, and this is where V2 is trickier:

The active consensus version to which the data belongs. Required in response so client can deserialize returned json or ssz data more effectively.

is not in the non-V2 getPoolAttestations, so the question doesn't come up.

Or, should we just assume the default value of uint64 ( 0 ) and use slot 0 ?
is not useful or specified, so wouldn't do that.

The most spec-correct way I can see is, when slot isn't specified:

get the current fork
get the first epoch of the current fork (RuntimeConfig object, various other ways, the REST API knows this information)
return only attestations within that epoch. If that means only one slot, so be it. It's a boundary condition, and it's more important to keep the

The active consensus version to which the data belongs. Required in response so client can deserialize returned json or ssz data more effectively.

invariant than to return a few more attestations for a basically debugging endpoint every few months.

tersec · 2024-09-23T22:44:58Z

This came up in ethereum/beacon-APIs#458 and that's basically the conclusion they came to also

beacon_chain/rpc/rest_beacon_api.nim

tersec · 2024-09-24T03:18:07Z

beacon_chain/spec/eth2_apis/eth2_rest_serialization.nim

+          writer.endRecord()
+          stream.getOutput(seq[byte])
+        except IOError:
+          default


default is already a template defined by the Nim standard library, so just as a point of naming, unless there's a quite compelling reason to name a symbol literally default (or one is writing effectively an type-specialized implementation of that template) it's less confusing to use a different name; and

more significantly, it's unclear why at all to define var default: seq[byte] in general and then only use it in the except part of the try/except. One can just directly use default(seq[byte]) here and only incur the runtime cost on the except case.

I do see that various other things in eth2_rest_serialization adopt this pattern, but I don't see any reason to.

Nice one.
If I'm not mistaken "default" is a reserved keyword in several languages. Given that the whole file is using the same nomenclature, I didn't pay much attention to it.

The second point, I would say that the runtime cost is not a problem given that the whole exception mechanism is already a performance hit, but exception handling should be simple and mem allocation/deallocation should never fail.
Regarding your suggestion, template code will be injected/replace in place, however, what are the impacts of allocating a sequence on except scope? any leak possbility or anything that we might be missing nim-wise?

Neverthless, I'm adressing this on #6580

tersec · 2024-09-24T07:01:53Z

New Attester slashing endpoints still need to be implemented, given that they still lack support on code base.

#6579

still doesn't have to be done in this PR though

beacon_chain/consensus_object_pools/attestation_pool.nim

tersec · 2024-09-26T09:37:23Z

#6582 should fill in the rest of attester slashing infrastructure

Pedro Miranda added 3 commits September 17, 2024 00:40

new V2 endpoint for beacon getBlockAttestations

156f0ce

nnew GET endpoint version (V2) for getPoolAttestations

5a667d1

new POST endpoint version (V2) for submitPoolAttestations

65ee98c

remove premature ncli tests

1115e4d

tersec reviewed Sep 18, 2024

View reviewed changes