-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(drand): add null HistoricalBeaconClient for old beacons #12830
base: master
Are you sure you want to change the base?
Conversation
Checked out this branch, and tried to see if I still encountered the #11802 issue. It got a bit longer then previous:
It then looped on the
|
OK, well I don't think the drand errors were your fatal problem here it's just looping in the background and logging those entries. But unfortunately we're stuck with the optimizing client: https://github.com/drand/drand/blob/v1.5.11/client/optimizing.go#L194, so perhaps we can't take this approach at all and need it fixed inside drand otherwise it's going to continue to speed test the null-client and spam those info log messages. I think your problem here is that you can't use a snapshot file when trying to import the full "chain". At least that's my understanding from reading the code - the main difference between importing a snapshot and importing the chain is that a chain import does a full tipset validation back to genesis, which is why it wants the beacons to be available. It calls I suppose a chain export is a |
Updated this branch with a modification that attempts to use the "watcher" mechanism in drand because watchers don't get speed tested, so if I can convince it that my historical null-client is a watcher then maybe it'll not speed test it. I haven't tested this but @rjan90 if you still have that snapshot handy, would you mind running it again to see if you get any of those INFO logs or any other errors from drand? |
Reran with the latest commit, and this is what I get:
|
cf963a5
to
30ca496
Compare
OK, one tiny change and this seems to work; I grabbed my own snapshot and have been running it and the only logs are the badger ones so I believe this is solved, even if it is a hack. I'd still like @AnomalRoil to take a look and suggest alternative approaches. Marking @Kubuxu as reviewer since he's touched a lot of this stuff previously. |
Some feedback is in https://filecoinproject.slack.com/archives/C047Z15JNEA/p1738587544472029 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments.
30ca496
to
c0fad9d
Compare
Attempted to upgrade to go-clients and drand/v2 while I was at this but we fail our new dependency check because go-clients happens to pull in these for some reason:
|
argh, and a slight upgrade to urfave/cli has made docsgen-cli change, I might have to back that one out |
urfave/cli has an OK change, but a bug in that particular release, a couple more patch levels and it's good, so I've done that here and checked in the changes, I'm just not sure about the go-spew and go-difflib untagged inclusions, it might be nice to track down where those are used |
go-clients -> github.com/hashicorp/consul/[email protected] pulls in both of these, but both of those are indirect too https://github.com/hashicorp/consul/blob/main/sdk/go.mod |
Will revisit this next week: If drand/go-clients#10 isn't addressed I might wind back to my previous version here of implementing my own fake client but keep the drand/v2 upgrade here and mark the untagged transient dependencies as OK. |
I think drand/go-clients#11 resolves this. The empty client already propagates an error via |
153b2cc
to
f417457
Compare
This should be ready now. @Kubuxu would you mind taking a look? |
🤦 this pulls us from a minimum of Go 1.22.7 to 1.22.10. But hopefully we're going to bump that to 1.23 soon anyway. |
Fixes: #11802
Ref: #11802 (comment)
I'm pretty sure this will work since we only need to verify, not fetch, old beacon entries. So when we remove
Servers
from the config (like we've done for Mainnet and Incentinet), this will satisfy drand but also be a noop.