Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix race caused by design of native Neo validators/committee cache #3110

Merged
merged 7 commits into from
Oct 10, 2023
Merged

Fix race caused by design of native Neo validators/committee cache #3110

merged 7 commits into from
Oct 10, 2023

Commits on Oct 10, 2023

  1. native: rework native NEO next block validators cache 

    Blockchain passes his own pure unwrapped DAO to
(*Blockchain).ComputeNextBlockValidators which means that native
RW NEO cache structure stored inside this DAO can be modified by
anyone who uses exported ComputeNextBlockValidators Blockchain API,
and technically it's valid, and we should allow this, because it's
the only purpose of `validators` caching. However, at the same time
some RPC server is allowed to request a subsequent wrapped DAO for
some test invocation. It means that descendant wrapped DAO
eventually will request RW NEO cache and try to `Copy()`
the underlying's DAO cache which is in direct use of
ComputeNextBlockValidators. Here's the race:
ComputeNextBlockValidators called by Consensus service tries to
update cached `validators` value, and descendant wrapped DAO
created by the  RPC server tries to copy DAO's native cache and
read the cached `validators` value.

So the problem is that native cache not designated to handle
concurrent access between parent DAO layer and derived (wrapped)
DAO layer. I've carefully reviewed all the usages of native cache,
and turns out that the described situation is the only place where
parent DAO is used directly to modify its cache concurrently with
some descendant DAO that is trying to access the cache. All other
usages of native cache (not only NEO, but also all other native
contrcts) strictly rely on the hierarchical DAO structure and don't
try to perform these concurrent operations between DAO layers.
There's also persist operation, but it keeps cache RW lock taken,
so it doesn't have this problem as far. Thus, in this commit we rework
NEO's `validators` cache value so that it always contain the relevant
list for upper Blockchain's DAO and is updated every PostPersist (if
needed).

Note: we must be very careful extending our native cache in the
future, every usage of native cache must be checked against the
described problem.

Close #2989.

Signed-off-by: Anna Shaleva <[email protected]>
    @AnnaShaleva
    AnnaShaleva committed Oct 10, 2023
    Configuration menu
    Copy the full SHA
    96449d8 View commit details
    Browse the repository at this point in the history

  2. core: rename (*Blockchain).GetValidators to ComputeNextBlockValidators 

    We have two similar blockchain APIs: GetNextBlockValidators and GetValidators.
It's hard to distinguish them, thus renaming it to match the meaning, so what
we have now is:

GetNextBlockValidators literally just returns the top of the committee that
was elected in the start of batch of CommitteeSize blocks batch. It doesn't
change its valie every block.

ComputeNextBlockValidators literally computes the list of validators based on
the most fresh committee members information got from the NeoToken's storage
and based on the latest register/unregister/vote events. The list returned by
this method may be updated every block.

Signed-off-by: Anna Shaleva <[email protected]>
    @AnnaShaleva
    AnnaShaleva committed Oct 10, 2023
    Configuration menu
    Copy the full SHA
    07e1bc7 View commit details
    Browse the repository at this point in the history

  3. native: don't recalculate validators every block 

    Recalculate them once per epoch. Consensus is aware of it and must
call CalculateNextValidators exactly when needed.

Signed-off-by: Anna Shaleva <[email protected]>
    @AnnaShaleva
    AnnaShaleva committed Oct 10, 2023
    Configuration menu
    Copy the full SHA
    312534a View commit details
    Browse the repository at this point in the history

  4. native: rename NEO's cached validators list to `newEpochNextValidat… 

    …ors`

Adjust all comments, make the field name match its meaning.

Signed-off-by: Anna Shaleva <[email protected]>
    @AnnaShaleva
    AnnaShaleva committed Oct 10, 2023
    Configuration menu
    Copy the full SHA
    794658b View commit details
    Browse the repository at this point in the history

  5. native: refactor argument of NEO's getCommitteeMembers function 

    No funcional changes, just refactoring. It doesn't need the whole cache,
only the set of committee keys with votes.

Signed-off-by: Anna Shaleva <[email protected]>
    @AnnaShaleva
    AnnaShaleva committed Oct 10, 2023
    Configuration menu
    Copy the full SHA
    27dddb4 View commit details
    Browse the repository at this point in the history

  6. native: optimize NEO's committee/validators cache handling 

    Do not recalculate new committee/validators value in the start of every
subsequent epoch. Use values that was calculated in the PostPersist method
of the previously processed block in the end of the previous epoch.

Signed-off-by: Anna Shaleva <[email protected]>
    @AnnaShaleva
    AnnaShaleva committed Oct 10, 2023
    Configuration menu
    Copy the full SHA
    f78f915 View commit details
    Browse the repository at this point in the history

  7. native: make newEpochNextValidators always contain non-empty value 

    If it's the end of epoch, then it contains the updated validators list recalculated
during the last block's PostPersist. If it's middle of the epoch, then it contains
previously calculated value (value for the previous completed epoch) that is equal
to the current nextValidators cache value.

Signed-off-by: Anna Shaleva <[email protected]>
    @AnnaShaleva
    AnnaShaleva committed Oct 10, 2023
    Configuration menu
    Copy the full SHA
    d964420 View commit details
    Browse the repository at this point in the history