Fix fee mismatch on snapshot deserialize #12697

carllin · 2020-10-06T23:15:33Z

Problem

#12670 (comment)

Summary of Changes

As I see it, two options:

Proper fix: Serialize lamports_per_signature field and add gating to roll this out
Just take the lamports_per_signature value from the fee calculator on deserialize

Fixes #

carllin · 2020-10-06T23:16:42Z

sdk/src/fee_calculator.rs

@@ -67,7 +67,6 @@ impl FeeCalculator {
 pub struct FeeRateGovernor {
    // The current cost of a signature  This amount may increase/decrease over time based on
    // cluster processing load.
-    #[serde(skip)]


this is where gating would be necessary. Alternatively we just set this value on deserialize to bank.fee_calculator.lamports_per_signature

@ryoqun after talking with @t-nelson, it seems removing the skip here directly also breaks RPC, seems like the path of least resistance is to just manually assign this field for now. Thoughts?

the path of least resistance is to just manually assign this field for now. Thoughts?

@carllin yeah, I think this solution is good.

Also, when did this bug started to occur? Since beginning? ;)

hmmm yeah I would guess since it was first checked in xD

mvines · 2020-10-06T23:28:44Z

(+v1.3 label since this could also affect mainnet-beta once the pico-inflation feature is enabled)

ryoqun · 2020-10-07T05:34:51Z

Proper fix: Serialize lamports_per_signature field and add gating to roll this out

@carllin Also, note that gating here would not be the pretty solana features, rather would be a snapshot version bump.

ryoqun · 2020-10-07T05:41:20Z

@carllin First of all, thanks for debugging and actually fixing this inconsistency bug. :)

However, what I'm a bit wondering is that I felt that the vote mismatches were very likely to occur only when the solana-tps-bench is running (i.e. under high load).

For me, this looks like a deterministic inconsystency issue. Could this bug explain the indermisticic behavior I've observed? For one thing, my observation isn't so well formal. So I'm not conclusive. (I just sensed this way when tackling another issue (tower assertion fixes #12671)).

carllin · 2020-10-07T06:15:07Z

@ryoqun, you're right that this is a deterministic bug.

For me when following your steps to repro yesterday, I was seeing this bug repro on restart under high tps, I wonder if it has something to do with how the fee is calculated here: https://github.com/solana-labs/solana/blob/master/sdk/src/fee_calculator.rs#L138, which is derived from latest_signatures_per_slot, which needs to hit some threshold before it changes.

codecov · 2020-10-07T08:27:05Z

Codecov Report

Merging #12697 into master will decrease coverage by 0.0%.
The diff coverage is 95.5%.

@@            Coverage Diff            @@
##           master   #12697     +/-   ##
=========================================
- Coverage    81.9%    81.8%   -0.1%     
=========================================
  Files         353      353             
  Lines       84208    84243     +35     
=========================================
- Hits        69004    68990     -14     
- Misses      15204    15253     +49

ryoqun · 2020-10-07T17:15:25Z

@carllin this is LGTM for me even if this is still a draft. Is there any remaining work like write a unit-test? I think this is a bit hard.
Or you might be able to write one by hooking up a new path-through call flow with new_from_fields & get_fields_to_serialize and impl From<BankFieldsToDeserialize> for BankFieldsToSerialize

carllin · 2020-10-07T23:17:24Z

@ryoqun, looks like the compare_bank() function should have covered this case but was missing some cases. I modified the existing test_bank_serialize_newer to cover this case, what do you think?

ryoqun · 2020-10-08T12:18:38Z

runtime/src/bank.rs

-        if ptr::eq(self, dbank) {
-            return;
-        }


maybe this should be kept to deadlock free? #10466

Yeah, we should have added an comment...

ah yeah good catch, thanks!

ryoqun · 2020-10-08T12:20:33Z

runtime/src/bank.rs

+            && *self.inflation.read().unwrap() == *other.inflation.read().unwrap()
+            && *self.stakes.read().unwrap() == *other.stakes.read().unwrap()
+            && self.epoch_stakes == other.epoch_stakes
+            && self.is_delta.load(Relaxed) == other.is_delta.load(Relaxed)


I wonder how we can maintain this for the addition of newer fields... This problem exists from compare_bank.

ryoqun · 2020-10-08T12:21:40Z

runtime/src/bank.rs

@@ -477,6 +477,42 @@ pub(crate) struct BankFieldsToSerialize<'a> {
    pub(crate) is_delta: bool,
 }

+impl PartialEq for Bank {


By the way, why can't we just derive PartialEq? A comment might be desirable to explain the limitation.

hehe so the issue was RwLock doesn't implement PartialEq, and we can't implement it for RwLock because it doesn't belong to this crate, added comment!

ryoqun · 2020-10-08T12:22:21Z

runtime/src/serde_snapshot/tests.rs

@@ -219,7 +219,7 @@ fn test_bank_serialize_style(serde_style: SerdeStyle) {
    assert_eq!(dbank.get_balance(&key1.pubkey()), 0);
    assert_eq!(dbank.get_balance(&key2.pubkey()), 10);
    assert_eq!(dbank.get_balance(&key3.pubkey()), 0);
-    bank2.compare_bank(&dbank);
+    assert!(bank2 == dbank);


nits: how about assert_eq!?

The issue I ran into with assert_eq! was that it requires Bank implement Debug, which means all the program stack CowCachedExecutors -> Executor -> InvokeContext have to all implement Debug , and I didn't want to plumb all of that through 😩

@carllin FYI, I've took the initiative here after stressed with not being able to freely dbg!() bunch of bank fields: https://github.com/solana-labs/solana/pull/13017/files#diff-b5dead0a38e5f63447595bbeda1428d3aa851d33954343bb1bc38b4b00add5fdR159

ryoqun · 2020-10-08T12:23:12Z

runtime/src/serde_snapshot/tests.rs

@@ -219,7 +219,7 @@ fn test_bank_serialize_style(serde_style: SerdeStyle) {
    assert_eq!(dbank.get_balance(&key1.pubkey()), 0);
    assert_eq!(dbank.get_balance(&key2.pubkey()), 10);
    assert_eq!(dbank.get_balance(&key3.pubkey()), 0);
-    bank2.compare_bank(&dbank);


🎉

(I wonder why this was like this not defining ==.)..

ryoqun · 2020-10-08T12:24:04Z

@carllin reviewed. :)

ryoqun · 2020-10-08T12:24:51Z

@ryoqun, looks like the compare_bank() function should have covered this case but was missing some cases. I modified the existing test_bank_serialize_newer to cover this case, what do you think?

Really thanks for adding this. I'm very happy knowing that we're now covered under tests. :)

ryoqun

LGTM!

Co-authored-by: Carl Lin <carl@solana.com> (cherry picked from commit c879e7c) # Conflicts: # runtime/src/bank.rs

Co-authored-by: Carl Lin <carl@solana.com> (cherry picked from commit c879e7c)

Co-authored-by: Carl Lin <carl@solana.com> (cherry picked from commit c879e7c) Co-authored-by: carllin <wumu727@gmail.com>

Co-authored-by: Carl Lin <carl@solana.com> (cherry picked from commit c879e7c)

Co-authored-by: Carl Lin <carl@solana.com> (cherry picked from commit c879e7c) Co-authored-by: carllin <wumu727@gmail.com>

carllin requested review from ryoqun and t-nelson October 6, 2020 23:15

carllin commented Oct 6, 2020

View reviewed changes

mvines added the v1.3 label Oct 6, 2020

carllin force-pushed the FixMetrics branch from af4db45 to c17679b Compare October 7, 2020 07:19

ryoqun mentioned this pull request Oct 7, 2020

Detect vote hash mismatch even at run-sanity.sh #12690

Closed

carllin marked this pull request as ready for review October 7, 2020 18:57

carllin force-pushed the FixMetrics branch from 5605035 to 1ec5f39 Compare October 8, 2020 03:05

carllin added 2 commits October 7, 2020 20:14

Fix fee mismatch on snapshot deserialize

f00885f

Update unit test

29a63cd

carllin force-pushed the FixMetrics branch from 1ec5f39 to b09c916 Compare October 8, 2020 03:15

partialeq bank

986db74

carllin force-pushed the FixMetrics branch from b09c916 to 986db74 Compare October 8, 2020 03:19

mvines added the v1.4 label Oct 8, 2020

ryoqun reviewed Oct 8, 2020

View reviewed changes

PR comments

f4db4e6

ryoqun approved these changes Oct 9, 2020

View reviewed changes

carllin merged commit c879e7c into solana-labs:master Oct 9, 2020

mergify bot pushed a commit that referenced this pull request Oct 9, 2020

Fix fee mismatch on snapshot deserialize (#12697)

0e95c64

Co-authored-by: Carl Lin <carl@solana.com> (cherry picked from commit c879e7c) # Conflicts: # runtime/src/bank.rs

mergify bot mentioned this pull request Oct 9, 2020

Fix fee mismatch on snapshot deserialize (bp #12697) #12753

Merged

mergify bot pushed a commit that referenced this pull request Oct 9, 2020

Fix fee mismatch on snapshot deserialize (#12697)

fb454c7

Co-authored-by: Carl Lin <carl@solana.com> (cherry picked from commit c879e7c)

mergify bot mentioned this pull request Oct 9, 2020

Fix fee mismatch on snapshot deserialize (bp #12697) #12754

Merged

carllin added a commit that referenced this pull request Oct 9, 2020

Fix fee mismatch on snapshot deserialize (#12697)

a95be16

Co-authored-by: Carl Lin <carl@solana.com> (cherry picked from commit c879e7c)

mergify bot added a commit that referenced this pull request Oct 9, 2020

Fix fee mismatch on snapshot deserialize (#12697) (#12754)

9188153

Co-authored-by: Carl Lin <carl@solana.com> (cherry picked from commit c879e7c) Co-authored-by: carllin <wumu727@gmail.com>

carllin added a commit that referenced this pull request Oct 12, 2020

Fix fee mismatch on snapshot deserialize (#12697)

ce7bc5d

Co-authored-by: Carl Lin <carl@solana.com> (cherry picked from commit c879e7c)

carllin added a commit that referenced this pull request Oct 12, 2020

Fix fee mismatch on snapshot deserialize (#12697)

ab606c6

Co-authored-by: Carl Lin <carl@solana.com> (cherry picked from commit c879e7c)

carllin added a commit that referenced this pull request Oct 12, 2020

Fix fee mismatch on snapshot deserialize (#12697)

860373e

Co-authored-by: Carl Lin <carl@solana.com> (cherry picked from commit c879e7c)

mergify bot added a commit that referenced this pull request Oct 12, 2020

Fix fee mismatch on snapshot deserialize (#12697) (#12753)

edf59cc

Co-authored-by: Carl Lin <carl@solana.com> (cherry picked from commit c879e7c) Co-authored-by: carllin <wumu727@gmail.com>

mvines mentioned this pull request Oct 13, 2020

vote hash mismatch in testnet with high tps #12670

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fee mismatch on snapshot deserialize #12697

Fix fee mismatch on snapshot deserialize #12697

carllin commented Oct 6, 2020

carllin Oct 6, 2020

carllin Oct 7, 2020

ryoqun Oct 7, 2020

ryoqun Oct 7, 2020

carllin Oct 7, 2020

mvines commented Oct 6, 2020

ryoqun commented Oct 7, 2020

ryoqun commented Oct 7, 2020

carllin commented Oct 7, 2020 •

edited

Loading

codecov bot commented Oct 7, 2020 •

edited

Loading

ryoqun commented Oct 7, 2020

carllin commented Oct 7, 2020 •

edited

Loading

ryoqun Oct 8, 2020

carllin Oct 8, 2020

ryoqun Oct 8, 2020 •

edited

Loading

ryoqun Oct 8, 2020

carllin Oct 8, 2020 •

edited

Loading

ryoqun Oct 8, 2020

carllin Oct 8, 2020

ryoqun Oct 21, 2020

ryoqun Oct 8, 2020

ryoqun commented Oct 8, 2020

ryoqun commented Oct 8, 2020

ryoqun left a comment

Fix fee mismatch on snapshot deserialize #12697

Fix fee mismatch on snapshot deserialize #12697

Conversation

carllin commented Oct 6, 2020

Problem

Summary of Changes

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

mvines commented Oct 6, 2020

ryoqun commented Oct 7, 2020

ryoqun commented Oct 7, 2020

carllin commented Oct 7, 2020 • edited Loading

codecov bot commented Oct 7, 2020 • edited Loading

Codecov Report

ryoqun commented Oct 7, 2020

carllin commented Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllin Oct 8, 2020 • edited Loading

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

ryoqun commented Oct 8, 2020

ryoqun commented Oct 8, 2020

ryoqun left a comment

Choose a reason for hiding this comment

carllin commented Oct 7, 2020 •

edited

Loading

codecov bot commented Oct 7, 2020 •

edited

Loading

carllin commented Oct 7, 2020 •

edited

Loading

ryoqun Oct 8, 2020 •

edited

Loading

carllin Oct 8, 2020 •

edited

Loading