Fixed missing Root notifications via geyser plugin framework #31180

lijunwangs · 2023-04-13T00:44:56Z

Problem

It is reported that Geyser is missing some Root notifications for slots. #31124
The Root notification is sent from replay_stage's code in handle_votable_bank. https://github.com/solana-labs/solana/blob/master/core/src/replay_stage.rs#L1981. However, the validator does not necessarily vote on every slot on the rooted chain. From @carllin

For instance if the rooted chain is 1->2->3->4

You might only vote on 1 and 4
But when 4 is rooted, 2 is also rooted
But handle_votavle_bank is not called on 2

As result of this, we may miss notifications for slot 2 and 3.

Summary of Changes

Enhanced BankNotification to add NewRootedChain enum to send the chains of parent roots.
Renamed BankNotification::Root -> BankNotification::NewRootBank
Introduced SlotNotification for SlotStatusObserver interfaces to send slot status without Bank.
In the OptimisticallyConfirmedBankTracker notify parents of a new root if these parents were not notified.
Modified and added unit test cases to verify the logic.

Fixes #

codecov · 2023-04-14T17:11:26Z

Codecov Report

Merging #31180 (bf76b14) into master (cc6c454) will decrease coverage by 0.1%.
The diff coverage is 84.6%.

@@            Coverage Diff            @@
##           master   #31180     +/-   ##
=========================================
- Coverage    81.5%    81.5%   -0.1%     
=========================================
  Files         734      734             
  Lines      207175   207248     +73     
=========================================
+ Hits       168965   169000     +35     
- Misses      38210    38248     +38

t-nelson · 2023-04-14T21:44:12Z

rpc/src/optimistically_confirmed_bank_tracker.rs

+        if bank_notification_subscribers.is_none() {
+            return;
+        }
+        for root_bank in bank.clone().parents_inclusive().iter().rev() {


i'm not sure Bank::parents_inclusive() is what we want to use here. it's going to be a slow linked-list walk to build the Vec that it returns and iirc, what's available is limited to what's in the local node's ledger. Bank::ancestors is probably what we want instead.

i think we should also take_while from current root back to the last reported root, reverse that, then notify forward, to reduce considering very old roots that have almost certainly been notified already

Thanks! The code in line 180 uses the same logic. Should we do the same optimization?

Unfortunately -- we cannot use Bank::ancestors -- the notification interfaces requires a Bank for the ancestor

Do we have any assurances that the ancestor roots haven't already been dropped from BankForks?

Great question. Looking into it!

To Tyera's point ancestors() is not safe to use because there could have been new roots made by the time this is called since this is async from replay_stage where the roots are set, and then earlier rooted banks would have been removed from BankForks

What we probably want is to send a Vec<Arc<Bank>> from replay_stage where the root is being set. In set_root We are already building a vector of parents of the current root, which should include all the parents up until the last root https://github.com/AshwinSekar/solana/blob/2efd72b1a1a06a89613232d92f75a8e1c1995a6a/core/src/replay_stage.rs#L2023-L2024. We could just grab those and send them over this channel.

A more detailed questions though is why do we need the actual banks in these notifications? I'm assuming to access AccountsDB? But having the bank N does not guarantee AccountsDB is in the same view as it was during bank N because future roots > N could have been made and cleaned up state

Thanks @carllin -- as pointed by @t-nelson, we can change the notifications by sending the (slot, parent_slot) pair without requiring the bank. Regarding the race condition on the roots on the BankForks -- I wonder why https://github.com/AshwinSekar/solana/blob/2efd72b1a1a06a89613232d92f75a8e1c1995a6a/rpc/src/optimistically_confirmed_bank_tracker.rs#L176 does not have the same concern?

I think the same issue is present there as well :)

i guess we could reduce the scope of this pr to "be consistently broken", rather than uniquely broken? 🙂

Or fix the other place first...?

Or fix the other place first...?

I will fix this issue first and the investigate the other next to reduce the scope.

rpc/src/optimistically_confirmed_bank_tracker.rs

carllin · 2023-04-25T04:49:53Z

For clarity in the future, dense changes like the switches from BankNotification -> SlotNotification could be pulled into a separate PR to make reviewing easier

… avoid race conditions

CriesofCarrots

I think I'm happy with the changes. However, optimistically_confirmed_bank_tracker is getting really difficult to grok. Can you update the PR summary of changes to include more details about the intermediate changes? eg. why are there two different notification types and sender/receivers defined?
Can you also update the module docs as well?

For clarity in the future, dense changes like the switches from BankNotification -> SlotNotification could be pulled into a separate PR to make reviewing easier

Yes, also this.

lijunwangs · 2023-04-25T16:31:14Z

For clarity in the future, dense changes like the switches from BankNotification -> SlotNotification could be pulled into a separate PR to make reviewing easier

The reason for that is it would be more messy to use BankNotification in the SlotStatusObserver interfaces with the changed BankNotification interface.

…lain the workflow

t-nelson · 2023-04-25T19:51:25Z

core/src/replay_stage.rs

+            let rooted_slots_with_parents = if bank_notification_sender
+                .as_ref()
+                .map_or(false, |sender| sender.should_send_parents)
+            {
+                let mut new_chain = rooted_slots.clone();
+                new_chain.push(oldest_parent.unwrap_or(bank.parent_slot()));
+                Some(new_chain)
+            } else {
+                None
+            };


Suggested change

let rooted_slots_with_parents = if bank_notification_sender

.as_ref()

.map_or(false, |sender| sender.should_send_parents)

{

let mut new_chain = rooted_slots.clone();

new_chain.push(oldest_parent.unwrap_or(bank.parent_slot()));

Some(new_chain)

} else {

None

};

let rooted_slots_with_parents = bank_notification_sender

.as_ref()

.map_or(false, |sender| sender.should_send_parents)

.then(|| {

let mut new_chain = rooted_slots.clone();

new_chain.push(oldest_parent.unwrap_or(bank.parent_slot()));

new_chain

};

bool::then()'s a little cleaner

CriesofCarrots

Lgtm with Trent's nit

t-nelson

lijunwangs · 2023-05-03T10:49:42Z

Enhanced BankNotification to add NewRootedChain enum to send the chains of parent roots.
Renamed BankNotification::Root -> BankNotification::NewRootBank
Introduced SlotNotification for SlotStatusObserver interfaces to send slot status without Bank.
In the OptimisticallyConfirmedBankTracker notify parents of a new root if these parents were not notified.
Modified and added unit test cases to verify the logic.

fanatid · 2023-05-03T12:03:03Z

Do you have a plan to make a backport to 1.13/1.14?

t-nelson · 2023-05-08T18:47:52Z

Definitely not 1.13. 1.14 bp probably unavoidable

* Fixed missing Root notifications via geyser plugin framework * Renamed a variable * fmt issue * Do not try the loop if no subscribers. * Addressing some feedback -- passing parent roots from replay_stage to avoid race conditions * clippy issue * Address some reviewing findings * Addressed some feedback from Carl * fix a clippy issue * Added comments on optimistically_confirmed_bank_tracker module to explain the workflow * Addressed Trent's review (cherry picked from commit 7cf50e6) # Conflicts: # core/src/validator.rs # geyser-plugin-manager/src/geyser_plugin_service.rs

@carllin

…backport of #31180) (#31650) Problem It is reported that Geyser is missing some Root notifications for slots. #31124 The Root notification is sent from replay_stage's code in handle_votable_bank. https://github.com/solana-labs/solana/blob/master/core/src/replay_stage.rs#L1981. However, the validator does not necessarily vote on every slot on the rooted chain. From @carllin For instance if the rooted chain is 1->2->3->4 You might only vote on 1 and 4 But when 4 is rooted, 2 is also rooted But handle_votavle_bank is not called on 2 As result of this, we may miss notifications for slot 2 and 3. Summary of Changes Enhanced BankNotification to add NewRootedChain enum to send the chains of parent roots. Renamed BankNotification::Root -> BankNotification::NewRootBank Introduced SlotNotification for SlotStatusObserver interfaces to send slot status without Bank. In the OptimisticallyConfirmedBankTracker notify parents of a new root if these parents were not notified. Modified and added unit test cases to verify the logic.

lijunwangs force-pushed the missing_root_notification branch 2 times, most recently from 6691a1b to 4704831 Compare April 14, 2023 01:35

lijunwangs requested review from CriesofCarrots and carllin April 14, 2023 01:38

lijunwangs force-pushed the missing_root_notification branch from 4704831 to 2edf2f6 Compare April 14, 2023 15:51

lijunwangs requested a review from t-nelson April 14, 2023 17:04

t-nelson reviewed Apr 14, 2023

View reviewed changes

lijunwangs force-pushed the missing_root_notification branch from 155b930 to 0d9b3ef Compare April 24, 2023 06:01

carllin reviewed Apr 25, 2023

View reviewed changes

lijunwangs added 8 commits April 25, 2023 01:11

Fixed missing Root notifications via geyser plugin framework

b7f1a72

Renamed a variable

a479089

fmt issue

83b1b59

Do not try the loop if no subscribers.

c012799

Addressing some feedback -- passing parent roots from replay_stage to…

1b15499

… avoid race conditions

clippy issue

d21d762

Address some reviewing findings

8e6b745

Addressed some feedback from Carl

e343bf7

lijunwangs force-pushed the missing_root_notification branch from 2eb627b to e343bf7 Compare April 25, 2023 08:12

fix a clippy issue

4a549d6

CriesofCarrots reviewed Apr 25, 2023

View reviewed changes

Added comments on optimistically_confirmed_bank_tracker module to exp…

253e4b2

…lain the workflow

t-nelson reviewed Apr 25, 2023

View reviewed changes

Addressed Trent's review

bf76b14

carllin previously approved these changes Apr 27, 2023

View reviewed changes

CriesofCarrots previously approved these changes Apr 27, 2023

View reviewed changes

lijunwangs dismissed stale reviews from CriesofCarrots and carllin via bf76b14 April 28, 2023 12:43

t-nelson approved these changes Apr 29, 2023

View reviewed changes

lijunwangs merged commit 7cf50e6 into solana-labs:master May 3, 2023
23 checks passed

lijunwangs added the v1.14 label May 15, 2023

mergify bot mentioned this pull request May 15, 2023

v1.14: Fixed missing Root notifications via geyser plugin framework (backport of #31180) #31650

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed missing Root notifications via geyser plugin framework #31180

Fixed missing Root notifications via geyser plugin framework #31180

lijunwangs commented Apr 13, 2023 •

edited

codecov bot commented Apr 14, 2023 •

edited

t-nelson Apr 14, 2023

lijunwangs Apr 14, 2023

lijunwangs Apr 14, 2023

CriesofCarrots Apr 17, 2023

lijunwangs Apr 17, 2023

lijunwangs Apr 18, 2023

carllin Apr 20, 2023

t-nelson Apr 20, 2023

CriesofCarrots Apr 20, 2023

lijunwangs Apr 20, 2023

carllin commented Apr 25, 2023 •

edited

CriesofCarrots left a comment

lijunwangs commented Apr 25, 2023

t-nelson Apr 25, 2023

CriesofCarrots left a comment

t-nelson left a comment

lijunwangs commented May 3, 2023

fanatid commented May 3, 2023

t-nelson commented May 8, 2023

Fixed missing Root notifications via geyser plugin framework #31180

Fixed missing Root notifications via geyser plugin framework #31180

Conversation

lijunwangs commented Apr 13, 2023 • edited

Problem

Summary of Changes

codecov bot commented Apr 14, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllin commented Apr 25, 2023 • edited

CriesofCarrots left a comment

Choose a reason for hiding this comment

lijunwangs commented Apr 25, 2023

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

t-nelson left a comment

Choose a reason for hiding this comment

lijunwangs commented May 3, 2023

fanatid commented May 3, 2023

t-nelson commented May 8, 2023

lijunwangs commented Apr 13, 2023 •

edited

codecov bot commented Apr 14, 2023 •

edited

carllin commented Apr 25, 2023 •

edited