Banking_stage drops TooOld vote transactions

[tao-stones]

Problem

A lot of vote transactions are transmitted and replayed only to fail at instruction_process stage. Leader could not include TooOld Vote transactions into block to improve efficiency.

Summary of Changes

1. add check_redundant_votes function to check vote by current bank's vote_state.
2. banking_stage calls above function after accounts were locked, and before calling bank.load_and_execute_transactions, redundant vote transactions are marked as TransactionError::ProcessedAlready in result.
3. bank.rs is unchanged, no impact to replay_stage

Fixes #17877

[carllin]

Leader could not include TooOld Vote transactions into block to improve efficiency.

Yeah, iirc on our last call the actions we thought to take were:

1. Order the votes in the queue beforehand to try and maximize in order processing of the votes for each validator
2. Drop votes that are less than the latest vote in the vote state of the validator. This means introspecting the vote state in the banking stage before instruction execution, as you suggested 😃

[tao-stones]

2. Drop votes that are less than the latest vote in the vote state of the validator. This means introspecting the vote state in the banking stage before instruction execution, as you suggested 😃

Yea, I am exploring if there are light-weight options to detect "old" votes without having to invoke validator's VoteState entirely. Maybe banking_stage keeps a HashMap<validator_key, its_latest_voted_slot> , any votes from a validator has last vote earlier than its_latest_voted_slot will be dropped. Wondering if such approach worth the effort.

[tao-stones]

I ran today PR build in testnet-dev, which utilizes VoteState in banking_stage to check vote transactions against its validator's latest_voted_slot in detecting redundant votes; comparing to master build. I expected the number of transactions Banking_stage adds to block to be reduced somewhat, and hopefully also resulted in reduced timing on converting transactions (due to reduced number of TXs). The grafana seems to agree - the left part of graph that drops vote is bit more sparse that master build, just not in super convincing way.

@carllin @sakridge What do you feel about this approach and result?

[carllin]

The grafana seems to agree - the left part of graph that drops vote is bit more sparse that master build, just not in super convincing way.

@taozhu-chicago, what is the metric the first graph is measuring? Do we have a metric for measuring VoteTooOld failures to see the efficacy?

[carllin]

Hmm I think there's a race here. Even if the vote_redundancy_checker see slots 1->2->3->4 in order, the execution could still be out of order because ultimately what matters is the order in which these banking threads hit the process_and_record_transactions_locked() and grab the locks for the accounts

This implies this check should probably happen after we've acquired the account locks.

[tao-stones]

Super! Thanks for the important point - vote check should be done after bank is locked, and redundant votes are not to be submitted to bank.

[tao-stones]

on second thought, if banking_stage tracks last voted slot, and assume all transaction it let through will eventually landed in blockstore, it should be able to drop votes that voted for older slots (as implemented as solution 2 at https://github.com/solana-labs/solana/pull/19152/files#diff-bb8464c2f02c8863fb60e6e29f023f3e6da93a93fbb9199a70a44c9c1b41c7f3R1103-R1118 ). Do you think so?

[carllin]

Hmm, this still seems to run into the same issue. I can have two threads, one gets a vote for slot 1, one for slot 2 from the same validator. They insert the slots in order into latest_voted_slots, but the thread with slot 2 processes it's vote transaction first. Then the thread adding slot 1 will incur the TooOld error when it tries to be process the transaction.

[tao-stones]

right! the "solution 1", which banking_stage checks vote without involving bank, inherently has chances to pass old votes to bank for processing, though the possibilities are largely reduced (by filtering out out-of-order votes received by banking_stage). The benefit of this approach, as mentioned in Summary of Change above, is it doesn't require bank, which alights with bank-less leader goal.

The other solution implemented in this PR (https://github.com/solana-labs/solana/pull/19152/files#diff-ed47b4a0198313377e091bb3957bbbc63d937805426d1b2b6de39d0a50d32a0cR3461) checks vote in bank without race condition, but my concern is if this approach will be invalidated when implementing bank-less leader (I haven't thought it through, just a general concern)

At the end, we only need one of these two implementations in this PR. The debate is "which one" :)

[carllin]

A few things:

1. Doesn't the bankless leader design also require access to a bank in order to figure out whether an account has sufficient funds to pay the transaction fee?

2. It seems like the stateless VoteRedundancyChecker could theoretically also be used in solution 2 after we grab the locks?

3. If we go with the VoteRedundancyChecker, I also think we may need a separate VoteRedundancyChecker per bank because of forking. For example if you had

      1
  /       \
2          3

A vote for slot 1 may have landed in the bank for slot 2, but not for the bank in slot 3.

If you were to check the account state for slot 2 and 3 though this is not a problem because account fetch takes into account the fork on which the bank resides.

[tao-stones]

1. Yeah, will need bank to check account balance, would be great is just simply in-n-out for that purpose only. But I am not complete sure, not opposing to do check after account lock at all.
2. Yep, the Bank solution currently does the same thing VoteRedundantChecker does.
3. This is another point for using bank-based solution, so current implement (https://github.com/solana-labs/solana/pull/19152/files#diff-ed47b4a0198313377e091bb3957bbbc63d937805426d1b2b6de39d0a50d32a0cR3117) covers the forking scenario, i think

Answering though these questions, I started to see the benefit of bank-based solution, I'll remove the banking_stage based code, to make the PR easier for review.

[tao-stones]

The grafana seems to agree - the left part of graph that drops vote is bit more sparse that master build, just not in super convincing way.

@taozhu-chicago, what is the metric the first graph is measuring? Do we have a metric for measuring VoteTooOld failures to see the efficacy?

Sorry, me sloppy here. The first metric is the count of verified transactions (https://github.com/solana-labs/solana/pull/19152/files#diff-bb8464c2f02c8863fb60e6e29f023f3e6da93a93fbb9199a70a44c9c1b41c7f3L1069-L1072), if redundant votes are dropped, this counter should drop too.

The second n=metric is the time takes to convert packets (https://github.com/solana-labs/solana/pull/19152/files#diff-bb8464c2f02c8863fb60e6e29f023f3e6da93a93fbb9199a70a44c9c1b41c7f3L1163), with less transactions to convert, this metric should go down too.

[tao-stones]

settled on checking vote using current bank's VoteState, after accounts were locked, and right before TXs were loaded and executed. Still working on manipulating test bank instances to test redundancy check function. But the implementation is ready for review.

ran testnet cluster, the metrics shows almost half of votes were identified. as redundant and dropped (Orange are vote TXs count, and Blue are dropped vote TXs count):

The counters are: https://github.com/solana-labs/solana/pull/19152/files#diff-63a4285e3f76194a2aacf5358419e54cc8e7059cd8a997945d5491d14a333c60R24-R27

[tao-stones]

rebased master

[tao-stones]

The PR is consistently failing on CI local-cluster test test_fork_choice_refresh_old_votes, it panicked at this call during CI. (it doesn't fail if run the test individually on local machine, but @AshwinSekar was able to reproduce it.) It seems possible the block with TXs in it was not voted on properly, due to changes in this PR.

I traced the test by log, notice there are mainly two type vote tx are being dropped:

1. the duplicated vote:

• processed vote Vote { slots: [0, 1, 2, 3, 4], hash: 2QBgKPpebAQqRoL6BiL7UBR6wExB u3ydsVRUjsZij8x7, timestamp: Some(1630448707) } is good for slot 32 vote account BvmUKAJSXytTDdFc48xALWAYM7cgXrzKkb5DLZBQn2dv
• drops dup vote Vote { slots: [0, 1, 2, 3, 4], hash: 2QBgKPpebAQqRoL6BiL7UBR6wExB u3ydsVRUjsZij8x7, timestamp: Some(1630448707) } is redundant for slot 41 vote account BvmUKAJSXytTDdFc48xALWAYM7cgXrzKkb5DLZBQn2dv

2. the 'too old' vote:

• processed vote Vote { slots: [0, 1, 2, 3], hash: yTjm9L8BGPjM6qKXYeCv8zKenVWpRcz LCS59fdnHa6u, timestamp: Some(1630448707) } is good for slot 32 vote account BvmUKAJSXytTDdFc48xALWAYM7cgXrzKkb5DLZBQn2dv
• drop TooOld vote Vote { slots: [0, 1, 2], hash: 8WKp7SFz4YUb5QsySbNh8vkixJouwJ7ZAX P9AUmcagNH, timestamp: Some(1630448707) } is redundant for slot 32 vote account BvmUKAJSXytTDdFc48xALWAYM7cgXrzKkb5DLZBQn2dv

Are any of these case could possibly impact consensus negatively? @carllin

[carllin]

processed vote Vote { slots: [0, 1, 2, 3, 4], hash: 2QBgKPpebAQqRoL6BiL7UBR6wExB u3ydsVRUjsZij8x7, timestamp: Some(1630448707) } is good for slot 32 vote account BvmUKAJSXytTDdFc48xALWAYM7cgXrzKkb5DLZBQn2dv
drops dup vote Vote { slots: [0, 1, 2, 3, 4], hash: 2QBgKPpebAQqRoL6BiL7UBR6wExB u3ydsVRUjsZij8x7, timestamp: Some(1630448707) } is redundant for slot 41 vote account BvmUKAJSXytTDdFc48xALWAYM7cgXrzKkb5DLZBQn2dv

Hmmm dup vote shouldn't affect consensus, since it only needs one copy to be ingested

the 'too old' vote:
processed vote Vote { slots: [0, 1, 2, 3], hash: yTjm9L8BGPjM6qKXYeCv8zKenVWpRcz LCS59fdnHa6u, timestamp: Some(1630448707) } is good for slot 32 vote account BvmUKAJSXytTDdFc48xALWAYM7cgXrzKkb5DLZBQn2dv
drop TooOld vote Vote { slots: [0, 1, 2], hash: 8WKp7SFz4YUb5QsySbNh8vkixJouwJ7ZAX P9AUmcagNH, timestamp: Some(1630448707) } is redundant for slot 32 vote account BvmUKAJSXytTDdFc48xALWAYM7cgXrzKkb5DLZBQn2dv

I would be worried if the older, dropped vote had slots that were not present in the processed vote, but the old vote here is a strict subset of the newer vote, so all the slot information should have landed.

It seems like certain validators' towers aren't making progress with this change, it would be helpful to log the vote state that has landed in a bank on each bank in the validator:

solana/core/src/consensus.rs

Line 223 in 8ad52fa

let mut vote_state = match account.vote_state().as_ref() {

. Here, you can log the

1. bank slot
2. the vote_account_pubkey
3. Slots in the vote state and their lockout

Goal here is to see if roots are making progress.

Then compare to the local tower of each validator with that vote_account_pubkey, which can be observed by logging the local_vote_state every time the validator votes after this line here:

solana/core/src/consensus.rs

Line 369 in 8ad52fa

local_vote_state.process_vote_unchecked(&vote);

1. If it's an issue with votes not landing, the local tower will most likely look much different than the vote state that's landed on chain.
2. If the local tower is also not making progress, this means the validator is also not voting, in which case look for these logs:
3. https://github.com/solana-labs/solana/blob/8ad52fa095c0d51f212c7ecfac4c4f4b459e763d/core/src/replay_stage.rs#L586-L590from the validator that's not voting. This would most likely be due to the failed threshold check

   solana/core/src/replay_stage.rs

   Lines 2450 to 2452 in 8ad52fa

   	if !vote_threshold {
   	failure_reasons.push(HeaviestForkFailures::FailedThreshold(bank.slot()));
   	}

   , which is computed here:

   solana/core/src/replay_stage.rs

   Lines 2230 to 2231 in 8ad52fa

   	stats.vote_threshold =
   	tower.check_vote_stake_threshold(bank_slot, &stats.voted_stakes, stats.total_stake);

   . This check is also based on on-chain vote state, so can also be debugged by the printing of the on-chain vote state recommended above.

[tao-stones]

Thanks @carllin ! It makes good sense and super helpful. I'll do your suggestions -- helps me to understand consensus much more too.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This stale pull request has been automatically closed. Thank you for your contributions.