gridcoin-community / Gridcoin-Research

Gridcoin-Research
MIT License
585 stars 173 forks source link

diag: fix researcher mode check #2725

Closed div72 closed 6 months ago

div72 commented 7 months ago

Fixes a non-deterministic crash that can happen due to a std::optional being dereferenced when it has no value: https://github.com/gridcoin-community/Gridcoin-Research/blob/8477d94403f168d7fdb189654b98a2fa4da5615f/src/wallet/diagnose.h#L536

Sadly it's non-deterministic, as dereferencing a non-valued std::optional is UB; compilers/stdlib is allowed to not have any checks. It results in a randomized garbage CPID being read on the machine I'm currently on.

jamescowens commented 7 months ago

Your commit does not address the comment above?

jamescowens commented 7 months ago

A fix for your comment could be

        const GRC::BeaconRegistry& beacons = GRC::GetBeaconRegistry();
        if (const GRC::CpidOption cpid = GRC::Researcher::Get()->Id().TryCpid()) {
            if (const GRC::BeaconOption beacon = beacons.Try(*cpid)) {
                if (!beacon->Expired(GetAdjustedTime())) {
                    return true;
                }
                for (const auto& beacon_ptr : beacons.FindPending(*cpid)) {
                    if (!beacon_ptr->Expired(GetAdjustedTime())) {
                        return true;
                    }
                }
            }
        }
        return false;
div72 commented 7 months ago

Your commit does not address the comment above?

The function is not called in the first case if the CPID is null with the commit. I can add another check with a warning if you want.

It'd the best if we had a linter which enforced this.

jamescowens commented 7 months ago

I understand that, but it is more correct to fix the behavior at the source. *cpid should be guarded.

Your actual commit is of course correct.

We should do something with the linter for this if possible.