feat: make relay bid collection optional via CLI flag#828
Conversation
aloknerurkar
left a comment
aloknerurkar
left a comment
There was a problem hiding this comment.
Also the comment from the previous PR where we need to have one common file for doing the three API calls which can be used by both backfill and forward indexing needs to be addressed. Not sure why you havent already done that yet. It should take a few hours to do at the most.
|
|
||
| for _, bid := range bids { | ||
|
|
||
| logger.Info("[BIDS] processing bids for block", "block", ei.BlockNumber, "slot", ei.Slot) |
There was a problem hiding this comment.
I dont understand why we are still using this [KEY] format in the logs. I have already given comments about this multiple times in the previous PR also. This is not needed. This doesnt help when you are parsing logs. It might be confusing to someone who is new to the codebase. We should be uniform all across. I would suggest remove all the log messages with this type of string.
There was a problem hiding this comment.
i've removed it now, thanks for the same.
| Value: 15 * time.Second, | ||
| }) | ||
|
|
||
| optionRelayFlag = altsrc.NewBoolFlag(&cli.BoolFlag{ |
There was a problem hiding this comment.
Not sure what is relay mode is supposed to mean. This flag is to specify whether we are collecting the bids from relay right? Relay mode is too generalized and confusing.
There was a problem hiding this comment.
i've changed it to RelayData, is that fine?
i've created a new package ingest to address this. |
|
I’ve updated the PR to modify the backfill logic, it now fetches beacon data from the Rated API (which supports batching) and retrieves validator information from the QuickNode API. |
| rows = append(rows, row) | ||
| } | ||
| } | ||
| if len(rows) > 0 { |
There was a problem hiding this comment.
Usually how many rows does this return?
I think maybe it could be better if you have one channel where all these go routines would write the BidRows' and then the batch update happens only once in the end once all goroutines are finished. But not sure how much of a performance gain this would give.
There was a problem hiding this comment.
Usually, it depends for each relay per slot, it’s around 30–40 bids. I’ve also added flags to make the number of relays configurable, so if I batch process them, it could lead to higher memory usage. For example, if there are around 5 relays, that would mean roughly 150–200 bids in total. I can implement batching if needed, but since the bids data is optional and the flag for it defaults to false, I don’t think there would be a significant performance gain. What do you think?
| @@ -1,21 +1,40 @@ | |||
| package config | |||
There was a problem hiding this comment.
I dont really think a separate pkg is needed for this. I dont mind having this but it seems unnecessary just to have a struct definition and default values in separate pkg
There was a problem hiding this comment.
Hey, the config struct is being used across multiple packages and moving it elsewhere would cause circular imports. I could move it to the database package itself doesn’t import anything from config, but conceptually it doesn’t really fit the config isn’t specific to database logic. I could move it there, but in few parts of the repo it has been to keep a separate file:- for example: validators-monitor/config/config.go I don’t have a strong preference, though moving it could improve readability a bit if you think that’s cleaner. What do you think?
.
2 participants
No description provided.