Implement the endpoint GET /v2/block-headers

[agodnic]

Summary

This pull request implements the endpoint GET /v2/block-headers (ported from AlgoNode#10).

In particular, this enables searching for blocks by different participation-related parameters (i.e.: proposer address, expired accounts, absent accounts).

Test Plan

The new endpoint has been tested manually, using the FNet dataset:

# no parameters
curl -Ss 'http://127.0.0.1:8980/v2/block-headers?limit=2' | jq

# filter by timestamp
curl -Ss 'http://127.0.0.1:8980/v2/block-headers?limit=2&after-time=2024-10-01T00:00:00Z' | jq

# filter by block proposer
curl -Ss 'http://127.0.0.1:8980/v2/block-headers?proposer=FNETC46DGTSSDHA6C54FWEGZ3Z5ADE4YAPYQTX5VEE2YGU5NFPAWTRANCE&limit=1' | jq

# filter by absent participation accounts
curl -Ss 'http://127.0.0.1:8980/v2/block-headers?absent=WXN65H5M7BRSABP545OES2NCMFGE7R7OICDZSXCTP5H5H2CHXCDKFQTF6U&limit=1' | jq

# filter by expired participation accounts
curl -Ss 'http://127.0.0.1:8980/v2/block-headers?expired=2D2HFM3EIYFNMBNLWZPK5E4SWVN6SR7RDSVCBQ4Z6HRHE26DBT6WQYO6XA&limit=1' | jq

So far there are no automated tests for this feature. Maybe the tests could look like these (mutatis mutandis):

• indexer/api/handlers_test.go

  Line 30 in e6a823d

  func TestTransactionParamToTransactionFilter(t *testing.T) {

• indexer/api/handlers_test.go

  Line 226 in e6a823d

  func TestValidateTransactionFilter(t *testing.T) {

• indexer/api/handlers_test.go

  Line 407 in e6a823d

  func TestFetchTransactions(t *testing.T) {

Performance

The underlying SQL query has been optimized to create efficient execution plans in CockroachDB. This optimization made the code harder to comprehend, it could probably be simpler for the PostgreSQL case.

[CLAassistant]

All committers have signed the CLA.

[jannotti]

Since there are no transactions, the endpoint might better be called blockheaders. I think that's what the similar algod endpoint under development is called.

[agodnic]

The route was changed to GET /v2/block-headers, in a302e5b

[jannotti]

This seems like a weird catch-all. I guess the intent is for block explorers that want to show any relevant block for particular accounts?

[agodnic]

I believe so. Maybe @urtho can give more context on how the participation parameter is intended to be used.

Performance-wise, it is the most problematic parameter. I can see the standard version of the indexer not including the participation parameter (I regard it as a low benefit / high cost feature)

Something similar happens with the updates parameter, maybe not as bad.

Moreover, the underlyng SQL query can be significantly simpler if these two parameters are not shipped.

[jannotti]

Is there any meaningful advantage to it, compared to calling the endpoint three times, specifying the address for each of the three things? I would guess you're using an index for each, so it's basically three index lookups no matter how you slice it.

These lists (expired and suspended) are almost always empty. It seems like an index lookup would be super fast. Just do two of them?

[agodnic]

Is there any meaningful advantage to it, compared to calling the endpoint three times, specifying the address for each of the three things? I would guess you're using an index for each, so it's basically three index lookups no matter how you slice it.

These lists (expired and suspended) are almost always empty. It seems like an index lookup would be super fast. Just do two of them?

That's exactly right. The essential functionality is already being exposed by the "atomic" parameters (that is: proposer, absent and expired)

The data returned by the updates and participation parameters could be obtained by calling the endpoint multiple times using the "atomic" parameters.

[urtho]

I went overboard with my initial requirements and there has been no feedback so far.
I am totally for making the queries/implementation simpler, by only allowing querying for one of the cases (proposer/abset/expired) at a time.

[agodnic]

Addressed in 8cb90c1

[jannotti]

I haven't written much indexer code, but this name seems a bit underspecified. Above, we see txnRowToTransaction, so this should probably be hdrRowToBlock. I'm not totally confident about this recommendation because I would have expected this to take some sort of idb type, rather than an sdk type.

[agodnic]

That's fair, the name hdrRowToBlock will generate less confusion when reading the code.

Addressed in eaefec7 and 8924c97

[jannotti]

I see that decodeAddress already existed, so this PR is not to blame for the unusual way that multiple errors are accumulated, but I think there is a better way to handle multiple errors these days.

I would use sdk.DecodeAddress directly, and if it returns an error, append that into a []error instead of []string. Use fmt.Errorf with a %w to produce an error that wraps the error from sdk.DecodeAddress but also contains the str that would have been passed into decodeAddress. Then at the end of a function that may have accumulated errors, test its size just as you do, but then use errors.Join(errArr...) to combine them into a single error.

I'll leave it to others to decide if this is important enough to change. I believe it will make it easier to follow the error handling, but it isn't critical if this is a common pattern in indexer.

[jannotti]

Oh, even nicer:
Join returns an error that wraps the given errors. Any nil error values are discarded. Join returns nil if every value in errs is nil.
therefore, you don't need to test the err that is returned from sdk.DecodeAddress, just append it to your error list. Then, at the bottom, don't check to see if the list has errors in it, just:

  err := errors.Join(errList...)
  if err != nil {
    return err
  }

[agodnic]

See #1638 (comment)

[jannotti]

slices.Sort is a more convenient, and usually faster, way to sort.

[agodnic]

Addressed in 82e864b.

There are more instances of the sort.Slice function being used, but I don't want to create scope creep. Can totally do this change (potentially in a separate PR) if you guys want.

[jannotti]

As an example of what I described earlier, I think this reads much better, without needing to understand the unusual error handling model of decodeAddress.

Suggested change

	creator, errorArr := decodeAddressToBytes(params.Creator, "creator", make([]string, 0))
	if len(errorArr) != 0 {
	return idb.AssetsQuery{}, errors.New(errUnableToParseAddress)
	creator, err := sdk.DecodeAddress(params.Creator)
	if err {
	return idb.AssetsQuery{}, fmt.Errorf("Unable to parse creator address: %v", err)

It's worth noting that the existing code supplies the word "creator" to decodeAddressToBytes in order to get a nicer error message, and then immediately throws that nice error message away in favor of a generic message.

You can also just use creator[:] in the code below, rather than have separate functions to decode to an sdk.Address, or to []byte.

[agodnic]

This change makes a lot of sense:

• removes unnecessary code (there are two functions that pretty much do the same thing as sdk.DecodeAddress)
• the logic in those functions is somewhat confusing to the reader
• handling errors as []error is preferrable over []string

Commit 7f26e0f adds more context information to the errors being returned. If this is considered a breaking change, I can make it return the same exact old error messages.

I'm not a fan of returning multiple errors, I think it is not worth the extra complexity compared to a fail-fast approach (returning upon the first error encountered). In fact this could introduce bugs, since the control flow keeps advancing with variables that may be in an unknown state (I'd rather stop the control flow immediately upon finding something that doesn't check out).
But there may be a reason why multiple errors are being used that I'm not aware of.

There are other instances in the codebase that are handling multiple errors as []string instead of []error. I didn't change all of these to avoid scope creep, but could do so in a separate PR.