fix(drand): add null HistoricalBeaconClient for old beacons #12830
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: |
rvagg
force-pushed
the
rvagg/historical-beacon-client
branch
2 times, most recently
from
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 |
chain/beacon/drand/drand.go
Outdated
| var _ dclient.Client = historicalBeaconClient{} | ||
|
|
||
| // historicalBeaconClient is a drand client that doesn't actually do anything. It's used when | ||
| // we don't have a drand network to connect to but still need to provide a beacon client. | ||
| // We don't expect calls through to the client to be made since we should only be verifying old | ||
| // randomness, not fetching it. | ||
| type historicalBeaconClient struct{} | ||
|
|
||
| func (h historicalBeaconClient) Get(ctx context.Context, round uint64) (dclient.Result, error) { | ||
| return nil, xerrors.Errorf("no historical randomness available") | ||
| } | ||
|
|
||
| func (h historicalBeaconClient) Watch(ctx context.Context) <-chan dclient.Result { | ||
| return nil | ||
| } | ||
|
|
||
| func (h historicalBeaconClient) Info(ctx context.Context) (*dchain.Info, error) { | ||
| return nil, xerrors.Errorf("no historical randomness available") | ||
| } | ||
|
|
||
| func (h historicalBeaconClient) RoundAt(time.Time) uint64 { | ||
| return 0 | ||
| } | ||
|
|
||
| func (h historicalBeaconClient) Close() error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I'm very surprised the Empty client isn't working for your needs?
There was a problem hiding this comment.
ahh, TIL about EmptyClientWithInfo, but yes, we still have the log problem with it, see below
chain/beacon/drand/drand.go
Outdated
| opts = append(opts, dclient.WithWatcher(func(chainInfo *dchain.Info, cache dclient.Cache) (dclient.Watcher, error) { | ||
| return historicalClient, nil |
There was a problem hiding this comment.
I think the trick is to have your client's Get return a drand.ErrEmptyClientUnsupportedGet error to avoid having to have a "valid" client in Wrap.
There was a problem hiding this comment.
Looking into this, it's possible we're not properly "supporting" that special error in the testSpeed function.
I could change it if it'd help, so that we're not reporting the
oc.log.Infow("", "optimizing_client", "endpoint down when speed tested", ...
log line when it sees that error.
There was a problem hiding this comment.
yep, leave it long enough and we get that:
2025-02-04T16:29:52.276+1100 INFO drand client/optimizing.go:194 {"optimizing_client": "endpoint down when speed tested", "client": "EmptyClient.(+verifier)", "err": "not supported"}
2025-02-04T16:29:52.276+1100 INFO drand client/optimizing.go:194 {"optimizing_client": "endpoint down when speed tested", "client": "EmptyClient.(+verifier)", "err": "not supported"}
Opened drand/go-clients#10 for this
rvagg
force-pushed
the
rvagg/historical-beacon-client
branch
from
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/sdk@v0.16.1 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. |
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
Serversfrom the config (like we've done for Mainnet and Incentinet), this will satisfy drand but also be a noop.